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

respect CARGO_PKG_RUST_VERSION when issuing deprecation warnings #98893

Closed

Conversation

hellow554
Copy link
Contributor

you can specify the rust-version0 in Cargo.toml file.
If you specify that, we should not emit a deprecated warning.
Reason for this is, that code will be sprinkled with
#[allow(deprecated)] and maybe be forgotten in the future, so
deprecated items will be used.
If you specify the rust-version field, no such deprecation warning
should be emitted, unless you upgrade your msrv.


This might not be the perfect solution, yet, but I'd like to have this in rust because of the reasons metioned above. Any feedback is welcome.

you can specify the `rust-version`[0] in `Cargo.toml` file.
If you specify that, we should not emit a deprecated warning.
Reason for this is, that code will be sprinkled with
`#[allow(deprecated)]` and maybe be forgotten in the future, so
deprecated items will be used.
If you specify the `rust-version` field, no such deprecation warning
should be emitted, unless you upgrade your msrv.

[0]: https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 4, 2022
@rust-highfive
Copy link
Collaborator

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2022
@nagisa
Copy link
Member

nagisa commented Jul 4, 2022

I don’t think rustc knowing anything about cargo specific environment variables is a right approach. I can imagine a -Z flag that does something similar, though.

@nagisa nagisa added the T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. label Jul 4, 2022
@nagisa
Copy link
Member

nagisa commented Jul 4, 2022

I’m adding T-cargo label here for it really mostly a question of what the semantics of rust-version ought to be in Cargo.

@hellow554
Copy link
Contributor Author

Sorry for not providing enough information here.

It all started with clippy, where I submitted this PR: rust-lang/rust-clippy#8774

During the implementation it was decided, that a new environment variable should be added: CARGO_PKG_RUST_VERSION: rust-lang/cargo#10713
That variable contains the version of the rust-version field in the Cargo.toml file.

Everything was accepted and finally merged into clippy, so that instead of using clippy.tomls msrv, you can use rust-version as a unified way to specify your msrv.

My logical next step was to use that rust-version field to suppress deprecated warnings.

Yes, that variable only set by cargo and not rustc itself, but that's why the code makes no assumption that it has to be there, but evaulates it, if it's present.

@nagisa
Copy link
Member

nagisa commented Jul 5, 2022

You probably misunderstood my point.

What I was trying to say is that rustc should not inspect any CARGO_ environment variables at all. rustc ought to provide an interface independent of the build system where possible (which it almost always is) and the responsibility should be with cargo to set-up rustc in whatever way necessary to achieve the desired handling of rust-version or any other manifest setting.

It is less important for clippy to maintain such a separation of interfaces, largely because it hasn’t a stability contract as strict as rustc’s to adhere to.

@nagisa
Copy link
Member

nagisa commented Jul 5, 2022

In terms of stabilized interfaces, I wonder if -A deprecated>=1.28 -W deprecated>=1.32 -D deprecated>=1.52-like interface wouldn’t be the most flexible and ideal solution. That way besides allowing this use-case, crate implementations would have an option to transition gracefully too… something along those lines, anyway.

@joshtriplett
Copy link
Member

Yeah, rust-version is the minimum Rust version required; that doesn't necessarily mean you want to ignore all deprecation warnings that may be unrelated to supporting that version.

@nagisa nagisa added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 23, 2022
@nagisa
Copy link
Member

nagisa commented Sep 3, 2022

Seeing as the PR has stalled and given the concerns raised above, I’m inclined to close this for now, if only to clean up my review queue. I would strongly advise some sort of a MCP or similar discussion medium before further development work is undertaken.

@nagisa nagisa closed this Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants