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

--hint-msrv=version option so the compiler can take MSRV into account when linting #772

Closed
1 of 3 tasks
joshtriplett opened this issue Aug 14, 2024 · 18 comments
Closed
1 of 3 tasks
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Aug 14, 2024

Proposal

Add a --hint-msrv=version command-line option to rustc, which cargo would then pass in based on the rust-version option in Cargo.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 the cfg 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:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

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.

@joshtriplett joshtriplett added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Aug 14, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 14, 2024

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:

@rustbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rustbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Aug 14, 2024
@joshtriplett joshtriplett changed the title -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 Aug 14, 2024
@ojeda

This comment was marked as off-topic.

@rustbot

This comment was marked as off-topic.

@ojeda

This comment was marked as off-topic.

@Nadrieril
Copy link
Member

Nadrieril commented Aug 15, 2024

To quote rustbot above: this issue is not meant to be used for technical discussion, please use the Zulip stream for that 🙏

@traviscross
Copy link

cc @rust-lang/lang

@estebank
Copy link

If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Aug 23, 2024

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.

@compiler-errors
Copy link
Member

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.

@compiler-errors
Copy link
Member

@rfcbot concern design-needs-fleshing-out

@traviscross
Copy link

traviscross commented Aug 23, 2024

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:

@joshtriplett joshtriplett changed the title --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 Aug 28, 2024
@compiler-errors
Copy link
Member

We decided in the lang team meeting that this should:

  1. The flag should be called --hint-msrv= or something similar that makes it clear that this is informational.
  2. Not be load-bearing -- this should not affect inference, trait solving, etc. without a full proposal, and I believe that full proposal should be RFC'd. This effectively limits the behavior today to best-effort linting, but we could come up with.
  3. The documentation should note that this is best-effort only, and that the only complete way to ensure MSRV is by downloading the right compiler version and testing it there. This will happen whenever we actually add the flag.

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

@compiler-errors
Copy link
Member

That being said, this doesn't need to be fcp'd until stabilization. It should be gated behind -Zunstable-options.

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Aug 28, 2024

@compiler-errors proposal cancelled.

@compiler-errors
Copy link
Member

compiler-errors commented Aug 28, 2024

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

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Aug 28, 2024
@joshtriplett
Copy link
Member Author

@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 unreachable_patterns/impossible_patterns. Pending such a proposal from @tmandry, I'd suggest that we not use this for that kind of case. (That's separate from the original rationale of improving linting/suggestions in other ways.) That would also make it less of a "hint".

I'll leave it to @tmandry to elaborate on that use case. This isn't a gate/blocker for adding this.

@tmandry
Copy link
Member

tmandry commented Aug 29, 2024

@joshtriplett I responded in Zulip to keep technical discussion off this thread.

ojeda added a commit to ojeda/linux that referenced this issue Sep 3, 2024
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 <[email protected]>
ojeda added a commit to ojeda/linux that referenced this issue Sep 4, 2024
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 <[email protected]>
@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Sep 16, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

10 participants