Skip to content
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 problem #2689

Closed
wants to merge 29 commits into from
Closed

fix: addressability problem #2689

wants to merge 29 commits into from

Conversation

petar-dambovaliev
Copy link
Contributor

@petar-dambovaliev petar-dambovaliev commented Aug 13, 2024

Fixes issue
Alligns the behavior with the Go spec.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 13, 2024
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 68.18182% with 28 lines in your changes missing coverage. Please review.

Project coverage is 60.23%. Comparing base (aae5d49) to head (c26c7c3).

Files Patch % Lines
gnovm/pkg/gnolang/values.go 58.46% 23 Missing and 4 partials ⚠️
gnovm/pkg/gnolang/gonative.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2689   +/-   ##
=======================================
  Coverage   60.23%   60.23%           
=======================================
  Files         562      562           
  Lines       75091    75170   +79     
=======================================
+ Hits        45228    45281   +53     
- Misses      26482    26503   +21     
- Partials     3381     3386    +5     
Flag Coverage Δ
contribs/gnodev 61.40% <ø> (ø)
contribs/gnofaucet 14.46% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 20.23% <ø> (ø)
tm2 61.94% <ø> (-0.14%) ⬇️

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.

@petar-dambovaliev petar-dambovaliev marked this pull request as ready for review August 19, 2024 14:56
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it is close to completion but there a few more cases that need to be covered.

Also, is this something that we could do in the preprocessor? Like adding an attribute to expressions that are not currently addressable?

gnovm/pkg/gnolang/values.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/op_expressions.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/op_expressions.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/values.go Outdated Show resolved Hide resolved
@deelawn
Copy link
Contributor

deelawn commented Aug 20, 2024

Here is another case where execution should not succeed:

package main

func main() {
	var s S
	assert4(s)
}

type S struct {
	s string
}

// Should fail but does not
func assert4(i interface{}) {
	_ = &((i.(S)).s)
}

In this case the type assertion of the struct makes a copy of the value, so it is not addressable and should not be able to get the address of S.s.

@deelawn
Copy link
Contributor

deelawn commented Sep 6, 2024

Closing in favor of #2731

@deelawn deelawn closed this Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[bug/gnovm] arrays returned by functions should not be addressable
2 participants