-
Notifications
You must be signed in to change notification settings - Fork 69
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
--hint-msrv=version
option so the compiler can take MSRV into account when linting
#772
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed. Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:
Concerns can be lifted with:
See documentation at https://forge.rust-lang.org cc @rust-lang/compiler @rust-lang/compiler-contributors |
-C msrv=version
option so the compiler can take MSRV into account when linting--msrv=version
option so the compiler can take MSRV into account when linting
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
cc @rust-lang/lang |
@rfcbot fcp merge |
Team member @estebank has proposed to merge this. The next step is review by the rest of the tagged team members:
Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
I feel like this should be FCP'd when it's ready for stabilization. This simply needs a second, I believe? Or at least that's what other unstable flags have done in the recent past. That being said, I'm somewhat hesitant to add a flag that does nothing right now. It feel like this design should be fleshed out more before, ideally as an RFC, but at least via some sort of design doc that people can look towards to guide implementation moving forward. I'll say a bit more in zulip, but this is somewhat process related so it's worth putting here. |
@rfcbot concern design-needs-fleshing-out |
Speaking of process, since lang approves new lints and how they are gated, it seems that it might be helpful to T-compiler to know whether and how lang might intend to use this gating. I've opened and nominated an issue for us to discuss that: |
--msrv=version
option so the compiler can take MSRV into account when linting--hint-msrv=version
option so the compiler can take MSRV into account when linting
We decided in the lang team meeting that this should:
For now, let's not discuss more radical extensions of this feature like affecting inference, trait solving, or having any sort of guarantee for correctness. @rfcbot resolve design-needs-fleshing-out |
That being said, this doesn't need to be fcp'd until stabilization. It should be gated behind @rfcbot cancel |
@compiler-errors proposal cancelled. |
That being said ×2, I'm happy with the flag in this form. Though other compiler (+contributor) team members can and should speak up and raise a concern if they disagree. @rustbot second |
@tmandry made the point of wanting to have a separate flag for if we decide to use this as an alternative for things like the split of I'll leave it to @tmandry to elaborate on that use case. This isn't a gate/blocker for adding this. |
@joshtriplett I responded in Zulip to keep technical discussion off this thread. |
Clippy has lints that refer to features in the language and library that may not exist in the minimum version a given project supports. Thus it offers the `msrv` field to avoid that. For instance, Clippy should not suggest using `let ... else` in a lint if the MSRV did not implement that syntax. Currently, there should be no effect setting it, since there is no lint that we currently enable that is affected by `msrv` [1] lower than our minimum supported Rust version (1.78.0). Rust is also considering adding a similar feature in `rustc` too, which we should probably enable if it becomes available [2]. Nevertheless, enable it. Link: https://github.com/rust-lang/rust-clippy/blob/8dd459d4c750dbf200bbd48a10f129b1401e7bf3/clippy_config/src/msrvs.rs#L19 [1] Link: rust-lang/compiler-team#772 [2] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Clippy has lints that refer to features in the language and library that may not exist in the minimum version a given project supports. Thus it offers the `msrv` field to avoid that. For instance, Clippy should not suggest using `let ... else` in a lint if the MSRV did not implement that syntax. Currently, there should be no effect setting it, since there is no lint that we currently enable that is affected by `msrv` [1] lower than our minimum supported Rust version (1.78.0). Rust is also considering adding a similar feature in `rustc` too, which we should probably enable if it becomes available [2]. Nevertheless, enable it. Link: https://github.com/rust-lang/rust-clippy/blob/8dd459d4c750dbf200bbd48a10f129b1401e7bf3/clippy_config/src/msrvs.rs#L19 [1] Link: rust-lang/compiler-team#772 [2] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@rustbot label -final-comment-period +major-change-accepted |
Clippy's lints may avoid emitting a suggestion to use a language or library feature that is not supported by the minimum supported version, if given by the `msrv` field in the configuration file. For instance, Clippy should not suggest using `let ... else` in a lint if the MSRV did not implement that syntax. If the MSRV is not provided, Clippy will assume all features are available. Thus enable it with the minimum Rust version the kernel supports. Note that there is currently a small disadvantage in doing so: since we still use unstable features that nevertheless work in the range of versions we support (e.g. `#[expect(...)]`), we lose suggestions for those. However, over time we will stop using unstable features (especially language and library ones) as it is our goal, thus, in the end, we will want to have the `msrv` set. Rust is also considering adding a similar feature in `rustc` too, which we should probably enable if it becomes available [2]. Link: https://github.com/rust-lang/rust-clippy/blob/8298da72e7b81fa30c515631b40fc4c0845948cb/clippy_utils/src/msrvs.rs#L20 [1] Link: rust-lang/compiler-team#772 [2] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Proposal
Add a
--hint-msrv=version
command-line option to rustc, which cargo would then pass in based on therust-version
option inCargo.toml
.The rationale for this is the same reason clippy wants to know rust-version: it allows smarter linting and suggestions. For instance, we could avoid making fix suggestions that would raise MSRV, or alternatively we could emit some metadata saying we'd increase MSRV so that
cargo fix
could automatically increase MSRV.As an additional rationale, when we have a
cfg
for the compiler version, having the MSRV would let us determine if thecfg
is always true or always false: rust-lang/rust#64796 (comment)I'm proposing that the initial implementation of this just validate that its argument is a valid version number (accepting exactly what
rust-version
accepts), and otherwise ignore it; this would be enough for cargo to start passing it to compilers that support it. Any additional functionality would come later.The name
--hint-msrv
helps make it clear that this isn't intended to be load-bearing, and is best-effort.(Until this is stable, it'll require
-Zunstable-options
.)Mentors or Reviewers
I don't have a specific mentor/reviewer in mind here.
Process
The main points of the Major Change Process are as follows:
@rustbot second
.-C flag
, then full team check-off is required.@rfcbot fcp merge
on either the MCP or the PR.You can read more about Major Change Proposals on forge.
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
The text was updated successfully, but these errors were encountered: