[mejrs]

View all comments

Currently, lints are passed as strings and parsed/split/reformatted/compared/etc in many many places. It's not always clear whether that string includes the tool or not.

Instead, they are passed around as:

pub struct LintName {
    pub tool: Option<Symbol>,
    pub name: Symbol,
}

r? @JonathanBrouwer

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_ir

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

JonathanBrouwer is currently at their maximum review capacity.
They may take a while to respond.

[mejrs]

haven't updated clippy yet. But I think I can do a perf run.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3dcd667 (3dcd66770271913d28347dd2820933e94b0f53a4)
Base parent: 07ea2af (07ea2af746e09a0e9e3ac9b39ee866bd9bf702a2)

[rust-timer]

Finished benchmarking commit (3dcd667): comparison URL.

Overall result: ❌✅ regressions and improvements - BENCHMARK(S) FAILED

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

❗ ❗ ❗ ❗ ❗
Warning ⚠️: The following benchmark(s) failed to build:

• libc-0.2.172
• Job failure

❗ ❗ ❗ ❗ ❗

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	5
Improvements ✅ (primary)	-0.4%	[-1.3%, -0.1%]	45
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	2
All ❌✅ (primary)	-0.4%	[-1.3%, -0.1%]	45

Max RSS (memory usage)

Results (primary -1.0%, secondary -1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.1%	[0.5%, 9.5%]	7
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-5.5%	[-8.3%, -3.2%]	8
All ❌✅ (primary)	-1.0%	[-1.0%, -1.0%]	1

Cycles

Results (primary 2.2%, secondary 0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	3.1%	[0.5%, 12.1%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-4.2%, -0.6%]	6
All ❌✅ (primary)	2.2%	[2.2%, 2.2%]	1

Binary size

Results (primary -0.0%, secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	1
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	-0.0%	[-0.0%, -0.0%]	1

Bootstrap: 489.513s -> 489.49s (-0.00%)
Artifact size: 409.13 MiB -> 409.09 MiB (-0.01%)

[Kivooeo]

huh, any ideas why this might happenened in libc?

   thread 'rustc' (1970916) panicked at /rustc-dev/3dcd66770271913d28347dd2820933e94b0f53a4/compiler/rustc_lint/src/levels.rs:839:25:
    renamed lint does not exist: rustdoc::bare_urls

[mejrs]

huh, any ideas why this might happenened in libc?

   thread 'rustc' (1970916) panicked at /rustc-dev/3dcd66770271913d28347dd2820933e94b0f53a4/compiler/rustc_lint/src/levels.rs:839:25:
    renamed lint does not exist: rustdoc::bare_urls

it's one of the cases where a function takes (lint, tool) but in this case it's passed (rustdoc::bare_urls, None). i just didnt catch this one, it happened for clippy lints too.

[rustbot]

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

[ada4a]

Please apologize the drive-by review. The idea seems very sensible, just some minor questions 👀

View changes since this review

[mejrs]

Please apologize the drive-by review. The idea seems very sensible, just some minor questions 👀

No need to apologize, all reviews are welcome :)

To answer your question w/r to spans: it would make comparing different LintNames complicated, because lint names with equal values but different spans will compare unequal.

And Span: !Ord, so this would require manual Ord impls, and I'd rather not go there...

[mejrs]

Also, possible followups:

• clippy has it's own LintName, it could use this one instead
• store all the builtin lints as symbols rather than string literals. just pre-interning them may also matter for perf.

[mejrs]

@bors try @rust-timer queue

[mejrs]

This code made:

• legacy_numeric_constants work as clippy::legacy_numeric_constants
• #[warn(DEAD_CODE)] would say that "in rustc" there is a lint named dead_code

The fix was simply to delete it, with no other effects.

[JonathanBrouwer]

Why is it ok to remove this? This is a behavior change right?
@rustbot author

[JonathanBrouwer]

Right, this is only for when clippy is running, this should be fine then

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[mejrs]

@rustbot ready

[JonathanBrouwer]

@bors r+
Thanks!

View changes since this review

[rust-bors]

📌 Commit 361557a has been approved by JonathanBrouwer

It is now in the queue for this repository.

[JonathanBrouwer]

@bors rollup=iffy
Making a rollup of perf sensitive prs