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

Result-ify `src/parser.rs` #608

Merged
merged 3 commits into from Aug 2, 2018

Conversation

Projects
None yet
2 participants
@alexcrichton
Copy link
Collaborator

alexcrichton commented Aug 1, 2018

This commit converts all of src/parser.rs away from panics to using
Diagnostic instead. Along the way this adds a test case per changed panic!,
ensuring that we don't regress in these areas!

cc #601

alexcrichton added some commits Aug 1, 2018

Make ConvertToAst trait fallible
It's got some panics, and we'll be switching those to errors!
First example of a diagnostic-driven error
Add a diagnostic-driven error `#[wasm_bindgen]` being attached to public
functions, and add some macros to boot to make it easier to generate errors!
@fitzgen

fitzgen approved these changes Aug 1, 2018

Copy link
Member

fitzgen left a comment

How does one run the UI tests? Is it the same as all of our other tests? If not, we should also document that.

Also, are rustc error messages stable enough that we can rely on this stderr content remaining the same?

Overall, this is a huge improvement for the UX of using wasm-bindgen! Thanks, Alex!

@alexcrichton

This comment has been minimized.

Copy link
Collaborator Author

alexcrichton commented Aug 1, 2018

How does one run the UI tests?

We've got some handy documentation about that!

Is it the same as all of our other tests?

Indeed! A simple cargo test -p ui-tests should do the trick.

Also, are rustc error messages stable enough that we can rely on this stderr content remaining the same?

I believe that since all this output is our error messages it should hopefully be pretty stable. That being said I wouldn't be surprised if these tests break over time. I'd like to eventually add UI tests as well for failing conditions like "you specified an argument that didn't implement IntoWasmAbi" to make sure it looks reasonable, but that sort of error message is practically guaranteed to change over time.

I think we may eventually move this test suite into an "allow failures" category on Travis and only update it every now and then (such as when the test suite itself is modified), but for now we hopefully won't have too many issues!

Result-ify `src/parser.rs`
This commit converts all of `src/parser.rs` away from panics to using
`Diagnostic` instead. Along the way this adds a test case per changed `panic!`,
ensuring that we don't regress in these areas!

@alexcrichton alexcrichton force-pushed the alexcrichton:better-errors-for-real branch from 8d6569b to 9137c3f Aug 1, 2018

@alexcrichton alexcrichton merged commit bdec258 into rustwasm:master Aug 2, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@alexcrichton alexcrichton deleted the alexcrichton:better-errors-for-real branch Aug 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.