Skip to content

/wasm-bindgen

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

Improve #[wasm_bindgen] errors with `compile_error!` #601

Closed
alexcrichton opened this Issue Aug 1, 2018 · 3 comments

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@alexcrichton @mkane0814
@alexcrichton
Copy link
Collaborator

alexcrichton commented Aug 1, 2018

Right now the procedural macro simply panics if anything goes wrong, but this is pretty bad! The error messages coming out look pretty opaque at times and are often quite subpar, especially in that they're pointing to the wrong items.

I've just realized, however, that the procedural macro can generate compile_error! invocations which gives us the ability to generate custom compiler errors as well as custom error spans. An example of this is:

https://gist.github.com/alexcrichton/35f9b54585b39d16a887cffd4eabdb2d

We should refactor the macro crate to use Result for error handling and use a structured error type which has a string and start/end spans, and that way we can generate nicely spanned error messages, yay!
@mkane0814

This comment has been minimized.

Sign in to view
Copy link

mkane0814 commented Aug 1, 2018

Yes please! These errors have been really frustrating to work around. Does anyone know what causes them?

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Aug 1, 2018

@alexcrichton
Prepare to have targeted error diagnostics 
This commit starts to add infrastructure for targeted diagnostics in the
`#[wasm_bindgen]` attribute, intended eventually at providing much better errors
as they'll be pointing to exactly the code in question rather than always to a
`#[wasm_bindgen]` attribute.

The general changes are are:

* A new `Diagnostic` error type is added to the backend. A `Diagnostic` is
  created with a textual error or with a span, and it can also be created from a
  list of diagnostics. A `Diagnostic` implements `ToTokens` which emits a bunch
  of invocations of `compile_error!` that will cause rustc to later generate
  errors.

* Fallible implementations of `ToTokens` have switched to using a new trait,
  `TryToTokens`, which returns a `Result` to use `?` with.

* The `MacroParse` trait has changed to returning a `Result` to propagate errors
  upwards.

* A new `ui-tests` crate was added which uses `compiletest_rs` to add UI tests.
  These UI tests will verify that our output improves over time and does not
  regress. This test suite is added to CI as a new builder as well.

* No `Diagnostic` instances are created just yet, everything continues to panic
  and return `Ok`, with the one exception of the top-level invocations of
  `syn::parse` which now create a `Diagnostic` and pass it along.

This commit does not immediately improve diagnostics but the intention is that
it is laying the groundwork for improving diagnostics over time. It should
ideally be much easier to contribute improved diagnostics after this commit!

cc rustwasm#601
0841a7b

@alexcrichton alexcrichton referenced this issue Aug 1, 2018

Merged

Prepare to have targeted error diagnostics #604

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

alexcrichton commented Aug 1, 2018

@mkane0814 the errors here are an unfortunate artifact of how procedural macros work today with one of the primary vectors of generating errors being panic!, which then poorly uses spans to attribute the wrong code.

Put another way, "what causes the bad errors messages?" can be answered with "the entire macro today", which is bad!

I've started to lay the groundwork for this in #604

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Aug 1, 2018

@alexcrichton
Prepare to have targeted error diagnostics 
This commit starts to add infrastructure for targeted diagnostics in the
`#[wasm_bindgen]` attribute, intended eventually at providing much better errors
as they'll be pointing to exactly the code in question rather than always to a
`#[wasm_bindgen]` attribute.

The general changes are are:

* A new `Diagnostic` error type is added to the backend. A `Diagnostic` is
  created with a textual error or with a span, and it can also be created from a
  list of diagnostics. A `Diagnostic` implements `ToTokens` which emits a bunch
  of invocations of `compile_error!` that will cause rustc to later generate
  errors.

* Fallible implementations of `ToTokens` have switched to using a new trait,
  `TryToTokens`, which returns a `Result` to use `?` with.

* The `MacroParse` trait has changed to returning a `Result` to propagate errors
  upwards.

* A new `ui-tests` crate was added which uses `compiletest_rs` to add UI tests.
  These UI tests will verify that our output improves over time and does not
  regress. This test suite is added to CI as a new builder as well.

* No `Diagnostic` instances are created just yet, everything continues to panic
  and return `Ok`, with the one exception of the top-level invocations of
  `syn::parse` which now create a `Diagnostic` and pass it along.

This commit does not immediately improve diagnostics but the intention is that
it is laying the groundwork for improving diagnostics over time. It should
ideally be much easier to contribute improved diagnostics after this commit!

cc rustwasm#601
9a8e620

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Aug 1, 2018

@alexcrichton
Prepare to have targeted error diagnostics 
This commit starts to add infrastructure for targeted diagnostics in the
`#[wasm_bindgen]` attribute, intended eventually at providing much better errors
as they'll be pointing to exactly the code in question rather than always to a
`#[wasm_bindgen]` attribute.

The general changes are are:

* A new `Diagnostic` error type is added to the backend. A `Diagnostic` is
  created with a textual error or with a span, and it can also be created from a
  list of diagnostics. A `Diagnostic` implements `ToTokens` which emits a bunch
  of invocations of `compile_error!` that will cause rustc to later generate
  errors.

* Fallible implementations of `ToTokens` have switched to using a new trait,
  `TryToTokens`, which returns a `Result` to use `?` with.

* The `MacroParse` trait has changed to returning a `Result` to propagate errors
  upwards.

* A new `ui-tests` crate was added which uses `compiletest_rs` to add UI tests.
  These UI tests will verify that our output improves over time and does not
  regress. This test suite is added to CI as a new builder as well.

* No `Diagnostic` instances are created just yet, everything continues to panic
  and return `Ok`, with the one exception of the top-level invocations of
  `syn::parse` which now create a `Diagnostic` and pass it along.

This commit does not immediately improve diagnostics but the intention is that
it is laying the groundwork for improving diagnostics over time. It should
ideally be much easier to contribute improved diagnostics after this commit!

cc rustwasm#601
6d42ad6

alexcrichton added a commit that referenced this issue Aug 1, 2018

@alexcrichton
Prepare to have targeted error diagnostics (#604) 
This commit starts to add infrastructure for targeted diagnostics in the
`#[wasm_bindgen]` attribute, intended eventually at providing much better errors
as they'll be pointing to exactly the code in question rather than always to a
`#[wasm_bindgen]` attribute.

The general changes are are:

* A new `Diagnostic` error type is added to the backend. A `Diagnostic` is
  created with a textual error or with a span, and it can also be created from a
  list of diagnostics. A `Diagnostic` implements `ToTokens` which emits a bunch
  of invocations of `compile_error!` that will cause rustc to later generate
  errors.

* Fallible implementations of `ToTokens` have switched to using a new trait,
  `TryToTokens`, which returns a `Result` to use `?` with.

* The `MacroParse` trait has changed to returning a `Result` to propagate errors
  upwards.

* A new `ui-tests` crate was added which uses `compiletest_rs` to add UI tests.
  These UI tests will verify that our output improves over time and does not
  regress. This test suite is added to CI as a new builder as well.

* No `Diagnostic` instances are created just yet, everything continues to panic
  and return `Ok`, with the one exception of the top-level invocations of
  `syn::parse` which now create a `Diagnostic` and pass it along.

This commit does not immediately improve diagnostics but the intention is that
it is laying the groundwork for improving diagnostics over time. It should
ideally be much easier to contribute improved diagnostics after this commit!

cc #601
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
c4dcaee

@alexcrichton alexcrichton referenced this issue Aug 1, 2018

Merged

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

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

alexcrichton commented Aug 3, 2018

I think this is effectively done now, yay!

@alexcrichton alexcrichton closed this Aug 3, 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.