typeck: fix ? suggestion span
#123654
Merged
typeck: fix ? suggestion span
#123654
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
|
r? @Nadrieril rustbot has assigned @Nadrieril. Use |
rustbot
added
S-waiting-on-review
| /// syntax context of a potential parent callsite changes, such as if the potential parent callsite | ||
| /// is in a foreign macro. This helps to prevent leaking implementation details from upstream | ||
| /// crates and stdlib crates that the user likely have no control over. | ||
| fn find_local_most_ancestor_suggestable_span(initial_span: Span) -> Span { |
There was a problem hiding this comment.
Why is this different than Span::find_ancestor_in_same_ctxt?
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh, I see. We find the last ancestor that shares the ctxt, not the first. Huh.
There was a problem hiding this comment.
Yeah, it was to get better error reporting for when there are multiple local nested macros. TBH, I'm still not completely sure if this logic is robust for all the weird cases macros have...
There was a problem hiding this comment.
This method should go in rustc_span, as a method on Span.
There was a problem hiding this comment.
Do you have a better name for this? find_local_most_ancestor_suggestable_span is what I came up with but it feels not very accurate
There was a problem hiding this comment.
How about find_oldest_ancestor_in_same_ctxt?
There was a problem hiding this comment.
Sounds better than what I had 😆
This comment has been minimized.
This comment has been minimized.
rust-cloud-vms
bot
force-pushed
the
question-mark-span
branch
from
ff0eb2d
to
98e9201
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot author |
rustbot
added
S-waiting-on-author
rust-cloud-vms
bot
force-pushed
the
question-mark-span
branch
from
98e9201
to
e628dfb
Compare
|
(Ignored the doctest because it contains type errors for illustrative purposes) |
rustbot
added
S-waiting-on-review
This can cause rustfix to crash because the `?` suggestion previously could point into non-local spans, such as into the stdlib.
This test contains conflicting MaybeIncorrect suggestions which will cause the fixed file to not compile.
rust-cloud-vms
bot
force-pushed
the
question-mark-span
branch
from
e628dfb
to
420e3f1
Compare
|
Changed the method name to |
|
Looks good! @bors r+ |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 12, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#123654 (typeck: fix `?` suggestion span) - rust-lang#123807 (Remove `sys_common::thread`) - rust-lang#123834 (Don't do coroutine-closure-specific upvar analysis if tainted by errors) - rust-lang#123847 (Suppress `let else` suggestion for uninitialized refutable `let`s) - rust-lang#123857 (std::net: TcpListener shrinks the backlog argument to 32 for Haiku.) - rust-lang#123858 (zkvm: fix path to cmath in zkvm module) - rust-lang#123867 (Add `unsafe` to two functions with safety invariants) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 12, 2024
Rollup merge of rust-lang#123654 - jieyouxu:question-mark-span, r=Nadrieril typeck: fix `?` suggestion span Noticed in <rust-lang#112043 (comment)>, if the ``` use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller ``` suggestion is applied to a macro that comes from a non-local crate (e.g. the stdlib), the suggestion span can become non-local, which will cause newer rustfix versions to fail. This PR tries to remedy the problem by recursively probing ancestors of the expression span, trying to identify the most ancestor span that is (1) still local, and (2) still shares the same syntax context as the expression. This is the same strategy used in rust-lang#112043. The test unfortunately cannot `//@ run-rustfix` because there are two conflicting MaybeIncorrect suggestions that when collectively applied, cause the fixed source file to become non-compilable. Also avoid running `//@ run-rustfix` for `tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rs` because that also contains conflicting suggestions. cc `@ehuss` who noticed this. This question mark span fix + not running rustfix on the tests containing conflicting MaybeIncorrect suggestions should hopefully unblock rustfix from updating.
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.
Noticed in #112043 (comment), if the
suggestion is applied to a macro that comes from a non-local crate (e.g. the stdlib), the suggestion span can become non-local, which will cause newer rustfix versions to fail.
This PR tries to remedy the problem by recursively probing ancestors of the expression span, trying to identify the most ancestor span that is (1) still local, and (2) still shares the same syntax context as the expression.
This is the same strategy used in #112043.
The test unfortunately cannot
//@ run-rustfixbecause there are two conflicting MaybeIncorrect suggestions that when collectively applied, cause the fixed source file to become non-compilable.Also avoid running
//@ run-rustfixfortests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rsbecause that also contains conflicting suggestions.cc @ehuss who noticed this. This question mark span fix + not running rustfix on the tests containing conflicting MaybeIncorrect suggestions should hopefully unblock rustfix from updating.