rustc: add item name to deprecated lint warning #45707
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
src/librustc/middle/stability.rs
Outdated
| @@ -516,11 +516,13 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { | |||
| return; | |||
| } | |||
|
|
|||
| let lint_deprecated = |note: Option<Symbol>| { | |||
| let lint_deprecated = |def_id: DefId, note: Option<Symbol>| { | |||
| let name = self.item_path_str(def_id); | |||
There was a problem hiding this comment.
This variable should be named path.
| let data = cur_def_key.disambiguated_data.data; | ||
| let symbol = | ||
| data.get_opt_name().unwrap_or_else(|| Symbol::intern("<unnamed>").as_str()); | ||
| cur_path.push(symbol); |
There was a problem hiding this comment.
@eddyb what specifically are you unsure about? The use of <unnamed>? I wonder if/when this can ever arise. Perhaps some cases like this?
struct Foo;
impl Foo {
#[deprecated]
fn bar(&self); // referenced like `Foo::bar`
}also maybe something like this
fn foo() {
let x = || {
#[deprecated]
fn bar() { }
bar();
};
}It'd definitely be good to include those two cases in the test harness, if nothing else.
There was a problem hiding this comment.
The first should already addressed by MethodTester in deprecation-lint.rs which includes testing all the UFCS forms.
I've added a test for the latter, which prints use of deprecated item 'this_crate::test_fn_closure_body::{{closure}}::bar which seems okay?
There was a problem hiding this comment.
Ah, I see that the <unnamed> only applies at the tip (and that it was here anyway). I guess @eddyb was likely complaining more about the fact that it now skips to the parent of a StructCtor. That made me nervous too, except that it is part of push_visible_item_path, which seems like it's a "user-facing printout" function, and in that case this does strike me as the right behavior.
There was a problem hiding this comment.
Ah, but my premise is totally false. We invoke try_push_visible_item_path from the normal item_path.
There was a problem hiding this comment.
In that case, I might rather do this "skip to parent of struct ctor" in the caller.
There was a problem hiding this comment.
Though really this is more evidence that item_path_str is just serving too many clients right now. Let me go do some ripgrep around -- I may be remembering wrong -- but I feel like it's used in a lot of places, some of which may want precision and some of which do not.
There was a problem hiding this comment.
In that case, I might rather do this "skip to parent of struct ctor" in the caller.
@Ryman do you see what I mean by this? thoughts?
There was a problem hiding this comment.
@nikomatsakis Yeah, I understand, I was waiting for a followup to 'Let me go do some ripgrep around' incase you had other ideas :P I think adding it to the caller could make sense here as we already do so in other cases in the only caller that I can find see here.
There was a problem hiding this comment.
To be honest I found it hard to assess all the places that it's being used. Still, that case you showed seems to suggest that we're already doing a similar amount of suppression.
|
r? @nikomatsakis for the item path changes |
kennytm
added
the
S-waiting-on-review
Ryman
force-pushed
the
deprecated-item-name
branch
from
40f410b to
bdcddd8
Compare
Ryman
force-pushed
the
deprecated-item-name
branch
from
bdcddd8 to
725ddb4
Compare
|
@bors r+ The worrisome case seems like it already exists (other parts of that same code skip over steps) -- clearly we need to rework |
|
📌 Commit 725ddb4 has been approved by |
kennytm
added
S-waiting-on-bors
rustc: add item name to deprecated lint warning It can sometimes be difficult to know what is actually deprecated when you have `foo.bar()` and `bar` comes from a trait in another crate.
|
☀️ Test successful - status-appveyor, status-travis |
It can sometimes be difficult to know what is actually deprecated when you have
foo.bar()andbarcomes from a trait in another crate.