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): check const type and values expr #2651

Closed

Conversation

MikaelVallenet
Copy link
Contributor

closes #2628

this code should not work following the issue above since the type is not valid for const

package main

func main() {
	const b []string = []string{}
	println("Hello", b)
}

The proposal avoid any expression that is not a basic litteral or a name expr fixing the issue above.

Ideally, we'd need to check that NameExpr have good underlying type & value to fix the underlying code

package main

func main() {
	var x = 10
	const y = -x
}
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 Aug 1, 2024
@MikaelVallenet MikaelVallenet changed the title Gnovm check const type expr fix(gnovm): check const type and values expr Aug 1, 2024
@MikaelVallenet MikaelVallenet marked this pull request as ready for review August 1, 2024 12:01
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.15%. Comparing base (14b9015) to head (b45887d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2651   +/-   ##
=======================================
  Coverage   60.15%   60.15%           
=======================================
  Files         560      560           
  Lines       74738    74738           
=======================================
+ Hits        44955    44958    +3     
+ Misses      26397    26394    -3     
  Partials     3386     3386           
Flag Coverage Δ
contribs/gnofaucet 15.31% <ø> (+0.85%) ⬆️
misc/genstd 80.54% <ø> (ø)
misc/logos 19.88% <ø> (ø)
tm2 62.03% <ø> (+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.

@ltzmaxwell
Copy link
Contributor

thank you for your work! can you add a test for the fix?

Comment on lines +243 to +244
case *CallExpr:
checkValConstValue(x.Func)
Copy link
Contributor

Choose a reason for hiding this comment

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

see this case:

package main

type f func()

func foo() {
	println("foo")
}

const ff = foo

func main() {
	println("ok")
}

it should be excluded, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for comment, In fact, I'm having a bit of trouble figuring out how to check that a name expr is a const or a cast of a basic type, for example const x = uint(8), do you have any tips to give me on this? In the meantime, I'll keep working on the code and keep looking, thanks.

Copy link
Contributor

@ltzmaxwell ltzmaxwell Aug 14, 2024

Choose a reason for hiding this comment

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

It seems there may be a misunderstanding about the use of nameExpr as the right-hand side (RHS) of a const declaration. For example:

const a = 1
// 'a' qualifies as a ConstExpr
const b = a

In this case, using a as the RHS in the declaration of b is perfectly valid since a is evaluated to be a constant expression.

Regarding the expression uint(8), this is a CallExpr with the function type(TypeType) of uint8. I'm not certain if this directly addresses your question.

However, I recommend we ensure a shared understanding of the related concepts by reviewing the section on constants in the Go language specification: Go Constants.

Tips, you can try to conduct checkValConstValue in trans_leave *ValueDecl to see if it works.

Comment on lines +238 to +239
case *BinaryExpr:
checkValConstValue(x.Left)
Copy link
Contributor

Choose a reason for hiding this comment

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

also this one:

package main


var x = 1

const ff = 1 << x

func main() {
	println("ok")
}

Copy link
Contributor Author

@MikaelVallenet MikaelVallenet Aug 21, 2024

Choose a reason for hiding this comment

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

Hello @ltzmaxwell,

I understand better thank you, however not all RHS are constExpr by default and this code is then executed to transform them at the last moment.

if n.Const {
	// NOTE: may or may not be a *ConstExpr,
	// but if not, make one now.
	for i, vx := range n.Values {
		n.Values[i] = evalConst(store, last, vx)
	}
}

So i tried to just change this code by adding a if !isConst(vx) -> panic but some expr are not yet evaluted as const.

I'm wondering whether I should try to evaluate all constant expressions as constExpr before reaching the snippet of code above or continue along the path of checking each type of expression, which can be complicated by the fact that special cases exist in many cases?

I also noticed that this code evaluates the const Minute as a constExpr.

package main

import (
	"fmt"
)

type Duration int64

const (
	Nanosecond  Duration = 1
	Microsecond          = 1000 * Nanosecond
	Millisecond          = 1000 * Microsecond
	Second               = 1000 * Millisecond
	Minute               = 60 * Second
	Hour                 = 60 * Minute
)

const df = Minute

func main() {
	fmt.Printf("df: %v %T\n", df, df)
}

but this code where i import the pkg time does not evalue the RHS time.Duration of the const decl. as a constExpr before the final loop, which transforms the RHS into constExpr regardless of its value.

package main

import (
	"fmt"
	"time"
)

const df = time.Minute

func main() {
	fmt.Printf("df: %v %T\n", df, df)
}

It's one of my problems right now.
I closed the PR for now since i think i need more vision and knowledge about the whole architecture to fix the problem, i would be happy to peer-code the fix or see a proposal for this issue.

@MikaelVallenet MikaelVallenet marked this pull request as draft August 9, 2024 15:33
Comment on lines +243 to +244
case *CallExpr:
checkValConstValue(x.Func)
Copy link
Contributor

@ltzmaxwell ltzmaxwell Aug 14, 2024

Choose a reason for hiding this comment

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

It seems there may be a misunderstanding about the use of nameExpr as the right-hand side (RHS) of a const declaration. For example:

const a = 1
// 'a' qualifies as a ConstExpr
const b = a

In this case, using a as the RHS in the declaration of b is perfectly valid since a is evaluated to be a constant expression.

Regarding the expression uint(8), this is a CallExpr with the function type(TypeType) of uint8. I'm not certain if this directly addresses your question.

However, I recommend we ensure a shared understanding of the related concepts by reviewing the section on constants in the Go language specification: Go Constants.

Tips, you can try to conduct checkValConstValue in trans_leave *ValueDecl to see if it works.

gfanton and others added 9 commits August 17, 2024 09:29
- Revert partially "feat(stdlibs): add math/rand (gnolang#2455)
(f547d7d)
- Update p/demo/rand and rename to p/demo/entropy

---------

Signed-off-by: moul <[email protected]>
Co-authored-by: Marc Vertes <[email protected]>
This PR has different purposes:
* adding Gnofaucet to Goreleaser task (closes gnolang#2557)
* moving Goreleaser from v 1.26 to v 2.1
* reducing the verbosity of tasks by merging the different release types
(latest, nightly, master) into a single goreleaser file
* adding a fallbacked env variable called `TAG_VERSION`, which label the
various docker images
  * adding the `nightly` subtask, which is ignored for latest releases
* skipping publish for master release (NOTE: this part is the one TBC,
because `master` release is using nightly too, but the variable
`tag_name` unfortunately cannot be templated, let's see if it can make
sense or if there is another workaround)

<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
- [x] 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>
Providing a tuned command to trigger goreleaser on merges in master
branch
## Description

This PR fixates the `golangci-lint` CI version to `1.59`, since this is
the version we utilize for `misc/devdeps` that powers the `make lint`
directive.

<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
- [ ] 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>
Introducing templating for gorelease when running in master branch
<!-- please provide a detailed description of the changes made in this
pull request. -->

## Description

This PR adds the r/gnoland/events realm. It handles events gno.land is
attending dynamically. An admin and modetators can add new events,
delete old ones, or make changes to existing ones.

<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
- [x] 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>

---------

Co-authored-by: Manfred Touron <[email protected]>
…#2697)

## Description

This PR updates the test4 item on the home page, and fixes a small
rendering bug in the r/events realm, and adds tests to it.

<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
- [x] 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>
## Description

This PR bumps the `gnolang/faucet` version to `v0.3.2`, which fixes a
bug where the middlewares would block a standard endpoints like
`/health`.

<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
- [ ] 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>
aeddi and others added 4 commits August 17, 2024 09:29
<!-- please provide a detailed description of the changes made in this
pull request. -->

<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
- [ ] Added references to related issues and PRs
- [ ] 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>

Co-authored-by: Guilhem Fanton <[email protected]>
<!-- please provide a detailed description of the changes made in this
pull request. -->
This PR aims to solves gnolang#2494 in order to parse `markdown` files from
packages templates. It currently uses the file extension (from
`window.location`) to detect if the content is a `md` file in a package
template.

_Although keeping the extension in the url is not a good practice for
SEO & UX (because we are using pretty URLs for some other contents and
are not consistent throughout gnoweb), This should do the work for now
as short term solution since we are gonna parse `.md` from `go` soon
with the new gnoweb design/codebase refactor (cc @gfanton)._

It also improves the package file template by removing global JS and
pushing the `source` content into the main rendering flow from
`renderer.js`.
)

Address gnolang#2008.


In this pull request, we're implementing a special handling for type
declarations to check cynic dependency. Due to their unique nature, we
must meticulously search through their fields to identify potential
cyclic reference.

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

- [*] 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](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Co-authored-by: deelawn <[email protected]>
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Aug 17, 2024
@MikaelVallenet
Copy link
Contributor Author

Hello,

To keep up to date with progress, I'm experiencing difficulties managing all cases, since some constant expressions, such as the import of a constant are not evaluated as ConstExpr. This is also the case for the len/cap function on a const and many other expr...

I've tried to add constExpr evaluation to the expressions that were missing, but I'm running into several problems.

I'm going to work sideways on the issue, and if anyone who's interested in the issue decides to work on it, I'll be happy to share my exp on it in more detail. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ 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: Done
Development

Successfully merging this pull request may close these issues.

Constant Declaration with Slice Type Compiles Without Error
9 participants