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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

*Resolve* spanned errors at call/mixed site #1467

Merged
merged 1 commit into from Jun 8, 2023

Conversation

danielhenrymantilla
Copy link
Contributor

  • This fixes the error message loss (shadowed by an error complaining about ::core not found) for 2015 edition callers when the callee was using Error::new_spanned over the caller's input tokens (since this would then resolve ::core using the caller's edition).

    • an example:

      error[E0433]: failed to resolve: unresolved import
        --> /Users/cmdjojo/.cargo/registry/src/github.com-1ecc6299db9ec823/stdweb-0.4.20/src/webcore/ffi/wasm_bindgen.rs:67:32
         |
      67 |             alloc: &Closure< Fn( usize ) -> *mut u8 >,
         |                                ^ unresolved import

      complains about unresolved import while pointing to an open parenthesis.

  • More generally, it seems more conceptually correct to only be tweaking the located_at aspect of spans when dealing with a compile_error! invocation 馃檪

- This fixes the error message loss (shadowed by an error complaining about `::core` not found) for 2015 edition callers when the callee was using `Error::new_spanned` over the caller's input tokens (since this would then *resolve* `::core` using the caller's edition).

  - an example: rustwasm/wasm-bindgen#3415 (comment) complains about `unresolved import` while pointing to an open parenthesis.


- More generally, it is more correct to only tweak the `located_at` aspect of spans when dealing with a `compile_error!` invocation.
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay dtolnay merged commit 145142a into dtolnay:master Jun 8, 2023
17 checks passed
@dtolnay
Copy link
Owner

dtolnay commented Jun 8, 2023

I am yanking this release because this had an effect on many error messages that I was not expecting and not sure I like.

Rustc assumes that if there is an error inside of code that has a call site span, then that is code that was conjured up by the macro and therefore the macro is the thing at fault, as opposed to whatever the user has written in their macro call.

For example, before:

error: pointer argument requires that the function be marked unsafe
 --> tests/ui/ptr_missing_unsafe.rs:6:27
  |
6 |         fn not_unsafe_ptr(c: *mut C);
  |                           ^^^^^^^^^

After:

error: pointer argument requires that the function be marked unsafe
 --> tests/ui/ptr_missing_unsafe.rs:6:27
  |
1 | #[cxx::bridge]
  | -------------- in this procedural macro expansion
...
6 |         fn not_unsafe_ptr(c: *mut C);
  |                           ^^^^^^^^^
  |
  = note: this error originates in the attribute macro `cxx::bridge` (in Nightly builds, run with -Z macro-backtrace for more info)

That error looks a lot more like the errors you get whenever a macro is buggy and produced invalid code, as opposed to the errors you get when you do something wrong in your own code.

Notice how it is guiding the user toward -Z macro-backtrace (and toward nightly, even if the current compiler is stable) to try to understand where inside the macro implementation the broken code was coming from, which is counterproductive.

$ cargo check -Z macro-backtrace
error: unknown `-Z` flag specified: macro-backtrace

$ RUSTFLAGS='-Z macro-backtrace' cargo check
error: pointer argument requires that the function be marked unsafe
  --> src/main.rs:6:27
   |
1  | #[cxx::bridge]
   | -------------- in this procedural macro expansion
...
6  |         fn not_unsafe_ptr(c: *mut C);
   |                           ^^^^^^^^^
   |
  ::: ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cxxbridge-macro-1.0.95/src/lib.rs:70:1
   |
70 | pub fn bridge(args: TokenStream, input: TokenStream) -> TokenStream {
   | ------------------------------------------------------------------- in this expansion of `#[cxx::bridge]`

error: could not compile `testing` (bin "testing") due to previous 

While there is some benefit to attaching the macro name as part of proc macro errors, I think overall this is worse than before. The following would be acceptable to me:

error: pointer argument requires that the function be marked unsafe
 --> tests/ui/ptr_missing_unsafe.rs:6:27
  |
6 |         fn not_unsafe_ptr(c: *mut C);
  |                           ^^^^^^^^^
  |
  = note: this error found during expansion of the attribute macro `cxx::bridge`

Another effect of this PR is that errors detected by more than one macro are no longer collapsed together, since rustc is trying to communicate that both of these macros are buggy:

error: unknown serde container attribute `asdf`
  --> src/main.rs:10:9
   |
10 | #[serde(asdf)]
   |         ^^^^
error: unknown serde container attribute `asdf`
  --> src/main.rs:10:9
   |
9  | #[derive(Serialize, Deserialize)]
   |          --------- in this derive macro expansion
10 | #[serde(asdf)]
   |         ^^^^
   |
   = note: this error originates in the derive macro `serde::Serialize` (in Nightly builds, run with -Z macro-backtrace for more info)

error: unknown serde container attribute `asdf`
  --> src/main.rs:10:9
   |
9  | #[derive(Serialize, Deserialize)]
   |                     ----------- in this derive macro expansion
10 | #[serde(asdf)]
   |         ^^^^
   |
   = note: this error originates in the derive macro `serde::Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)

@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Jun 8, 2023

You are totally right this should not be released, especially for something as niche as an edition 2015 user, when it hinders diagnostics so much.

Apologies for the regression and for the churn involved (I very much did not anticipate the role resolved_at was gonna play in the location of errors) 馃檱

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

Successfully merging this pull request may close these issues.

None yet

2 participants