Skip to content

rustc: add item name to deprecated lint warning #45707

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 11, 2017
Merged

rustc: add item name to deprecated lint warning #45707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/librustc/middle/stability.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 path = self.item_path_str(def_id);

let msg = if let Some(note) = note {
format!("use of deprecated item: {}", note)
format!("use of deprecated item '{}': {}", path, note)
} else {
format!("use of deprecated item")
format!("use of deprecated item '{}'", path)
};

self.lint_node(lint::builtin::DEPRECATED, id, span, &msg);
Expand All @@ -538,7 +540,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
};

if !skip {
lint_deprecated(depr_entry.attr.note);
lint_deprecated(def_id, depr_entry.attr.note);
}
}

Expand All @@ -557,7 +559,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
if let Some(&Stability{rustc_depr: Some(attr::RustcDeprecation { reason, .. }), ..})
= stability {
if id != ast::DUMMY_NODE_ID {
lint_deprecated(Some(reason));
lint_deprecated(def_id, Some(reason));
}
}

Expand Down
20 changes: 17 additions & 3 deletions src/librustc/ty/item_path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,23 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}
}

cur_path.push(self.def_key(cur_def)
.disambiguated_data.data.get_opt_name().unwrap_or_else(||
Symbol::intern("<unnamed>").as_str()));
let mut cur_def_key = self.def_key(cur_def);

// For a UnitStruct or TupleStruct we want the name of its parent rather than <unnamed>.
if let DefPathData::StructCtor = cur_def_key.disambiguated_data.data {
let parent = DefId {
krate: cur_def.krate,
index: cur_def_key.parent.expect("DefPathData::StructCtor missing a parent"),
};

cur_def_key = self.def_key(parent);
}

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);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@Ryman Ryman Nov 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, but my premise is totally false. We invoke try_push_visible_item_path from the normal item_path.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, I might rather do this "skip to parent of struct ctor" in the caller.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


match visible_parent_map.get(&cur_def) {
Some(&def) => cur_def = def,
None => return false,
Expand Down
18 changes: 18 additions & 0 deletions src/test/compile-fail/auxiliary/deprecation-lint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,24 @@ pub enum Enum {
#[deprecated(since = "1.0.0", note = "text")]
pub struct DeprecatedTupleStruct(pub isize);

pub mod nested {
#[deprecated(since = "1.0.0", note = "text")]
pub struct DeprecatedStruct {
pub i: isize
}

#[deprecated(since = "1.0.0", note = "text")]
pub struct DeprecatedUnitStruct;

pub enum Enum {
#[deprecated(since = "1.0.0", note = "text")]
DeprecatedVariant,
}

#[deprecated(since = "1.0.0", note = "text")]
pub struct DeprecatedTupleStruct(pub isize);
}

pub struct Stable {
#[deprecated(since = "1.0.0", note = "text")]
pub override2: u8,
Expand Down
Loading