-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: addressability #2731
base: master
Are you sure you want to change the base?
fix: addressability #2731
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2731 +/- ##
==========================================
+ Coverage 60.87% 60.90% +0.03%
==========================================
Files 563 563
Lines 75193 75249 +56
==========================================
+ Hits 45770 45827 +57
+ Misses 26055 26053 -2
- Partials 3368 3369 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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)
} |
gnovm/pkg/gnolang/preprocess.go
Outdated
baseType := baseOf(ft.Results[0].Type) | ||
switch baseType.(type) { | ||
case *PointerType, *SliceType: | ||
n.IsAddressable = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)>():
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
another one out of scope too: 😆 package main
type MyInt int
func main() {
_ = &MyInt
_ = MyInt
} |
gnovm/pkg/gnolang/nodes.go
Outdated
@@ -502,6 +561,10 @@ type FuncLitExpr struct { | |||
Body // function body | |||
} | |||
|
|||
func (x *FuncLitExpr) isAddressable() bool { | |||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be updated based on the result type, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This is a function literal, not an execution. Hopefully the tests added here make this more clear af12ca1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work.
_ = &MyInt | ||
} | ||
|
||
// Error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No error msg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, yeah you're right. I removed this test. It should fail, but doesn't. I created an issue for this here: #2787
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of proposed reorgs, overall implementation looks good, good job 🎉
gnovm/pkg/gnolang/addressability.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be in nodes.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
gnovm/pkg/gnolang/nodes.go
Outdated
@@ -328,6 +328,7 @@ var ( | |||
type Expr interface { | |||
Node | |||
assertExpr() | |||
addressability() Addressability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressability() Addressability | |
Addressable() Addressability |
If Addressability is exported, so should the method to get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither should really be exported; I was just running short on names. I renamed it so this is no longer an issue. 0e80801
gnovm/pkg/gnolang/nodes.go
Outdated
@@ -374,6 +375,10 @@ type NameExpr struct { | |||
Name | |||
} | |||
|
|||
func (x *NameExpr) addressability() Addressability { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
gnovm/pkg/gnolang/nodes.go
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
gnovm/pkg/gnolang/nodes.go
Outdated
|
||
func (x *IndexExpr) addressability() Addressability { | ||
// In this case NotApplicable means that it wasn't set, the default value. | ||
if x.Addressability == AddressabilityNotApplicable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does this happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
gnovm/pkg/gnolang/nodes.go
Outdated
@@ -430,16 +471,33 @@ type StarExpr struct { // *X | |||
X Expr // operand | |||
} | |||
|
|||
func (x *StarExpr) addressability() Addressability { | |||
return x.X.addressability() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you follow the code coverage annotations to add more test cases?
gnovm/pkg/gnolang/nodes.go
Outdated
type FuncTypeExpr struct { | ||
Attributes | ||
Params FieldTypeExprs // (incoming) parameters, if any. | ||
Results FieldTypeExprs // (outgoing) results, if any. | ||
} | ||
|
||
func (x *FuncTypeExpr) addressability() Addressability { | ||
return AddressabilityNotApplicable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these cases where we just return NotApplicable, I wonder if we should just panic...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
gnovm/pkg/gnolang/preprocess.go
Outdated
} | ||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused by the comment
Also, maybe add a comment to explain what happens when dt is an array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This comment is wrong. Updated. 548af87
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is an array, it will defer to the array's addressability. Comment added. 26e8529
@@ -1642,6 +1669,10 @@ func Preprocess(store Store, ctx BlockNode, n Node) Node { | |||
} | |||
} | |||
|
|||
if ftype == TRANS_REF_X { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an explanatory comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
BREAKING CHANGE: xxx
message was included in the description