-
Notifications
You must be signed in to change notification settings - Fork 91
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
Refactor validation #392
Refactor validation #392
Conversation
efe0844
to
7593a90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks
packages/validation/src/index.js
Outdated
errors.push(...isConst(ast, moduleContext)); | ||
errors.push(...importOrderValidate(ast)); | ||
errors.push(...typeChecker(ast, moduleContext)); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some inconsistencies in the way the repl
constructs modules. It doesnt always pass a Program
here, but sometimes ast
is directly a Module
node itself. I tried checking for it here, but it didnt fix all the tests, so it probably requires some changes to the repl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the problem. The repl also used the type checker directly. I switched to the new getValidationErrors
API.
aebc4a6
to
d436777
Compare
@xtuc I finally got around fixing this, except for one spec test. But I am not sure how it relates to the current changes in the specification: WebAssembly/spec#814. Do you know when the official testsuite is updated? |
@maurobringolf (sorry for the delai). I tried to upgrade our spec tests here #296, I will try to finish it eventually. The issue with mutable global is that you can't really use them atm, no support but it's coming soon |
Some refactor of validations improving a couple of things:
Mutable globals cannot be used in global initialization Fixes initializer expression cannot reference a mutable global #355.
ModuleContext is created before running validations and passed to the individual steps.
Validation
fixtures
now run all validation steps, not just type-checking and I removed the separateisConst
tests since its API changed toProgram -> Array<string>
giving a list of errors too.We should probably merge all validation visitors and then run one big
traverse
doing all the work at the same time?