fix: addressability #2731

deelawn · 2024-08-27T00:21:03Z

Closes #2299.

This PR enforces addressability rules from the go spec. Basically, you can't take references of values that don't have addresses, which makes sense. I think it's important to have the VM's behavior match Go's in this regard. If this isn't in place before we launch and we decide to do it later, we run the risk of breaking realms with code that doesn't adhere to the addressability rules. Omitting addressability rules may also have unforeseen future consequences when we decide to make the VM compatible with newer Go language features.

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2024-08-27T00:22:46Z

Codecov Report

Attention: Patch coverage is 59.79381% with 39 lines in your changes missing coverage. Please review.

Project coverage is 60.91%. Comparing base (9897b66) to head (71afb10).

Files with missing lines	Patch %	Lines
gnovm/pkg/gnolang/nodes.go	46.57%	37 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2731      +/-   ##
==========================================
+ Coverage   60.87%   60.91%   +0.04%     
==========================================
  Files         563      563              
  Lines       75193    75250      +57     
==========================================
+ Hits        45770    45839      +69     
+ Misses      26055    26047       -8     
+ Partials     3368     3364       -4

Flag	Coverage Δ
contribs/gnodev	`61.46% <ø> (ø)`
contribs/gnofaucet	`14.46% <ø> (-0.86%)`	⬇️
gno.land	`67.17% <ø> (ø)`
gnovm	`65.76% <59.79%> (+0.12%)`	⬆️
misc/genstd	`80.54% <ø> (ø)`
misc/logos	`20.23% <ø> (ø)`
tm2	`62.07% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ltzmaxwell · 2024-08-29T10:38:03Z

start looking, this seems to be a missing case:

package main

import "fmt"

type MyStruct struct {
	Mp *int
}

func makeT() MyStruct {
	x := 10
	return MyStruct{Mp: &x}
}

func main() {
	p := &makeT().Mp // This would cause an error because you cannot take the address of a struct field when the struct is a direct return value of a function.
	fmt.Println(p)
}

ltzmaxwell · 2024-08-29T10:55:21Z

gnovm/pkg/gnolang/preprocess.go

+					baseType := baseOf(ft.Results[0].Type)
+					switch baseType.(type) {
+					case *PointerType, *SliceType:
+						n.IsAddressable = true


I know this is out of scope, but the error should be about assignment mismatch.

package main func foo() ([]int, []string) { return []int{1, 2, 3}, []string{"a", "b", "c"} } func main() { r1 := &foo() } // Error: // cannot take address of foo<VPBlock(3,0)>():

This one is actually in scope; you're right that in this case it doesn't make sense for the addressability error message to supersede the results mismatch. In fact, the addressability message doesn't make sense because it is trying to take the address of multiple values. Addressed this here: 3f4f1b6

ltzmaxwell · 2024-08-29T11:15:14Z

another one out of scope too: 😆

package main

type MyInt int

func main() {

	_ = &MyInt
	_ = MyInt

}

ltzmaxwell · 2024-08-29T11:20:41Z

gnovm/pkg/gnolang/nodes.go

@@ -502,6 +561,10 @@ type FuncLitExpr struct {
 	Body              // function body
 }

+func (x *FuncLitExpr) isAddressable() bool {
+	return false


this will be updated based on the result type, right?

No. This is a function literal, not an execution. Hopefully the tests added here make this more clear af12ca1

gnovm/pkg/gnolang/nodes.go

deelawn · 2024-08-29T23:32:05Z

another one out of scope too: 😆

package main

type MyInt int

func main() {

	_ = &MyInt
	_ = MyInt

}

Yeah, good catch but out of scope. But 3f4f1b6 ensures that when this is fixed that it won't be an addressability error message that is displayed.

petar-dambovaliev

Good work.

ltzmaxwell · 2024-09-11T07:53:26Z

gnovm/tests/files/addressable_7b_err.gno

+	_ = &MyInt
+}
+
+// Error:


No error msg?

Ha, yeah you're right. I removed this test. It should fail, but doesn't. I created an issue for this here: #2787

gnovm/tests/files/addressable_7a_err.gno

thehowl

A bunch of proposed reorgs, overall implementation looks good, good job 🎉

thehowl · 2024-09-12T17:29:57Z

gnovm/pkg/gnolang/addressability.go

This should just be in nodes.go

Ideally nodes.go would b less than 2200 LOC, but I've moved this back here as you suggested since moving a single concept into a new file that is closely related to this may be premature; a larger reorganization is necessary here. 0e80801

thehowl · 2024-09-12T17:46:28Z

gnovm/pkg/gnolang/nodes.go

@@ -328,6 +328,7 @@ var (
 type Expr interface {
 	Node
 	assertExpr()
+	addressability() Addressability


Suggested change

addressability() Addressability

Addressable() Addressability

If Addressability is exported, so should the method to get it.

Neither should really be exported; I was just running short on names. I renamed it so this is no longer an issue. 0e80801

thehowl · 2024-09-12T17:58:17Z

gnovm/pkg/gnolang/nodes.go

@@ -374,6 +375,10 @@ type NameExpr struct {
 	Name
 }

+func (x *NameExpr) addressability() Addressability {


I've been thinking about what's the best approach here.

Part of me would prefer if this was just a isAddressable if statement with a big type switch. However, I think there's a good point in your approach:

There's many expressions where addressability depends on the "children" expressions; having them as fields allow us to memoize in Preprocess what their result is.

Having this method as a requirement of the Expr interface means all Expr types must implement it, avoiding us to have to write a should-not-happen kind of panic.

But I do think we should concentrate the rules for addressability in some code packed tightly together, so it can be easily cross-checked with the Go reference in hand.

So what do you say about moving all of these methods underneath the block of assertExpr methods?

I think I was able to do even one better here and remove the assertExpr method entirely. Now that Expr has a meaningful method, addressability, there is no need for an extra method just to satisfy the interface. Given that change, let's keep the method implementations with each of the types; it is the natural place where one would expect them to be. 804c17e

thehowl · 2024-09-12T18:04:25Z

gnovm/pkg/gnolang/nodes.go

+	Args           Exprs // function arguments, if any.
+	Varg           bool  // if true, final arg is variadic.
+	NumArgs        int   // len(Args) or len(Args[0].Results)
+	Addressability Addressability


For the cases like this where the addressability is assigned during preprocess, what do you think about moving Addressability as an attribute?

The reason why I say this is that we currently have a bunch of information which is not actually persisted, but recovered by using Preprocess. The Expr types here contain in their struct only information which comes directly out of the AST representation; not from preprocessing. I'm not entirely sure of why it's designed this way; but I think for consistency we should maintain the split between "preprocessed information" in Attributes, and AST information in the struct themselves.

If addressability is an attribute, I also wonder if there's a way to remove the specific type and turn this into Addressable() bool instead.

thehowl · 2024-09-12T18:05:24Z

gnovm/pkg/gnolang/nodes.go

+
+func (x *IndexExpr) addressability() Addressability {
+	// In this case NotApplicable means that it wasn't set, the default value.
+	if x.Addressability == AddressabilityNotApplicable {


When does this happen?

This happens when it is not set in TRANS_LEAVE, here

gno/gnovm/pkg/gnolang/preprocess.go

Lines 1500 to 1506 in 804c17e

if dt.Kind() == SliceKind {

// A string index is not addressable.

n.Addressability = addressabilityStatusSatisfied

} else if dt.Kind() == StringKind {

// Special case; string indexes are never addressable.

n.Addressability = addressabilityStatusUnsatisfied

}

I've added a clarifying comment. 44cc298

thehowl · 2024-09-12T18:07:55Z

gnovm/pkg/gnolang/nodes.go

@@ -430,16 +471,33 @@ type StarExpr struct { // *X
 	X Expr // operand
 }

+func (x *StarExpr) addressability() Addressability {
+	return x.X.addressability()


Can you follow the code coverage annotations to add more test cases?

thehowl · 2024-09-12T18:09:16Z

gnovm/pkg/gnolang/nodes.go

 type FuncTypeExpr struct {
 	Attributes
 	Params  FieldTypeExprs // (incoming) parameters, if any.
 	Results FieldTypeExprs // (outgoing) results, if any.
 }

+func (x *FuncTypeExpr) addressability() Addressability {
+	return AddressabilityNotApplicable


For these cases where we just return NotApplicable, I wonder if we should just panic...

In the case of type expressions, yes, we should panic because it is unexpected to have this method called 8bec242

But we shouldn't get rid of NotApplicable altogether. There are some valid cases, for example, when there is code like this:

func main() { _ = &DoSomething() } func DoSomething() (int, int) { ... }

In this case addressability is not applicable because it should fail with an error message referencing the fact that it is trying to take a reference, which requires one operand, on the result of a function that returns multiple values, so we don't want to trigger an addressability error here. In Go, this would panic with something like multiple-value DoSomething() (value of type (int, int)) in single-value context.

thehowl · 2024-09-12T18:11:39Z

gnovm/pkg/gnolang/preprocess.go

 				}
 				switch dt.Kind() {
 				case StringKind, ArrayKind, SliceKind:
 					// Replace const index with int *ConstExpr,
 					// or if not const, assert integer type..
 					checkOrConvertIntegerKind(store, last, n.Index)
+					if dt.Kind() == SliceKind {
+						// A string index is not addressable.


Confused by the comment

Also, maybe add a comment to explain what happens when dt is an array?

Thanks. This comment is wrong. Updated. 548af87

If it is an array, it will defer to the array's addressability. Comment added. 26e8529

thehowl · 2024-09-12T18:16:35Z

gnovm/pkg/gnolang/preprocess.go

@@ -1642,6 +1669,10 @@ func Preprocess(store Store, ctx BlockNode, n Node) Node {
 					}
 				}

+				if ftype == TRANS_REF_X {


Add an explanatory comment?

Added it. While looking at this again, I also noticed a case that wasn't previously covered, the double reference -- something like &(&value) -- so I've fixed that here as well. b33f0b0

The first fix was premature. I've corrected it here so that the error message is more correct. 71afb10

preprocessor addressability work

c8462ec

github-actions bot assigned deelawn Aug 27, 2024

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 27, 2024

deelawn added 14 commits August 26, 2024 17:40

fix selector isaddressable

ef97274

fix starexpr isaddressable

7fa9f50

fix sliceexpr addressability

08563c3

fix typeassertexpr addressability

3b78ae9

fix callexpr addressable return types

a0e13b9

mark append and new results as addressable

ae55b84

fix selector addressability

990017d

make string indexes non-addressable

3621dc7

addressability file tests

9faf3df

prohibit taking address of imported typed constant

8671ea5

more tests

816dbd6

fix type assertion addressability and add tests

77caea9

make sure to use base types when considering addressability

89a041c

move tests

9956c26

ltzmaxwell reviewed Aug 29, 2024

View reviewed changes

deelawn marked this pull request as ready for review August 29, 2024 15:52

deelawn requested review from jaekwon, moul, piux2, thehowl and mvertes as code owners August 29, 2024 15:52

thehowl reviewed Aug 29, 2024

View reviewed changes

gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved

fix index and selector addressability

6940714

deelawn added 6 commits August 29, 2024 16:51

fixed test

c16e5e9

fixed test

ac79f00

fix test

df764cd

func lit tests

af12ca1

index, map, and call expr fixes and tests

e498f1c

addressability consistentcy fixes

bee868c

petar-dambovaliev self-requested a review August 31, 2024 00:58

petar-dambovaliev approved these changes Aug 31, 2024

View reviewed changes

Kouteki mentioned this pull request Sep 3, 2024

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

consolidate simplest block path traversal

9698461

deelawn mentioned this pull request Sep 6, 2024

fix: addressability problem #2689

Closed

fix comment

b284b56

ltzmaxwell reviewed Sep 11, 2024

View reviewed changes

gnovm/tests/files/addressable_7a_err.gno Show resolved Hide resolved

Removed test and renamed other

dfe024f

thehowl reviewed Sep 12, 2024

View reviewed changes

michelleellen mentioned this pull request Sep 17, 2024

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

deelawn added 12 commits September 19, 2024 12:24

move and rename addressability constants

0e80801

remove assertExpr

804c17e

clarifying comment

44cc298

fixed comment

548af87

panic when calling addressability on type expressions

8bec242

fixed tesst

8fe3b37

added comment

26e8529

Merge branch 'master' into fix/addressability

3c120ae

fixed test

feee13d

fix double reference addressability and add comments

b33f0b0

fixed test

a8827ff

corrected double ref addressability check

71afb10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: addressability #2731

fix: addressability #2731

deelawn commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

ltzmaxwell commented Aug 29, 2024

ltzmaxwell Aug 29, 2024

deelawn Aug 29, 2024

ltzmaxwell commented Aug 29, 2024 •

edited

Loading

ltzmaxwell Aug 29, 2024

deelawn Aug 30, 2024

deelawn commented Aug 29, 2024

petar-dambovaliev left a comment

ltzmaxwell Sep 11, 2024

deelawn Sep 12, 2024

thehowl left a comment

thehowl Sep 12, 2024

deelawn Sep 19, 2024

thehowl Sep 12, 2024

deelawn Sep 19, 2024

thehowl Sep 12, 2024

deelawn Sep 19, 2024

thehowl Sep 12, 2024

thehowl Sep 12, 2024

deelawn Sep 19, 2024

thehowl Sep 12, 2024

thehowl Sep 12, 2024

deelawn Sep 21, 2024

thehowl Sep 12, 2024

deelawn Sep 19, 2024

deelawn Sep 21, 2024

thehowl Sep 12, 2024

deelawn Sep 21, 2024

	if dt.Kind() == SliceKind {
	// A string index is not addressable.
	n.Addressability = addressabilityStatusSatisfied
	} else if dt.Kind() == StringKind {
	// Special case; string indexes are never addressable.
	n.Addressability = addressabilityStatusUnsatisfied
	}

fix: addressability #2731

Are you sure you want to change the base?

fix: addressability #2731

Conversation

deelawn commented Aug 27, 2024 • edited Loading

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

ltzmaxwell commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltzmaxwell commented Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deelawn commented Aug 29, 2024

petar-dambovaliev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehowl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deelawn commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

ltzmaxwell commented Aug 29, 2024 •

edited

Loading