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

Regression in #[allow(deprecated)] for `impl Trait` return type #54045

Closed
cramertj opened this Issue Sep 7, 2018 · 11 comments

Comments

Projects
6 participants
@cramertj
Copy link
Member

cramertj commented Sep 7, 2018

The following code compiles on stable but fails to compile on beta and nightly with the warning "use of deprecated item Deprecated":

#![deny(warnings)]

#[deprecated]
trait Deprecated {}

#[deprecated]
struct DeprecatedTy;

#[allow(deprecated)]
impl Deprecated for DeprecatedTy {}

#[allow(deprecated)]
fn foo() -> impl Deprecated { DeprecatedTy }

fn main() {
    foo();
}

Error:

error: use of deprecated item 'Deprecated'
  --> src/main.rs:13:18
   |
13 | fn foo() -> impl Deprecated { DeprecatedTy }
   |                  ^^^^^^^^^^
   |
note: lint level defined here
  --> src/main.rs:1:9
   |
1  | #![deny(warnings)]
   |         ^^^^^^^^
   = note: #[deny(deprecated)] implied by #[deny(warnings)]

error: aborting due to previous error

Possibly related to @oli-obk 's refactoring of impl Trait?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Sep 8, 2018

Possible. Is the deprecation checker running on HIR? because we now have a HIR existential type item, and I don't think we're copying lint attributes from the function over to it

@pietroalbini pietroalbini added this to Triaged in 1.29 regressions via automation Sep 10, 2018

@pietroalbini pietroalbini added this to the 1.29 milestone Sep 10, 2018

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Sep 10, 2018

cc @Mark-Simulacrum @rust-lang/compiler

Beta promotion to stable should happen today...

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Sep 10, 2018

because we now have a HIR existential type item, and I don't think we're copying lint attributes from the function over to it

Ideally, the HIR would be nested in such a way that the lints implicitly apply to the existential.

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Sep 10, 2018

Bisected this, the cause of the regression is #51806. cc @oli-obk (author) @cramertj (reviewer)

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Sep 10, 2018

Ideally, the HIR would be nested in such a way that the lints implicitly apply to the existential.

for that we'd either need some form of anonymous modules that fill their items into the outer module, or we'd move the existential type item into the function body (which you were explicitly against), or we'd add a hir::TyKind::Item which contains an actual hir::Item

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Sep 10, 2018

or we'd add a hir::TyKind::Item which contains an actual hir::Item

This is the closest to a real fix IMO, the path trick is cool but problematic.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Sep 13, 2018

visited at compiler team meeting. P-high. Assigning to both @eddyb and @oli-obk

@oli-obk : if you're not available , just remove your assignment (but realize you are ceding authority! ha ha ha)

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Sep 20, 2018

Discussed at T-compiler meeting

Some notes:

  • There exists a workaround; namely, one can tag a containing item with the #[allow(..)], as demonstrated here on play.
  • Since there exists a workaround, and the bug has at this point leaked out to stable, it seems unlikely that we would attempt a full backport all the way to stable.
  • And therefore, we are willing to wait for @oli-obk to return in order to work together with @eddyb to identify the best fix for this bug.
  • So, we're keeping this at P-high, but we're not striving to make a PR that would be backport-able to stable nor to beta.)
  • Having said that, I'm leaving the stable-nominated tag on the bug so that @oli-obk and/or @cramertj get a chance to chime in before we remove it.
@cramertj

This comment has been minimized.

Copy link
Member Author

cramertj commented Sep 20, 2018

Seems fine to me!

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Sep 27, 2018

visited for triage. No change in status; plan of record remains as recorded in my previous comment.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Oct 1, 2018

I'm on it

bors added a commit that referenced this issue Oct 5, 2018

Auto merge of #54741 - oli-obk:impl_trait_hierarchy, r=cramertj
Nest the `impl Trait` existential item inside the return type

fixes #54045

r? @cramertj

@bors bors closed this in #54741 Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment