Don't use unwrap after calling Span.join
#25
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The method `Span.join` will return `None` if the start and end `Span`s are from different files. This is currently difficult to observe in practice due to rust-lang/rust#43081, which causes span information (including file information) to be lost in many cases. However, PR rust-lang/rust#73084 will cause `Spans` to be properly preserved in many more cases. This will cause `rocket` to to stop compiling, as this code will end up getting hit with spans from different files (one from rocket, and one from the `result` ident defined in the `pear_try!` macro. To reproduce the issue: 1. Checkout SergioBenitez/Rocket#63a4ae048540a6232c3c7c186e9d027081940382 2. Install https://github.com/kennytm/rustup-toolchain-install-master if you don't already have it 3. Run `rustup-toolchain-install-master 879b8cb7dc2ad9102994457e73cf78d124926ea5` (this corresponds to the latest commit from rust-lang/rust#73084) 4. From the `Rocket` checkout, run `cargo +879b8cb7dc2ad9102994457e73cf78d124926ea5 build` 5. Observe the panic from `Pear` I've based this PR on the commit for `Pear 0.1.2`, since the master branch has many breaking changes. I would recommend merging this change into a separate branch, and making a new `0.1.3` point release.
|
Thank you for this incredibly thoughtful PR. I sincerely appreciate it. I've merged this in a new |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 24, 2020
…ture, r=petrochenkov Always capture tokens for `macro_rules!` arguments When we invoke a proc-macro, the `TokenStream` we pass to it may contain 'interpolated' AST fragments, represented by `rustc_ast::token::Nonterminal`. In order to correctly, pass a `Nonterminal` to a proc-macro, we need to have 'captured' its `TokenStream` at the time the AST was parsed. Currently, we perform this capturing when attributes are present on items and expressions, since we will end up using a `Nonterminal` to pass the item/expr to any proc-macro attributes it is annotated with. However, `Nonterminal`s are also introduced by the expansion of metavariables in `macro_rules!` macros. Since these metavariables may be passed to proc-macros, we need to have tokens available to avoid the need to pretty-print and reparse (see rust-lang#43081). This PR unconditionally performs token capturing for AST items and expressions that are passed to a `macro_rules!` invocation. We cannot know in advance if captured item/expr will be passed to proc-macro, so this is needed to ensure that tokens will always be available when they are needed. This ensures that proc-macros will receive tokens with proper `Spans` (both location and hygiene) in more cases. Like all work on rust-lang#43081, this will cause regressions in proc-macros that were relying on receiving tokens with dummy spans. In this case, Crater revealed only one regression: the [Pear](https://github.com/SergioBenitez/Pear) crate (a helper for [rocket](https://github.com/SergioBenitez/Rocket)), which was previously [fixed](SergioBenitez/Pear#25) as part of rust-lang#73084. This regression manifests itself as the following error: ``` [INFO] [stdout] error: proc macro panicked [INFO] [stdout] --> /opt/rustwide/cargo-home/registry/src/github.com-1ecc6299db9ec823/rocket_http-0.4.5/src/parse/uri/parser.rs:119:34 [INFO] [stdout] | [INFO] [stdout] 119 | let path_and_query = pear_try!(path_and_query(is_pchar)); [INFO] [stdout] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [INFO] [stdout] | [INFO] [stdout] = help: message: called `Option::unwrap()` on a `None` value [INFO] [stdout] = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) ``` It can be fixed by running `cargo update -p pear`, which updates your `Cargo.lock` to use the latest version of Pear (which includes a bugfix for the regression). Split out from rust-lang#73084
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The method
Span.joinwill returnNoneif the start and endSpansare from different files. This is currently difficult to observe in
practice due to rust-lang/rust#43081, which causes span information
(including file information) to be lost in many cases.
However, PR rust-lang/rust#73084 will cause
Spansto be properlypreserved in many more cases. This will cause
rocketto to stopcompiling, as this code will end up getting hit with spans from
different files (one from rocket, and one from the
resultidentdefined in the
pear_try!macro.To reproduce the issue:
you don't already have it
rustup-toolchain-install-master 879b8cb7dc2ad9102994457e73cf78d124926ea5(this corresponds to the latest commit from Re-land PR #72388: Recursively expand
TokenKind::Interpolatedinprobably_equal_for_proc_macrorust-lang/rust#73084)Rocketcheckout, runcargo +879b8cb7dc2ad9102994457e73cf78d124926ea5 buildPearI've based this PR on the commit for
Pear 0.1.2, since the masterbranch has many breaking changes. I would recommend merging this change
into a separate branch, and making a new
0.1.3point release.