Skip to content

rustc_lint: Enforce rustc::potential_query_instability lint - #119251

Merged
bors merged 2 commits into
rust-lang:masterfrom
Enselic:rustc_lint-query-stability
Dec 31, 2023
Merged

rustc_lint: Enforce rustc::potential_query_instability lint#119251
bors merged 2 commits into
rust-lang:masterfrom
Enselic:rustc_lint-query-stability

Conversation

@Enselic

@EnselicEnselic commented Dec 23, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Stop allowing rustc::potential_query_instability on all of rustc_lint and instead allow it on a case-by-case basis if it is safe to do so. In this particular crate, all lints were safe to allow.

Part of #84447 which is E-help-wanted.

@rustbot

rustbot commented Dec 23, 2023

Copy link
Copy Markdown
Collaborator

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbotrustbot 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 Dec 23, 2023
cjgillot
cjgillot reviewed Dec 23, 2023
View reviewed changes
Comment threadcompiler/rustc_lint/src/levels.rs Outdated
}

if !is_crate_node {
// We break on the first lint, and it does not matter which one.

@cjgillotcjgillotDec 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It may not matter, but emitting a different diagnostic depending on the architecture is a bug.

@EnselicEnselicDec 31, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I changed current_specs() to return a &FxIndexMap, which only cascaded to 13 other places, which seems fine. So now the iteration order here should be deterministic.

(Due to conflicts with master I also had to rebase.)

@EnselicEnselic mentioned this pull request Dec 28, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 29, 2023
@matthiaskrgr
Rollup merge ofrust-lang#119388- Enselic:prevent-lint-triplication,…
c8d4274
… r=cjgillot
rustc_lint: Prevent triplication of various lints
Prevent triplication of various lints. The triplication happens because we run the same lint three times (or less in some cases):
* In `BuiltinCombinedPreExpansionLintPass`
* In `BuiltinCombinedEarlyLintPass`
* In `shallow_lint_levels_on()`
Only run the lints one time by checking the `lint_added_lints` bool.
Set your GitHub diff setting to ignore whitespaces changes when reviewing this PR, since I had to enclose a block inside an if.
Closesrust-lang#73301
(I found this while exploring the code related to [this](rust-lang#119251 (comment)) comment.)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 30, 2023
@rust-timer
Unrolled build forrust-lang#119388
a243ef5
Rollup merge of rust-lang#119388 - Enselic:prevent-lint-triplication, r=cjgillot
rustc_lint: Prevent triplication of various lints
Prevent triplication of various lints. The triplication happens because we run the same lint three times (or less in some cases):
* In `BuiltinCombinedPreExpansionLintPass`
* In `BuiltinCombinedEarlyLintPass`
* In `shallow_lint_levels_on()`
Only run the lints one time by checking the `lint_added_lints` bool.
Set your GitHub diff setting to ignore whitespaces changes when reviewing this PR, since I had to enclose a block inside an if.
Closesrust-lang#73301
(I found this while exploring the code related to [this](rust-lang#119251 (comment)) comment.)
@bors

bors commented Dec 30, 2023

Copy link
Copy Markdown
Collaborator

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

@Enselic
rustc_lint: Make LintLevelsProvider::current_specs() return `&FxInd…
231dbbc
…exMap`
So that lint iteration order becomes predicitable. Discovered with
`rustc::potential_query_instability`.
@Enselic
rustc_lint: Enforce rustc::potential_query_instability lint
295d600
Stop allowing `rustc::potential_query_instability` on all of
`rustc_lint` and instead allow it on a case-by-case basis if it is safe
to do so. In this particular crate, all lints were safe to allow.
@Enselic
Enselicforce-pushed the rustc_lint-query-stability branch from bdc6fc4 to 295d600CompareDecember 31, 2023 13:53
@Enselic

Enselic commented Dec 31, 2023

Copy link
Copy Markdown
MemberAuthor

r? @cjgillot

@rustbotrustbot assigned cjgillot and unassigned wesleywiserDec 31, 2023
@cjgillot

cjgillot commented Dec 31, 2023

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Dec 31, 2023

Copy link
Copy Markdown
Collaborator

📌 Commit 295d600 has been approved by cjgillot

It is now in the queue for this repository.

@borsbors 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 Dec 31, 2023
@bors

bors commented Dec 31, 2023

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 295d600 with merge e51e98d...

@bors

bors commented Dec 31, 2023

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing e51e98d to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Dec 31, 2023
@bors
bors merged commit e51e98d into rust-lang:masterDec 31, 2023
@rustbotrustbot added this to the 1.77.0 milestone Dec 31, 2023
@rust-timer

rust-timer commented Dec 31, 2023

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (e51e98d): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)		0.2%[0.2%, 0.3%]2
Regressions ❌
(secondary)		0.3%[0.2%, 0.4%]8
Improvements ✅
(primary)		--0
Improvements ✅
(secondary)		--0
All ❌✅ (primary)0.2%[0.2%, 0.3%]2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

meanrangecount
Regressions ❌
(primary)		--0
Regressions ❌
(secondary)		--0
Improvements ✅
(primary)		-1.3%[-2.2%, -0.4%]2
Improvements ✅
(secondary)		--0
All ❌✅ (primary)-1.3%[-2.2%, -0.4%]2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 665.69s -> 666.95s (0.19%)
Artifact size: 311.76 MiB -> 311.74 MiB (-0.01%)

@rustbotrustbot added the perf-regression Performance regression. label Dec 31, 2023
@EnselicEnselic mentioned this pull request Jan 1, 2024
Closed
@Enselic

Enselic commented Jan 1, 2024

Copy link
Copy Markdown
MemberAuthor

The performance regression should be fixed by #119488.

@Mark-SimulacrumMark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Jan 8, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjgillotcjgillotcjgillot left review comments

Assignees

@cjgillotcjgillot

Labels

merged-by-borsThis PR was explicitly merged by bors.perf-regressionPerformance regression.perf-regression-triagedThe performance regression has been triaged.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.77.0

Development

Successfully merging this pull request may close these issues.

7 participants

@Enselic@rustbot@bors@cjgillot@rust-timer@wesleywiser@Mark-Simulacrum