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

Stabilize lint_reasons in rustc (RFC 2383) #99063

Closed

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Jul 8, 2022

The day has come, this PR has the necessary changes to stabilize the lint_reasons feature. This covers all changes required in rustc and connected tools. The documentation will be added in a separate PR.

I'll keep this PR as a draft, until the stabilization report has been created in the tracking issue.


r? @wesleywiser

cc: #54503

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 8, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 8, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2022
@xFrednet xFrednet added the F-lint_reasons `#![feature(lint_reasons)]` label Jul 8, 2022
@rust-log-analyzer

This comment has been minimized.

@xFrednet
Copy link
Member Author

xFrednet commented Jul 8, 2022

It seems like Clippy expects the feature to still be present 🤔. Locally, ./x.py test Clippy passed just fine... Does somebody know how to fix this error?

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@xFrednet xFrednet force-pushed the rfc-2383-stabilize-lint-reasons branch from 6bb0b2b to c396df3 Compare July 8, 2022 20:22
@rust-log-analyzer

This comment has been minimized.

@xFrednet xFrednet force-pushed the rfc-2383-stabilize-lint-reasons branch 3 times, most recently from e9d17d2 to 66d7e56 Compare July 8, 2022 21:54
@xFrednet
Copy link
Member Author

xFrednet commented Jul 8, 2022

Hey @wesleywiser, @flip1995, the CI is currently failing, since Clippy requires the feature to be complied, even if the feature has been removed. I'm unsure how to fix this. Does somebody know how to fix this or who I can ask about this? 🙃

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Jul 9, 2022

@xFrednet those errors are from the stage 0 compiler. You need to add cfg(bootstrap) to the feature attribute rather than removing it altogether

@xFrednet
Copy link
Member Author

xFrednet commented Jul 9, 2022

Thank you for the response @jyn514! I've thought about it, but wasn't sure since I expected that clippy is only built with the current rustc version. I've tried adding #![cfg_attr(feature = "bootstrap", feature(lint_reasons))] but that fails the local build. #![cfg_attr(not(feature = "bootstrap"), feature(lint_reasons))] seams to work, I'll test it here in the CI 🙃

@jyn514
Copy link
Member

jyn514 commented Jul 9, 2022

@xFrednet no, I mean cfg(bootstrap), not cfg(feature = "bootstrap"). Your cfg is always false and will fail stage 1 builds.

@xFrednet xFrednet force-pushed the rfc-2383-stabilize-lint-reasons branch from 61736b4 to 47b8fb4 Compare July 9, 2022 12:56
@xFrednet
Copy link
Member Author

xFrednet commented Jul 9, 2022

Ahh, sorry, then I misunderstood you at first. 😅 Now I've changed it to #![cfg_attr(bootstrap, feature(lint_reasons))] 🙃

@xFrednet
Copy link
Member Author

xFrednet commented Jul 9, 2022

The CI is, thank you so much for the support @jyn514! ❤️

@xFrednet xFrednet force-pushed the rfc-2383-stabilize-lint-reasons branch 2 times, most recently from a525ec5 to 8347701 Compare July 9, 2022 15:20
@xFrednet xFrednet marked this pull request as ready for review July 9, 2022 15:57
@bors

This comment was marked as outdated.

@xFrednet xFrednet force-pushed the rfc-2383-stabilize-lint-reasons branch 2 times, most recently from de4eeea to 2b013d4 Compare July 17, 2022 21:48
@xFrednet xFrednet force-pushed the rfc-2383-stabilize-lint-reasons branch from 26b2a33 to 996564b Compare July 18, 2022 22:16
@rust-log-analyzer

This comment has been minimized.

src/doc/rustc/src/lints/levels.md Outdated Show resolved Hide resolved
src/doc/rustc/src/lints/levels.md Outdated Show resolved Hide resolved
src/doc/rustc/src/lints/levels.md Outdated Show resolved Hide resolved
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the small update in the levels.md file, this all LGTM. Not sure if this has to go through an FCP still or what the process here is.

@xFrednet xFrednet force-pushed the rfc-2383-stabilize-lint-reasons branch from 08a1e99 to dbb93b9 Compare July 20, 2022 16:55
@bors
Copy link
Contributor

bors commented Aug 12, 2022

☔ The latest upstream changes (presumably #100419) made this pull request unmergeable. Please resolve the merge conflicts.

@xFrednet
Copy link
Member Author

I'm going to wait with the rebase until the discussion in the tracking issue was resolved :)

@est31
Copy link
Member

est31 commented Aug 27, 2022

👋 Hello, I'm writing this comment in this stabilization PR to notify you, the authors of this PR, that #100591 has been merged, which implemented a change in how features are stabilized.

Your PR has been filed before the change, so will likely require modifications in order to comply with the new rules. I recommend you to:

  1. rebase the PR onto latest master, so that uses of the placeholder are possible.
  2. replace the version numbers in the PR with the placeholder CURRENT_RUSTC_VERSION. For language changes, this means the version numbers in accepted.rs (example: 4caedba). For library changes, this means the since fields (example e576a9b).

That's it! The CURRENT_RUSTC_VERSION placeholder will, as part of the release process, be replaced with the version number that the PR merged for. It can be used anywhere in rust-lang/rust, not just accepted.rs and the since fields.

If you have any questions, feel free to drop by the zulip stream, or ping me directly in this PR's thread. Thanks! 👋

@apiraino
Copy link
Contributor

apiraino commented Sep 1, 2022

Switching to waiting on author to incorporate changes (based on this comment). Feel free to request a review with @rustbot ready, thanks!

@rustbot author

@rustbot rustbot 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 Sep 1, 2022
@wesleywiser wesleywiser added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Oct 14, 2022
@wesleywiser
Copy link
Member

I believe this is still waiting on consensus in the tracking issue so marking as blocked.

@xFrednet feel free to ping to me for reviews when consensus emerges! 🙂

@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 16, 2023
@nyabinary
Copy link

Wha the update on this?

@xFrednet
Copy link
Member Author

xFrednet commented Aug 4, 2023

The stabilization is awaiting a decision from that lang team, about the mental model of the #[expect] attribute. The issue is tagged, we just have to wait until they find the time to give feedback on the issue.

See: #54503

@xFrednet
Copy link
Member Author

I'm closing this in favor of #120924

Thank you for everyone, that participated in this discussion!

@xFrednet xFrednet closed this Feb 11, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2024
…y, r=Urgau,blyxyas

Let's `#[expect]` some lints: Stabilize `lint_reasons` (RFC 2383)

Let's give this another try! The [previous stabilization attempt](rust-lang#99063) was stalled by some unresolved questions. These have been discussed in a [lang team](rust-lang/lang-team#191) meeting. The last open question, regarding the semantics of the `#[expect]` attribute was decided on in rust-lang#115980

I've just updated the [stabilization report](rust-lang#54503 (comment)) with the discussed questions and decisions. Luckily, the decision is inline with the current implementation.

This hopefully covers everything. Let's hope that the CI will be green like the spring.

fixes rust-lang#115980
fixes rust-lang#54503

---

r? `@wesleywiser`

Tacking Issue: rust-lang#54503
Stabilization Report: rust-lang#54503 (comment)
Documentation Update: rust-lang/reference#1237

<!--
For Clippy:

changelog: [`allow_attributes`]: Is now available on stable, since the `lint_reasons` feature was stabilized
changelog: [`allow_attributes_without_reason`]: Is now available on stable, since the `lint_reasons` feature was stabilized
-->

---

Roses are red,
Violets are blue,
Let's expect lints,
With reason clues
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 27, 2024
…u,blyxyas

Let's `#[expect]` some lints: Stabilize `lint_reasons` (RFC 2383)

Let's give this another try! The [previous stabilization attempt](rust-lang/rust#99063) was stalled by some unresolved questions. These have been discussed in a [lang team](rust-lang/lang-team#191) meeting. The last open question, regarding the semantics of the `#[expect]` attribute was decided on in rust-lang/rust#115980

I've just updated the [stabilization report](rust-lang/rust#54503 (comment)) with the discussed questions and decisions. Luckily, the decision is inline with the current implementation.

This hopefully covers everything. Let's hope that the CI will be green like the spring.

fixes #115980
fixes #54503

---

r? `@wesleywiser`

Tacking Issue: rust-lang/rust#54503
Stabilization Report: rust-lang/rust#54503 (comment)
Documentation Update: rust-lang/reference#1237

<!--
For Clippy:

changelog: [`allow_attributes`]: Is now available on stable, since the `lint_reasons` feature was stabilized
changelog: [`allow_attributes_without_reason`]: Is now available on stable, since the `lint_reasons` feature was stabilized
-->

---

Roses are red,
Violets are blue,
Let's expect lints,
With reason clues
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 27, 2024
…u,blyxyas

Let's `#[expect]` some lints: Stabilize `lint_reasons` (RFC 2383)

Let's give this another try! The [previous stabilization attempt](rust-lang/rust#99063) was stalled by some unresolved questions. These have been discussed in a [lang team](rust-lang/lang-team#191) meeting. The last open question, regarding the semantics of the `#[expect]` attribute was decided on in rust-lang/rust#115980

I've just updated the [stabilization report](rust-lang/rust#54503 (comment)) with the discussed questions and decisions. Luckily, the decision is inline with the current implementation.

This hopefully covers everything. Let's hope that the CI will be green like the spring.

fixes #115980
fixes #54503

---

r? `@wesleywiser`

Tacking Issue: rust-lang/rust#54503
Stabilization Report: rust-lang/rust#54503 (comment)
Documentation Update: rust-lang/reference#1237

<!--
For Clippy:

changelog: [`allow_attributes`]: Is now available on stable, since the `lint_reasons` feature was stabilized
changelog: [`allow_attributes_without_reason`]: Is now available on stable, since the `lint_reasons` feature was stabilized
-->

---

Roses are red,
Violets are blue,
Let's expect lints,
With reason clues
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
…u,blyxyas

Let's `#[expect]` some lints: Stabilize `lint_reasons` (RFC 2383)

Let's give this another try! The [previous stabilization attempt](rust-lang/rust#99063) was stalled by some unresolved questions. These have been discussed in a [lang team](rust-lang/lang-team#191) meeting. The last open question, regarding the semantics of the `#[expect]` attribute was decided on in rust-lang/rust#115980

I've just updated the [stabilization report](rust-lang/rust#54503 (comment)) with the discussed questions and decisions. Luckily, the decision is inline with the current implementation.

This hopefully covers everything. Let's hope that the CI will be green like the spring.

fixes #115980
fixes #54503

---

r? `@wesleywiser`

Tacking Issue: rust-lang/rust#54503
Stabilization Report: rust-lang/rust#54503 (comment)
Documentation Update: rust-lang/reference#1237

<!--
For Clippy:

changelog: [`allow_attributes`]: Is now available on stable, since the `lint_reasons` feature was stabilized
changelog: [`allow_attributes_without_reason`]: Is now available on stable, since the `lint_reasons` feature was stabilized
-->

---

Roses are red,
Violets are blue,
Let's expect lints,
With reason clues
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Jul 11, 2024
…u,blyxyas

Let's `#[expect]` some lints: Stabilize `lint_reasons` (RFC 2383)

Let's give this another try! The [previous stabilization attempt](rust-lang/rust#99063) was stalled by some unresolved questions. These have been discussed in a [lang team](rust-lang/lang-team#191) meeting. The last open question, regarding the semantics of the `#[expect]` attribute was decided on in rust-lang/rust#115980

I've just updated the [stabilization report](rust-lang/rust#54503 (comment)) with the discussed questions and decisions. Luckily, the decision is inline with the current implementation.

This hopefully covers everything. Let's hope that the CI will be green like the spring.

fixes #115980
fixes #54503

---

r? `@wesleywiser`

Tacking Issue: rust-lang/rust#54503
Stabilization Report: rust-lang/rust#54503 (comment)
Documentation Update: rust-lang/reference#1237

<!--
For Clippy:

changelog: [`allow_attributes`]: Is now available on stable, since the `lint_reasons` feature was stabilized
changelog: [`allow_attributes_without_reason`]: Is now available on stable, since the `lint_reasons` feature was stabilized
-->

---

Roses are red,
Violets are blue,
Let's expect lints,
With reason clues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-lint_reasons `#![feature(lint_reasons)]` S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.