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: achieve type assertion parity with go #1689

Merged
merged 30 commits into from
May 21, 2024

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Feb 23, 2024

Fixes #1650

This PR was originally intended to fix the case described in issue #1650, but it soon became apparent that there were other subtle inconsistencies between type assertions in go vs gno. The changes outlined here attempt to fix the issues and the new file tests ensure correctness. The summary below provides details as to the cases that were fixed for type assertions, both 1 and 2 -- 1 and 2 referring to the type assertion op codes, OpTypeAssert1 and OpTypeAssert2. The former handles type assertions with one LHS argument that panic on assertion failure and the latter handles those with two LHS arguments that assigns a boolean value dependent on success or failure.

Summary of type assertion changes

  • fail when the value being asserted has a nil type (i.e. the result of recover() when no panic has occurred). This applies for both cases where the type being asserted to is an interface or concrete type.
  • fail when asserting to an interface type and the underlying type of the value being asserted is also an interface type (non-concrete type)

Also, is this an accurate assumption?

// If the underlying native type is reflect.Interface kind, then this has no

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.

@deelawn deelawn added the 📦 🤖 gnovm Issues or PRs gnovm related label Feb 23, 2024
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.25%. Comparing base (229bf0e) to head (7685853).
Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1689      +/-   ##
==========================================
+ Coverage   45.10%   48.25%   +3.15%     
==========================================
  Files         464      408      -56     
  Lines       68039    62354    -5685     
==========================================
- Hits        30689    30092     -597     
+ Misses      34774    29752    -5022     
+ Partials     2576     2510      -66     

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

gnovm/pkg/gnolang/op_expressions.go Show resolved Hide resolved
gnovm/pkg/gnolang/op_expressions.go Outdated Show resolved Hide resolved
@ltzmaxwell ltzmaxwell self-requested a review April 15, 2024 08:47
@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented Apr 15, 2024

Hey nice work. took a peek mostly good. here are some of my questions:

  1. Please take a look at this one:
package main

type ex int

func (ex) Error() string { return "" }

func main() {
	r := []int(nil)
	_, ok := r.(ex)
	println(ok)
}

// Output:
// false

// Go_Output:
// invalid operation: r (variable of type []int) is not an interface

the behavior is inconsistent with go. in this case, r has a static type of []int, so it actually can not be asserted to another concrete type.

  1. And this one, this should be another problem actually:
package main

type ex int

func (ex) Error() string { return "" }

func main() {
	defer func() {
		r := _.(ex)
		println(r)
	}()
}

// Error:
// nil is not of type main.ex


// Go_Error:
// cannot use _ as value or type
  1. third one:
package main

type A interface {
	Do(s string)
}

func main() {
	defer func() {
		e := recover()
		_ = e.(A)
	}()
}

// Error:
// nil doesn't implement main.A

// Go_Error:
// panic: interface conversion: interface is nil, not main.A

This pertains to the error message related to type assertions. According to the Go language specification, type assertions only happens from an interface type to other types. see https://go.dev/ref/spec#Type_assertions

@deelawn
Copy link
Contributor Author

deelawn commented Apr 15, 2024

@ltzmaxwell so is it only the first case where the behavior is different between go and gno? The behavior of exampels 2 and 3 appear to be the same -- just the error messages are different. I'll fix the first and look at the ease of changing these error messages to something more specific.

@deelawn
Copy link
Contributor Author

deelawn commented Apr 16, 2024

@ltzmaxwell I've made changes to address your comments. Let me know what you think.

gnovm/pkg/gnolang/preprocess.go Show resolved Hide resolved
gnovm/tests/file.go Outdated Show resolved Hide resolved
@deelawn deelawn requested a review from mvertes as a code owner May 21, 2024 07:39
@deelawn deelawn merged commit 7d939a0 into gnolang:master May 21, 2024
14 checks passed
@deelawn deelawn deleted the bug/nil-typeassert2 branch May 21, 2024 07:47
leohhhn pushed a commit to leohhhn/gno that referenced this pull request May 21, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->
Fixes gnolang#1650 

This PR was originally intended to fix the case described in issue
gnolang#1650, but it soon became apparent that there were other subtle
inconsistencies between type assertions in go vs gno. The changes
outlined here attempt to fix the issues and the new file tests ensure
correctness. The summary below provides details as to the cases that
were fixed for type assertions, both 1 and 2 -- 1 and 2 referring to the
type assertion op codes, `OpTypeAssert1` and `OpTypeAssert2`. The former
handles type assertions with one LHS argument that panic on assertion
failure and the latter handles those with two LHS arguments that assigns
a boolean value dependent on success or failure.

### Summary of type assertion changes
- fail when the value being asserted has a nil type (i.e. the result of
`recover()` when no panic has occurred). This applies for both cases
where the type being asserted to is an interface or concrete type.
- fail when asserting to an interface type and the underlying type of
the value being asserted is also an interface type (non-concrete type)

Also, is this an accurate assumption?

https://github.com/gnolang/gno/blob/7d4e8e645ca1d2e89f5a8f2e85470e66b3253b33/gnovm/pkg/gnolang/op_expressions.go#L261

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
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
Status: 🚀 Needed for Launch
Status: Done
Development

Successfully merging this pull request may close these issues.

type assertion on recovered nils panics
5 participants