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(gnolang): fix an indexExpr preprocess bug #514

Merged
merged 2 commits into from
Feb 22, 2023

Conversation

ltzmaxwell
Copy link
Contributor

Description

type assertion to *MapType may fail while preprocessing *IndexExpr, it may cause a v, ok := Values[k] operation fail especially when it's declared like type Values map[string][]string, detailed in map28c.gno test.

How has this been tested?

passed.

@ltzmaxwell ltzmaxwell requested a review from a team as a code owner February 13, 2023 11:02
@zivkovicmilos
Copy link
Member

Hey @ltzmaxwell,

Thank you for the contribution 🙏

Can you please give me an example of when this *MapType assertion might fail?

@moul moul changed the title Fix: fix an indexExpr preprocess bug fix(gnolang): fix an indexExpr preprocess bug Feb 18, 2023
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.

rest looks good to me

(as an aside, I don't understand why we have files almost exactly matching files2 if the tests are executed in the same way, but that belongs to another issue)

tests/files/map28c.gno Show resolved Hide resolved
@moul
Copy link
Member

moul commented Feb 22, 2023

rest looks good to me

(as an aside, I don't understand why we have files almost exactly matching files2 if the tests are executed in the same way, but that belongs to another issue)

Let me add info about this point in the files/README.md file.


Edit: I just checked and the information is already there.

The difference is:

  • files uses ImportModeNativePreferred
  • files2 uses ImportModeStdlibsPreferred

See: https://github.com/gnolang/gno/blob/master/tests/imports.go#L61-L63

By default, we should try to put the tests on both folders.

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Let’s merge, and wait for a make fmt to happen in another PR, or that someone updates the CI to include tests/ files.

@moul moul merged commit 7a73325 into gnolang:master Feb 22, 2023
@thehowl
Copy link
Member

thehowl commented Feb 22, 2023

The difference is:

files uses ImportModeNativePreferred
files2 uses ImportModeStdlibsPreferred

Missed that. Still think we could make this better, but will make a proposal in a pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants