-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add enclosing scope
parameter to rustc_on_unimplemented
#66651
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM so far! Thanks for the contribution, I'll leave another comment on where to look for the Try
trait.
To add this note to the Lines 8 to 20 in 9420ff4
However, this might not work because the stage0 compiler (typically beta, it's used to build the compiler and standard library with your changes), won't know about Line 14 in 35ef33a
|
2916b92
to
1ae3823
Compare
enclosing scope
parameter to rustc_on_unimplemented
enclosing scope
parameter to rustc_on_unimplemented
add ui test compute enclosing_scope_span on demand add scope test make tidy happy stylistic and typo fixes
1ae3823
to
1d0c015
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I've left a comment that I think we should consider to improve the diagnostic further, but I'm happy with this as-is if we don't want to make that change.
@@ -794,6 +796,19 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { | |||
// If it has a custom `#[rustc_on_unimplemented]` note, let's display it | |||
err.note(s.as_str()); | |||
} | |||
if let Some(ref s) = enclosing_scope { | |||
let enclosing_scope_span = tcx.def_span( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should just highlight the function name (see below) or return type instead of the whole body - often these spans can be really long and I tend to find them hard to read. Thoughts, @rust-lang/wg-diagnostics?
rust/src/libsyntax_pos/source_map.rs
Lines 886 to 893 in ab21557
pub fn generate_fn_name_span(&self, span: Span) -> Option<Span> { | |
let prev_span = self.span_extend_to_prev_str(span, "fn", true); | |
self.span_to_snippet(prev_span).map(|snippet| { | |
let len = snippet.find(|c: char| !c.is_alphanumeric() && c != '_') | |
.expect("no label after fn"); | |
prev_span.with_hi(BytePos(prev_span.lo().0 + len as u32)) | |
}).ok() | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I normally agree, particularly because some IDEs will highlight the entire thing making the error borderline useless, but in cases similar to this, where knowing where the block ends might be useful, I prefer this CLI output. We can of course merge as is and open a follow up ticket to explore the options around this.
I think this PR might also close #63078. |
Are we waiting on something or this can be merged? |
@bors r+ Oops, sorry @Areredify, had lost track of this. |
📌 Commit 1d0c015 has been approved by |
…=davidtwco Add `enclosing scope` parameter to `rustc_on_unimplemented` Adds a new parameter to `#[rustc_on_unimplemented]`, `enclosing scope`, which highlights the function or closure scope with a message. The wip part refers to adding this annotation to `Try` trait to improve ergonomics (which I don't know how to do since I change both std and librustc) Closes rust-lang#61709.
…=davidtwco Add `enclosing scope` parameter to `rustc_on_unimplemented` Adds a new parameter to `#[rustc_on_unimplemented]`, `enclosing scope`, which highlights the function or closure scope with a message. The wip part refers to adding this annotation to `Try` trait to improve ergonomics (which I don't know how to do since I change both std and librustc) Closes rust-lang#61709.
Rollup of 6 pull requests Successful merges: - #66148 (Show the sign for signed ops on `exact_div`) - #66651 (Add `enclosing scope` parameter to `rustc_on_unimplemented`) - #66904 (Adding docs for keyword match, move) - #66935 (syntax: Unify macro and attribute arguments in AST) - #66941 (Remove `ord` lang item) - #66967 (Remove hack for top-level or-patterns in match checking) Failed merges: r? @ghost
Adds a new parameter to
#[rustc_on_unimplemented]
,enclosing scope
, which highlights the function or closure scope with a message.The wip part refers to adding this annotation to
Try
trait to improve ergonomics (which I don't know how to do since I change both std and librustc)Closes #61709.