fix(gnovm): enforce Go's addressability rules at preprocess - #6083
fix(gnovm): enforce Go's addressability rules at preprocess#6083jaekwon wants to merge 7 commits into
Conversation
…ility # Conflicts: # gnovm/tests/files/addressable_1d_err.gno
go/types reworded its unaddressable-slice diagnostic after Go 1.23: invalid operation: cannot slice x (value of type T) (value not addressable) cannot slice unaddressable value x (value of type T) go.mod pins go 1.25.9, which emits the second form, so the TypeCheckError lines in these two filetests no longer matched and the suite was red after merging master. Only the expectations move; no behavior changes. Verified independent of the addressability work that follows: both files pass with that change reverted.
Addresses the review on #5609. The helper answered for node kinds rather than for what a node denotes, so several forms still reached the runtime, and the one address the preprocessor synthesizes itself was never checked. isAddressable now answers the spec's question: - A name is addressable only when it denotes a variable. Delegated to StaticBlock.IsAssignable, which already records this declaratively (a package-level func decl lands in its block's UnassignableNames, uverse names are refused) and resolves in the innermost block, so a local shadowing a func keeps its own answer. Closes &funcName, which built a working *func() that could overwrite the function, and &pkg.Func. - The composite-literal exception moved to the & call site. It applies to that operand only, and the helper now also serves slicing and the pointer-method receiver. Closes &T{}.f, &[3]int{1,2,3}[0], [3]int{…}[:]. - A StarExpr is an indirection only when its operand is not a type; `*T` in the method expression (*T).M survives preprocess as a StarExpr. - String indexes are never addressable, a pointer-to-array index always is, an array element is addressable iff the array is, and a method value is not a field. - The selector arm now names every VPType and fails closed by default. Previously an unhandled path type fell through to the base, so an addressable base would have made the selection addressable. The synthesized receiver address for a pointer-method call is now gated too. The spec sentence quoted at that site begins "If x is addressable", but nothing checked it, so m["k"].Inc() mutated the stored map element, T{}.Inc() discarded the write, and mk().Inc() surfaced as "illegal assignment X expression type *gnolang.CallExpr" at runtime. Reported against the type written at the call site, not the embedded type a promoted method resolved to. &nil is reported instead of crashing: it has no static type, and the error path formatted that type, taking the preprocessor down with a nil dereference. Gas is unaffected — preprocess gas is charged per source byte, before preprocessing — and the package-qualified branch reads the *PackageValue from the import slot rather than evaluating it, which would spin up a sub-Machine inheriting the tx gas meter and bill per occurrence. Note the assignment path still accepts some of the same shapes (m["k"].n++ writes to the stored element), so that family is narrowed here, not closed. See the ADR. 15 _err filetests, each verified to fail against the unfixed preprocessor, plus two positive filetests covering the forms that must keep working. ADR: gnovm/adr/pr5609_addressability_at_preprocess.md
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):No automated checks match this pull request. ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
|
Follow-up from a review discussion on this PR: the No conflict with this PR — [AI-Assisted] |
Go has rules about when you are allowed to take the address of something. The
Gno VM did not follow all of them. This makes it follow them.
Builds on #5609 by @aronpark1007 (issue #5586). Their three commits are included
here unchanged. The review on that PR, by @notJoon, @thehowl and @davd-gzl, is
what drove everything added on top.
The problem
Some code that Go refuses to compile was accepted by the VM. Most of the time
that only meant a missing error message. But one case was worse.
If you call a method that takes a pointer receiver on a map entry, Go rejects it.
The VM allowed it, and the write went through to the stored value:
Two more in the same family:
And a few others that were simply accepted when they should not be:
The cause
The old check looked at the shape of the code rather than at what it actually
referred to. So a name was always treated as a variable, even when it was really
a function. A temporary value was always treated as addressable, even when it was
used as the base of something else.
There was also one place where the VM takes an address on your behalf. When you
call a pointer method, the VM inserts the
&for you. Nothing checked thataddress, even though the Go rule quoted in the comment right there begins "If x
is addressable". That is the source of the map bug above.
The fix
The check now asks the question the Go spec asks: is this a variable, a pointer
being followed, an element of a slice, a field of something addressable, or an
element of an addressable array? Everything else is refused.
It is also now applied in every place an address is taken, including the one the
VM inserts for you.
What this does not fix
Assignment has the same hole and is left alone here:
This is the family the original author said he wanted to handle in a follow-up,
so it is left for that. This PR makes the problem smaller, not gone. There is one
other small case left,
&T(v), which comes from an unrelated optimisation ratherthan from this rule. Both are described in the ADR.
Neither can reach the chain today. Adding or running a package always
type-checks first, and the type checker already rejects all of these.
How it was checked
two answers compared. They now agree on all 70. Before this change, 16
disagreed.
decorative. Two more tests cover the code that must keep working, and were
checked by deliberately breaking the fix to make sure they notice.
examples/still pass. Nothing that used to work stoppedworking.
command line tests, the gas tests, the integration tests, the formatters, and
the linter.
Speed and gas are unchanged. Gas for this stage is charged by the size of the
source file, before any of this runs, so extra checks cannot change it. A 124 KB
file built entirely out of the affected patterns takes the same time as before.
Two things worth a maintainer's opinion
IsAssignable. It gives the right answer, but it is the first place in thecodebase that uses it for something other than an assignment. The alternative
is a few more lines that work out the same fact by hand.
than at each place in the syntax. Only one of the two such points needed a
check, and it got one. Doing it as a general mechanism would be a bigger
change.
Written with AI assistance (Claude Code). Every finding was reproduced by running
it before being acted on. ADR included, as AGENTS.md asks.