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

feat(gnovm): align Gno constant handling with Go specifications #2828

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

omarsy
Copy link
Member

@omarsy omarsy commented Sep 21, 2024

Related Issues:
Closes #2628

This PR aims to align Gno's constant handling with the Go specification regarding constant expressions (see Go Specification on Constants).

  1. Primitive Type Requirement

    • Should Work:
      const t = 1
    • Should Return an Error:
      const t = []string{"1"}
      Error:
      (const (slice[("1" string)] []string)) (value of type []string) is not constant
      
  2. Function Calls Disallowed
    Only built-in functions should be allowed.

    • Should Work:
      const t = len("s")
    • Should Return an Error:
      func v() string {
          return ""
      }
      const t = v()
      Error:
      v<VPBlock(3,0)>() (value of type string) is not constant
      
  3. Constant Operands Requirement
    Constant expressions may contain only constant operands and are evaluated at compile time.

    • Should Work:
      const t = 1
      const v = t
    • Should Raise an Error:
      t := 1
      const v = t
      Error:
      t (variable of type int) is not constant
      
  4. Type Assertion Forbidden

    • This code:
      var i interface{} = 1
      const t, ok = i.(int)
    • Should Raise This Error:
      i.(int) (comma, ok expression of type int) is not constant
      
  5. Index Expression Forbidden

    • This code:
      var i = []string{}
      const t, ok = i[0]
    • Should Return This Error:
      i[0] (variable of type string) is not constant
      
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 Sep 21, 2024
Copy link

codecov bot commented Sep 21, 2024

Codecov Report

Attention: Patch coverage is 74.13793% with 30 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/type_check.go 73.91% 26 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

@omarsy omarsy force-pushed the fix/2628 branch 4 times, most recently from c9b02d2 to f49c088 Compare September 25, 2024 11:54
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Sep 25, 2024
@omarsy omarsy force-pushed the fix/2628 branch 2 times, most recently from 355d799 to e9a9be7 Compare September 26, 2024 10:31
@omarsy omarsy marked this pull request as ready for review October 18, 2024 20:52
@omarsy omarsy requested review from jaekwon, moul, piux2, thehowl, mvertes and a team as code owners October 18, 2024 20:52
@omarsy omarsy changed the title feat(gnovm): align Gno constant handling with Go specifications WIP feat(gnovm): align Gno constant handling with Go specifications Oct 18, 2024
@jefft0
Copy link
Contributor

jefft0 commented Oct 19, 2024

@omarsy , can you fix the lint-pr-title CI check?

@jefft0 jefft0 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 19, 2024
@omarsy omarsy changed the title WIP feat(gnovm): align Gno constant handling with Go specifications feat(gnovm): align Gno constant handling with Go specifications Oct 19, 2024
@Kouteki Kouteki added this to the 🚀 Mainnet launch milestone Oct 22, 2024
gnovm/pkg/gnolang/type_check.go Outdated Show resolved Hide resolved
const t = 1 + 2 + len([]string{})
fmt.Println(t)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I test this one, seems not caught by your new added logic

package main

 import "fmt"

 func main() {
 	const t = 1 + 2 + len([2]string{})
 	fmt.Println(t)
 }

Copy link
Member Author

Choose a reason for hiding this comment

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

This will work because an array is permitted in a constant expression, whereas a slice is not.

switch vx := vx.(type) {
case *NameExpr:
t := evalStaticTypeOf(store, last, vx)
if _, ok := t.(*ArrayType); ok {
Copy link
Contributor

@ltzmaxwell ltzmaxwell Nov 25, 2024

Choose a reason for hiding this comment

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

doubt about this, only with len(arr)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes array is only variable allowed in a constant.

this should work on go:

package main

func main() {
	var l = [2]string{}
	const i = len(l)
	println(i)
}

this should not work:

package main

func main() {
	var l = "test"
	const i = len(l)
	println(i)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

consider this:

package main

func main() {
	const a = [2]int{1, 2}
	println(a)
}

and

package main

func main() {
	a := [2]int{1, 2}

	const b = a
}

these should not work, but works on the current branch.

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 I did a bad merge ^^

Copy link
Contributor

@ltzmaxwell ltzmaxwell Dec 9, 2024

Choose a reason for hiding this comment

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

hmmm, this logic seems incorrect:

if _, ok := t.(*ArrayType); ok {
	break Main
}

Due to this logic, the error in const43.gno was not caught.

I guess your intention is to check len(arr), right? I think that needs additional context to know if the outer expr is a built func call: len or max?

Not so sure, but we need to focus on ensuring that the logic of this function is correct(not limited to the above-mentioned).

@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Nov 26, 2024
@omarsy omarsy requested a review from ltzmaxwell November 28, 2024 20:55
@petar-dambovaliev
Copy link
Contributor

Part of your PR related to this but from what i see doesn't address the whole problem. I've fixed it here.

@Gno2D2
Copy link
Collaborator

Gno2D2 commented Nov 30, 2024

🛠 PR Checks Summary

🔴 Maintainers must be able to edit this pull request (more info)

Manual Checks (for Reviewers):
  • SKIP: Do not block the CI for this PR
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🔴 Maintainers must be able to edit this pull request (more info)
🟢 The pull request head branch must be up-to-date with its base (more info)

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: TERITORI/gno)

Then

🔴 Requirement not satisfied
└── 🔴 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base (more info)

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Head branch (TERITORI:fix/2628) is up to date with base (master): behind by 0 / ahead by 25

Manual Checks
**SKIP**: Do not block the CI for this PR

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

@petar-dambovaliev
Copy link
Contributor

Part of your PR related to this but from what i see doesn't address the whole problem. I've fixed it here.

@omarsy i am going to close my PR. You can integrate that weird Go feature for arrays in your PR. Better in the preprocessor so no functions are called during runtime.

@omarsy
Copy link
Member Author

omarsy commented Dec 3, 2024

Part of your PR related to this but from what i see doesn't address the whole problem. I've fixed it here.

@omarsy i am going to close my PR. You can integrate that weird Go feature for arrays in your PR. Better in the preprocessor so no functions are called during runtime.

I think it is better to handle it in another PR. WDYT ?

switch vx := vx.(type) {
case *NameExpr:
t := evalStaticTypeOf(store, last, vx)
if _, ok := t.(*ArrayType); ok {
Copy link
Contributor

@ltzmaxwell ltzmaxwell Dec 9, 2024

Choose a reason for hiding this comment

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

hmmm, this logic seems incorrect:

if _, ok := t.(*ArrayType); ok {
	break Main
}

Due to this logic, the error in const43.gno was not caught.

I guess your intention is to check len(arr), right? I think that needs additional context to know if the outer expr is a built func call: len or max?

Not so sure, but we need to focus on ensuring that the logic of this function is correct(not limited to the above-mentioned).

assertValidConstantExprRecursively(store, last, expr, nil)
}

func assertValidConstantExprRecursively(store Store, last BlockNode, currExpr, parentExpr Expr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func assertValidConstantExprRecursively(store Store, last BlockNode, currExpr, parentExpr Expr) {
func assertValidConstValue(store Store, last BlockNode, currExpr, parentExpr Expr) {

Copy link
Member Author

Choose a reason for hiding this comment

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

should be done here 54878f0

@omarsy omarsy requested a review from ltzmaxwell December 11, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

Constant Declaration with Slice Type Compiles Without Error
7 participants