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

Do not filter empty lint passes & re-do CTFE pass #132637

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Nov 5, 2024

Some structs implement LintPass without having a Lint associated with them #125116 broke that behaviour by filtering them out. This PR ensures that lintless passes are not filtered out.

@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2024

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 5, 2024
@blyxyas blyxyas changed the title Do not filter empty lint passes Do not filter empty lint passes & re-do CTFE pass Nov 5, 2024
Comment on lines 425 to 427
// Lintless passes are always in
lints.is_empty()
|| !lints.iter().all(|lint| lints_that_dont_need_to_run.contains(&LintId::of(lint)))
Copy link
Member

@flip1995 flip1995 Nov 5, 2024

Choose a reason for hiding this comment

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

This condition is quite hard to read now. I suggest to store the second part in a variable:

Suggested change
// Lintless passes are always in
lints.is_empty()
|| !lints.iter().all(|lint| lints_that_dont_need_to_run.contains(&LintId::of(lint)))
let all_lints_allowed = lints.iter().all(|lint| lints_that_dont_need_to_run.contains(&LintId::of(lint)));
// `LintPass`es without lints always run
lints.is_empty() || !all_lints_allowed

(might need formatting)

Copy link
Member Author

Choose a reason for hiding this comment

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

Philipp wait because this PR doesn't handle the whole second part of your comment. As making a sync is a hard task, I assumed you wouldn't want to redo it. I'm currently testing making CTFE a lintless pass.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I can only do the sync after this is merged and got into nightly.

This was just a general note on this. I still like the variable name over the comment. But just a NIT, so feel free to keep either.

@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@flip1995
Copy link
Member

flip1995 commented Nov 5, 2024

I tested the author and dump_hir "lints" locally and they now work with this change. I expect the CTFE also work, but didn't test it and leave the confirmation for this to you @blyxyas . Thanks for addressing this so quickly! r=me once you confirmed that CTFE works (or request another review from the compiler (?) team).

@blyxyas
Copy link
Member Author

blyxyas commented Nov 5, 2024

Just verified CTFE manually, everything works as expected.
@bors r=@flip1995

@bors
Copy link
Contributor

bors commented Nov 5, 2024

📌 Commit 2eac3c0 has been approved by flip1995

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#132259 (rustc_codegen_llvm: Add a new 'pc' option to branch-protection)
 - rust-lang#132409 (CI: switch 7 linux jobs to free runners)
 - rust-lang#132498 (Suggest fixing typos and let bindings at the same time)
 - rust-lang#132524 (chore(style): sync submodule exclusion list between tidy and rustfmt)
 - rust-lang#132567 (Properly suggest `E::assoc` when we encounter `E::Variant::assoc`)
 - rust-lang#132571 (add const_eval_select macro to reduce redundancy)
 - rust-lang#132637 (Do not filter empty lint passes & re-do CTFE pass)
 - rust-lang#132642 (Add documentation on `ast::Attribute`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 560248f into rust-lang:master Nov 6, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 6, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 6, 2024
Rollup merge of rust-lang#132637 - blyxyas:lint-less-passes, r=flip1995

Do not filter empty lint passes & re-do CTFE pass

Some structs implement `LintPass` without having a `Lint` associated with them rust-lang#125116 broke that behaviour by filtering them out. This PR ensures that lintless passes are not filtered out.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 7, 2024
Do not filter empty lint passes & re-do CTFE pass

Some structs implement `LintPass` without having a `Lint` associated with them rust-lang#125116 broke that behaviour by filtering them out. This PR ensures that lintless passes are not filtered out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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