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

rustc: add item name to deprecated lint warning #45707

Merged
merged 1 commit into from Nov 11, 2017

Conversation

Projects
None yet
6 participants
@Ryman
Contributor

Ryman commented Nov 2, 2017

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.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 2, 2017

Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Nov 2, 2017

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

Show outdated Hide outdated src/librustc/middle/stability.rs Outdated
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);

This comment has been minimized.

@eddyb

eddyb Nov 2, 2017

Member

I'm not sure about this.

@eddyb

eddyb Nov 2, 2017

Member

I'm not sure about this.

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 2, 2017

Contributor

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

@nikomatsakis

nikomatsakis Nov 2, 2017

Contributor

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

This comment has been minimized.

@Ryman

Ryman Nov 2, 2017

Contributor

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?

@Ryman

Ryman Nov 2, 2017

Contributor

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?

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 3, 2017

Contributor

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.

@nikomatsakis

nikomatsakis Nov 3, 2017

Contributor

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.

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 3, 2017

Contributor

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

@nikomatsakis

nikomatsakis Nov 3, 2017

Contributor

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

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 3, 2017

Contributor

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

@nikomatsakis

nikomatsakis Nov 3, 2017

Contributor

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

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 3, 2017

Contributor

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.

@nikomatsakis

nikomatsakis Nov 3, 2017

Contributor

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.

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 7, 2017

Contributor

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?

@nikomatsakis

nikomatsakis Nov 7, 2017

Contributor

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?

This comment has been minimized.

@Ryman

Ryman Nov 8, 2017

Contributor

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

@Ryman

Ryman Nov 8, 2017

Contributor

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

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 9, 2017

Contributor

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.

@nikomatsakis

nikomatsakis Nov 9, 2017

Contributor

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.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Nov 2, 2017

Member

r? @nikomatsakis for the item path changes

Member

eddyb commented Nov 2, 2017

r? @nikomatsakis for the item path changes

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 10, 2017

Contributor

@bors r+

The worrisome case seems like it already exists (other parts of that same code skip over steps) -- clearly we need to rework item_path_str.

Contributor

nikomatsakis commented Nov 10, 2017

@bors r+

The worrisome case seems like it already exists (other parts of that same code skip over steps) -- clearly we need to rework item_path_str.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 10, 2017

Contributor

📌 Commit 725ddb4 has been approved by nikomatsakis

Contributor

bors commented Nov 10, 2017

📌 Commit 725ddb4 has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 10, 2017

Contributor

⌛️ Testing commit 725ddb4 with merge 25cc4a8...

Contributor

bors commented Nov 10, 2017

⌛️ Testing commit 725ddb4 with merge 25cc4a8...

bors added a commit that referenced this pull request Nov 10, 2017

Auto merge of #45707 - Ryman:deprecated-item-name, r=nikomatsakis
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.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 11, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 25cc4a8 to master...

Contributor

bors commented Nov 11, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 25cc4a8 to master...

@bors bors merged commit 725ddb4 into rust-lang:master Nov 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment