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: custom boolean types in conditional statements #2147

Merged
merged 12 commits into from
Jun 19, 2024

Conversation

nettijoe96
Copy link
Contributor

@nettijoe96 nettijoe96 commented May 20, 2024

Resolves #1087

Preprocess conditional expr with checkOrConvertBoolType

Unit test for if and for condition and integ test for invalid assignment. The invalid assignment is a case that fails to compile in Go, but succeeded in Gno before this change.

Run tests:

go test -run ^TestRunApp$ github.com/gnolang/gno/gnovm/cmd/gno 
go test -run ^TestFiles$ github.com/gnolang/gno/gnovm/tests
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.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label May 20, 2024
@nettijoe96 nettijoe96 changed the title Fix: custom boolean types in conditional statements fix: custom boolean types in conditional statements May 20, 2024
@thehowl
Copy link
Member

thehowl commented May 29, 2024

Hey, sorry for the wait. I'll take a look today!

@thehowl thehowl requested a review from ltzmaxwell May 29, 2024 12:18
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
@nettijoe96
Copy link
Contributor Author

Note: currently looking into why go test -timeout 30s -run ^TestFiles$ github.com/gnolang/gno/gnovm/tests is failing

@nettijoe96 nettijoe96 force-pushed the dev/nettijoe/cond-bool-type branch from bf5d7e5 to 20be919 Compare May 31, 2024 17:42
@thehowl
Copy link
Member

thehowl commented May 31, 2024

Note: currently looking into why go test -timeout 30s -run ^TestFiles$ github.com/gnolang/gno/gnovm/tests is failing

It seems like the expression can be nil, like in for loops where the conditional is optional. Let's remove the nil check.

convertConst(store, last, cx, BoolType)
} else if x != nil {
t := evalStaticTypeOf(store, last, x)
checkType(baseOf(t), BoolType, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

checkOrConvertIntegerType calls checkOrConvertType here, I wonder why there is asymmetry, and if we need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

also take a look at this, which I haven't reviewed; it makes changes to dependent functions.
#1426

Copy link
Contributor Author

@nettijoe96 nettijoe96 Jun 4, 2024

Choose a reason for hiding this comment

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

checkOrConvertIntegerType is calling checkIntegerType here:

func checkOrConvertIntegerType(store Store, last BlockNode, x Expr) {
if cx, ok := x.(*ConstExpr); ok {
convertConst(store, last, cx, IntType)
} else if x != nil {
xt := evalStaticTypeOf(store, last, x)
checkIntegerType(xt)
}
}

Copy link
Contributor Author

@nettijoe96 nettijoe96 Jun 4, 2024

Choose a reason for hiding this comment

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

also take a look at this, which I haven't reviewed; it makes changes to dependent functions. #1426

I think checkType would have to be changed to assertAssignableTo after that PR merges. I added a comment noting this

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.61%. Comparing base (71a298b) to head (552f46f).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2147   +/-   ##
=======================================
  Coverage   54.61%   54.61%           
=======================================
  Files         582      582           
  Lines       78372    78372           
=======================================
  Hits        42801    42801           
  Misses      32360    32360           
  Partials     3211     3211           
Flag Coverage Δ
gno.land 61.86% <ø> (ø)
gnovm 59.75% <ø> (ø)

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.

@jaekwon
Copy link
Contributor

jaekwon commented Jun 19, 2024

thank you! modified to use new functions.

@jaekwon jaekwon merged commit 3801d34 into gnolang:master Jun 19, 2024
49 checks passed
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.

conditional statements don't support custom boolean types
4 participants