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(gnovm): forbid star expression when value is not a pointer #2984

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

omarsy
Copy link
Member

@omarsy omarsy commented Oct 19, 2024

closes: #1088

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

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

codecov bot commented Oct 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.08%. Comparing base (f6bd2d3) to head (cc7ed1a).
Report is 40 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2984   +/-   ##
=======================================
  Coverage   63.08%   63.08%           
=======================================
  Files         563      563           
  Lines       79254    79258    +4     
=======================================
+ Hits        49998    50001    +3     
+ Misses      25896    25895    -1     
- Partials     3360     3362    +2     
Flag Coverage Δ
contribs/gnodev 60.62% <ø> (ø)
contribs/gnofaucet 14.82% <ø> (ø)
gno.land 67.56% <ø> (ø)
gnovm 67.25% <100.00%> (+<0.01%) ⬆️
misc/genstd 79.72% <ø> (ø)
misc/logos 19.45% <ø> (ø)
tm2 62.27% <ø> (+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.

@omarsy omarsy marked this pull request as ready for review October 19, 2024 16:00
@jefft0 jefft0 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 19, 2024
@notJoon
Copy link
Member

notJoon commented Oct 21, 2024

It looks fine to me. However, I found that for the following case, the error message slightly differs from what Go outputs:

package main

const VALUE = 5

func main() {
    println(*VALUE)
}

When this code is executed in Go, it produces the following error message:

invalid operation: cannot indirect VALUE (untyped int constant 5)

However, when run this code in gno, you can see const 5 is output instead of the constant name VALUE.

panic: invalid operation: cannot indirect (const (5 <untyped> bigint)) (variable of type <untyped> bigint) [recovered]
        panic: main/gnovm/tests/files/ptr10.gno:6:13: invalid operation: cannot indirect (const (5 <untyped> bigint)) (variable of type <untyped> bigint):
        --- preprocess stack ---
        stack 2: func main() { (const (println func(xs ...interface{})()))(*((const (5 <untyped> bigint)))) }
        stack 1: file{ package main; const VALUE<VPBlock(2,0)> = (const (5 <untyped> bigint)); func main() { (const (println func(xs ...interface{})()))(*((const (5 <untyped> bigint)))) } }
        stack 0: package(main)
        ------------------------

I think it would be helpful for debugging if it were made to output the constant's name like Go does.

Additionally, if you declare it with var instrade of const, VALUE is output correctly.

package main

var VALUE = 5

func main() {
    println(*VALUE)
}

// Output: panic: invalid operation: cannot indirect VALUE<VPBlock(3,0)> (variable of type int) [recovered]

@omarsy
Copy link
Member Author

omarsy commented Oct 24, 2024

It looks fine to me. However, I found that for the following case, the error message slightly differs from what Go outputs:

package main

const VALUE = 5

func main() {
    println(*VALUE)
}

When this code is executed in Go, it produces the following error message:

invalid operation: cannot indirect VALUE (untyped int constant 5)

However, when run this code in gno, you can see const 5 is output instead of the constant name VALUE.

panic: invalid operation: cannot indirect (const (5 <untyped> bigint)) (variable of type <untyped> bigint) [recovered]
        panic: main/gnovm/tests/files/ptr10.gno:6:13: invalid operation: cannot indirect (const (5 <untyped> bigint)) (variable of type <untyped> bigint):
        --- preprocess stack ---
        stack 2: func main() { (const (println func(xs ...interface{})()))(*((const (5 <untyped> bigint)))) }
        stack 1: file{ package main; const VALUE<VPBlock(2,0)> = (const (5 <untyped> bigint)); func main() { (const (println func(xs ...interface{})()))(*((const (5 <untyped> bigint)))) } }
        stack 0: package(main)
        ------------------------

I think it would be helpful for debugging if it were made to output the constant's name like Go does.

Additionally, if you declare it with var instrade of const, VALUE is output correctly.

package main

var VALUE = 5

func main() {
    println(*VALUE)
}

// Output: panic: invalid operation: cannot indirect VALUE<VPBlock(3,0)> (variable of type int) [recovered]

@notJoon
Thank you for your input! The difference in how ConstExpr and NameExpr are displayed stems from the specific formatting logic defined in their respective implementations. You can see this in the nodes_string.go file:

  • For ConstExpr, the display logic is found here.
  • For NameExpr, the relevant logic is located here.

@notJoon
Copy link
Member

notJoon commented Oct 30, 2024

It looks fine to me. However, I found that for the following case, the error message slightly differs from what Go outputs:

package main

const VALUE = 5

func main() {
    println(*VALUE)
}

When this code is executed in Go, it produces the following error message:

invalid operation: cannot indirect VALUE (untyped int constant 5)

However, when run this code in gno, you can see const 5 is output instead of the constant name VALUE.

panic: invalid operation: cannot indirect (const (5 <untyped> bigint)) (variable of type <untyped> bigint) [recovered]
        panic: main/gnovm/tests/files/ptr10.gno:6:13: invalid operation: cannot indirect (const (5 <untyped> bigint)) (variable of type <untyped> bigint):
        --- preprocess stack ---
        stack 2: func main() { (const (println func(xs ...interface{})()))(*((const (5 <untyped> bigint)))) }
        stack 1: file{ package main; const VALUE<VPBlock(2,0)> = (const (5 <untyped> bigint)); func main() { (const (println func(xs ...interface{})()))(*((const (5 <untyped> bigint)))) } }
        stack 0: package(main)
        ------------------------

I think it would be helpful for debugging if it were made to output the constant's name like Go does.
Additionally, if you declare it with var instrade of const, VALUE is output correctly.

package main

var VALUE = 5

func main() {
    println(*VALUE)
}

// Output: panic: invalid operation: cannot indirect VALUE<VPBlock(3,0)> (variable of type int) [recovered]

@notJoon Thank you for your input! The difference in how ConstExpr and NameExpr are displayed stems from the specific formatting logic defined in their respective implementations. You can see this in the nodes_string.go file:

* For `ConstExpr`, the display logic is found [here](https://github.com/gnolang/gno/blob/master/gnovm/pkg/gnolang/nodes_string.go#L552).

* For `NameExpr`, the relevant logic is located [here](https://github.com/gnolang/gno/blob/master/gnovm/pkg/gnolang/nodes_string.go#L98).

Thanks for response. it seems reasonable to me. So I'll remove the triage-pending flag.

@notJoon notJoon removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 30, 2024
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

LGTM.

@thehowl thehowl changed the title fix(gnovm): forbid start expression when value is not a pointer fix(gnovm): forbid star expression when value is not a pointer Oct 30, 2024
@thehowl thehowl merged commit 850182c into gnolang:master Oct 30, 2024
124 checks passed
case *StarExpr:
xt := evalStaticTypeOf(store, last, n.X)
if xt.Kind() != PointerKind && xt.Kind() != TypeKind {
panic(fmt.Sprintf("invalid operation: cannot indirect %s (variable of type %s)", n.X.String(), xt.String()))
Copy link
Contributor

@ltzmaxwell ltzmaxwell Oct 30, 2024

Choose a reason for hiding this comment

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

consider this one:

package main

func main() {
	println(*nil)
}

but yes I'll make a new issue: #3052

Copy link
Member

Choose a reason for hiding this comment

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

always ahead of the game :) should have let you take a look first

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch. Missed this one ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

did a fix here: #3053

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.

cryptic error: invalid dereferences (*<Expr>) panic with should not happen
5 participants