Skip to content
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

`#[deprecated]` does nothing if placed on an impl item. #39935

Open
sgrif opened this Issue Feb 18, 2017 · 5 comments

Comments

Projects
None yet
5 participants
@sgrif
Copy link
Contributor

sgrif commented Feb 18, 2017

Given the following two crates:

#![crate_name = "foo"]

pub struct Foo;
pub struct Bar;

pub trait IntoBar {
    fn into_bar(self) -> Bar;
}

#[deprecated(note = "Just pass `Bar` instead")]
impl IntoBar for Foo {
    fn into_bar(self) -> Bar {
        Bar
    }
}
extern crate foo;

use foo::*;

fn main() {
    let _ = Foo.into_bar();
}

I would expect the second crate to see a deprecation warning on the use of Foo.into_bar(). Instead, both crates compile successfully with no errors. I think that allowing deprecations on impls is a useful option to provide to authors (one that I was looking to do, and found this while seeing if it would work). However, if we do not wish to provide that ability to library authors, placing the #[deprecated] attribute on an impl should result in a compiler error.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 21, 2017

This is unfortunately a limitation of stability today, it just doesn't work at all with trait impls. (we run into this all the time in libstd as well)

quodlibetor added a commit to quodlibetor/rust-chrono that referenced this issue Jul 9, 2017

Deprecated warnings in cargo output for rustc-serialize feature
Unfortunately due to rust-lang/rust#39935 placing the annotation on the `impl`s
of `Encodable`/`Decodable` for the various items have no effect whatsoever, so
we need to place it on some type that chrono actually uses internally. The only
*type* that I can find that only exists for rustc-serialize only is the
 `TsSeconds` struct.

So, marking TsSeconds deprecated causes Chrono's internal uses of `TsSeconds`
to emit deprecation warnings, both in our builds and for packages that specify
Chrono as a dependency with the `rustc-serialize` feature active. This means
that the current commit will cause a `warning: use of deprecated item:
RustcSerialize will be removed before chrono 1.0, use Serde instead` to appear
in `cargo build` output.

Unfortunately I don't think that it's possible for downstream crates to disable
the warning the warning in any way other than actually switching to Serde or
using an older chrono. That's the reason for all the `#[allow(deprecated)]`
through the code, it means that the warning appears almost exactly once,
instead of dozens of times.

quodlibetor added a commit to quodlibetor/rust-chrono that referenced this issue Mar 28, 2018

Deprecated warnings in cargo output for rustc-serialize feature
Unfortunately due to rust-lang/rust#39935 placing the annotation on the `impl`s
of `Encodable`/`Decodable` for the various items have no effect whatsoever, so
we need to place it on some type that chrono actually uses internally. The only
*type* that I can find that only exists for rustc-serialize only is the
 `TsSeconds` struct.

So, marking TsSeconds deprecated causes Chrono's internal uses of `TsSeconds`
to emit deprecation warnings, both in our builds and for packages that specify
Chrono as a dependency with the `rustc-serialize` feature active. This means
that the current commit will cause a `warning: use of deprecated item:
RustcSerialize will be removed before chrono 1.0, use Serde instead` to appear
in `cargo build` output.

Unfortunately I don't think that it's possible for downstream crates to disable
the warning the warning in any way other than actually switching to Serde or
using an older chrono. That's the reason for all the `#[allow(deprecated)]`
through the code, it means that the warning appears almost exactly once,
instead of dozens of times.

quodlibetor added a commit to quodlibetor/rust-chrono that referenced this issue Mar 28, 2018

Deprecated warnings in cargo output for rustc-serialize feature
Unfortunately due to rust-lang/rust#39935 placing the annotation on the `impl`s
of `Encodable`/`Decodable` for the various items have no effect whatsoever, so
we need to place it on some type that chrono actually uses internally. The only
*type* that I can find that only exists for rustc-serialize only is the
 `TsSeconds` struct.

So, marking TsSeconds deprecated causes Chrono's internal uses of `TsSeconds`
to emit deprecation warnings, both in our builds and for packages that specify
Chrono as a dependency with the `rustc-serialize` feature active. This means
that the current commit will cause a `warning: use of deprecated item:
RustcSerialize will be removed before chrono 1.0, use Serde instead` to appear
in `cargo build` output.

Unfortunately I don't think that it's possible for downstream crates to disable
the warning the warning in any way other than actually switching to Serde or
using an older chrono. That's the reason for all the `#[allow(deprecated)]`
through the code, it means that the warning appears almost exactly once,
instead of dozens of times.

quodlibetor added a commit to quodlibetor/rust-chrono that referenced this issue Apr 1, 2018

Deprecated warnings in cargo output for rustc-serialize feature
Unfortunately due to rust-lang/rust#39935 placing the annotation on the `impl`s
of `Encodable`/`Decodable` for the various items have no effect whatsoever, so
we need to place it on some type that chrono actually uses internally. The only
*type* that I can find that only exists for rustc-serialize only is the
 `TsSeconds` struct.

So, marking TsSeconds deprecated causes Chrono's internal uses of `TsSeconds`
to emit deprecation warnings, both in our builds and for packages that specify
Chrono as a dependency with the `rustc-serialize` feature active. This means
that the current commit will cause a `warning: use of deprecated item:
RustcSerialize will be removed before chrono 1.0, use Serde instead` to appear
in `cargo build` output.

Unfortunately I don't think that it's possible for downstream crates to disable
the warning the warning in any way other than actually switching to Serde or
using an older chrono. That's the reason for all the `#[allow(deprecated)]`
through the code, it means that the warning appears almost exactly once,
instead of dozens of times.
@abonander

This comment has been minimized.

Copy link
Contributor

abonander commented Sep 1, 2018

@alexcrichton Do we want to support stability on trait impls or not? I remember this being discussed elsewhere but I don't remember the conclusion.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 3, 2018

It'd be nice yeah! At this point everyone just assumes it doesn't work, so there's likely lots of misannotated impls or unintended fallout from a "bugfix" change like this, but it'd be good to implement if possible!

@sgrif

This comment has been minimized.

Copy link
Contributor Author

sgrif commented Sep 10, 2018

I still think we should at least make this a compiler error until we can make it work. I disagree with the "everyone assumes it doesn't work" take, I think folks do assume it works, and never verify if their users are receiving a warning or not.

@mzabaluev

This comment has been minimized.

Copy link
Contributor

mzabaluev commented Nov 26, 2018

I still think we should at least make this a compiler error until we can make it work.

A warning would be a gentler option. I agree that it's not good to let people put the deprecation attribute on and assume it will work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.