Move lint level source explanation to the bottom #101986

WaffleLapkin · 2022-09-18T17:10:09Z

So, uhhhhh

User-facing change

"note: #[warn(...)] on by default" and such are moved to the bottom of the diagnostic:

-   = note: `#[warn(unsupported_calling_conventions)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #87678 <https://github.com/rust-lang/rust/issues/87678>
+   = note: `#[warn(unsupported_calling_conventions)]` on by default

Why warning is enabled is the least important thing, so it shouldn't be the first note the user reads, IMO.

Developer-facing change

struct_span_lint and similar methods have a different signature.

Before: ..., impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>)
After: ..., impl Into<DiagnosticMessage>, impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>

The reason for this is that struct_span_lint needs to edit the diagnostic after decorate closure is called. This also makes lint code a little bit nicer in my opinion.

Another option is to use impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>) -> DiagnosticBuilder<'a, ()> altough I don't really see reasons to do let lint = lint.build(message) everywhere.

Subtle problem

By moving the message outside of the closure (that may not be called if the lint is disabled) format!(...) is executed earlier, possibly formatting Ty which may call a query that trims paths that crashes the compiler if there were no warnings...

I don't think it's that big of a deal, considering that we move from format!(...) to fluent (which is lazy by-default) anyway, however this required adding a workaround which is unfortunate.

P.S.

I'm sorry, I do not how to make this PR smaller/easier to review. Changes to the lint API affect SO MUCH 😢

rustbot · 2022-09-18T17:10:13Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in const_evaluatable.rs

cc @lcnr

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler/rustc_middle/src/lint.rs

cjgillot · 2022-09-19T17:16:35Z

compiler/rustc_error_messages/src/lib.rs

+/// A workaround for "good path" ICEs when formatting types in disables lints.
+///
+/// Delays formatting until `.into(): DiagnosticMessage` is used.
+pub struct DelayDm<F>(pub F);


Will this go away after the migration to translatable messages?

Yes, this is the intention.

Should we add a rustc lint to detect when we're using Ty: Display without wrapping it in this? (Maybe not to address in this PR, but I know I'm bound to make that mistake myself at some point in the future.)

I think we should add a lint, yes.

compiler/rustc_middle/src/lint.rs

compiler/rustc_trait_selection/src/traits/specialize/mod.rs

compiler/rustc_lint/src/context.rs

compiler/rustc_typeck/src/check/check.rs

compiler/rustc_typeck/src/check/cast.rs

compiler/rustc_lint/src/types.rs

estebank · 2022-09-20T17:02:35Z

compiler/rustc_mir_build/src/thir/pattern/check_match.rs

@@ -561,26 +565,28 @@ fn check_for_bindings_named_same_as_variants(
                BINDINGS_WITH_VARIANT_NAME,
                p.hir_id,
                p.span,
+                DelayDm(|| format!(


Idle thought: maybe let's make this argument always DelayDm (or maybe "just" the closure?) That way you make it less likely to make the mistake? Not necessary for this PR (just a thought so we don't impl yet another complex internal lint.)

That is an option. Maybe a trait like IntoLintMessage implemented for F: FnOnce() -> String and the fluent thing. We could also do the same with Into<SubdiagnosticMessage>, but that would also affect non-lints, so idk.

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

estebank · 2022-09-22T14:46:22Z

You'll need to re--bless :)

WaffleLapkin · 2022-09-22T16:13:33Z

I forget that clippy exists every time xd

rustbot · 2022-09-23T09:41:58Z

The Miri subtree was changed

cc @rust-lang/miri

WaffleLapkin · 2022-09-23T19:49:09Z

I think this should be at least rollup=never, since it will conflict with basically any lint change.

bors · 2022-09-24T11:59:58Z

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

estebank · 2022-09-28T14:52:36Z

r=me after rebasing

WaffleLapkin · 2022-09-29T18:46:14Z

I got more conflicts, while rebasing ✌️

brb rebasing again

rust-timer · 2022-10-01T15:15:41Z

Finished benchmarking commit (744e397): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.0%	[0.9%, 1.0%]	2
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.2%]	3
Improvements ✅ (secondary)	-0.9%	[-1.4%, -0.3%]	8
All ❌✅ (primary)	-0.3%	[-0.3%, -0.2%]	3

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.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.1%	[3.6%, 4.6%]	2
Improvements ✅ (primary)	-3.0%	[-3.3%, -2.7%]	2
Improvements ✅ (secondary)	-3.1%	[-5.8%, -1.3%]	5
All ❌✅ (primary)	-3.0%	[-3.3%, -2.7%]	2

Cycles

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.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-3.3%	[-3.6%, -3.0%]	2
All ❌✅ (primary)	-2.6%	[-2.6%, -2.6%]	1

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-10-02T23:39:57Z

The tiny icount regressions are matched or exceeded by the tiny icount improvements. This seems fine.

@rustbot label: +perf-regression-triaged

…illot Remove a couple lifetimes that can be infered From the review: rust-lang#101986 (comment) r? `@cjgillot`

…ottom, r=estebank Move lint level source explanation to the bottom So, uhhhhh r? `@estebank` ## User-facing change "note: `#[warn(...)]` on by default" and such are moved to the bottom of the diagnostic: ```diff - = note: `#[warn(unsupported_calling_conventions)]` on by default = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#87678 <rust-lang#87678> + = note: `#[warn(unsupported_calling_conventions)]` on by default ``` Why warning is enabled is the least important thing, so it shouldn't be the first note the user reads, IMO. ## Developer-facing change `struct_span_lint` and similar methods have a different signature. Before: `..., impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>)` After: `..., impl Into<DiagnosticMessage>, impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>` The reason for this is that `struct_span_lint` needs to edit the diagnostic _after_ `decorate` closure is called. This also makes lint code a little bit nicer in my opinion. Another option is to use `impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>) -> DiagnosticBuilder<'a, ()>` altough I don't _really_ see reasons to do `let lint = lint.build(message)` everywhere. ## Subtle problem By moving the message outside of the closure (that may not be called if the lint is disabled) `format!(...)` is executed earlier, possibly formatting `Ty` which may call a query that trims paths that crashes the compiler if there were no warnings... I don't think it's that big of a deal, considering that we move from `format!(...)` to `fluent` (which is lazy by-default) anyway, however this required adding a workaround which is unfortunate. ## P.S. I'm sorry, I do not how to make this PR smaller/easier to review. Changes to the lint API affect SO MUCH 😢

RalfJung · 2022-10-11T21:54:59Z

compiler/rustc_middle/src/lint.rs

@@ -373,12 +386,12 @@ pub fn struct_lint_level<'s, 'd>(
        if let Level::Expect(_) = level {
            let name = lint.name_lower();
            err.code(DiagnosticId::Lint { name, has_future_breakage, is_force_warn: false });
-            decorate(LintDiagnosticBuilder::new(err));
+
+            decorate(&mut err);


Why does decorate have a return vale, if it is being ignored?

To optimize the most common case, #101986 (comment)

Could you add doc comments explaining that to all functions that take such a strangely typed closure? I spent 15min digging through the code to figure out what the return value does...

yeah, sure xD

<rust-lang/rust#101986>

Slightly tweak comments wrt `lint_overflowing_range_endpoint` From the review: rust-lang#101986 (comment) It _seemed_ that the lint was not emitted when the `if` check failed, but _actually_ this happens already in a special case and the lint is emitted outside of this function, if this function doesn't. I've cleared up the code/comments a bit, so it's more obvious :) r? `@estebank`

Slightly tweak comments wrt `lint_overflowing_range_endpoint` From the review: rust-lang#101986 (comment) It _seemed_ that the lint was not emitted when the `if` check failed, but _actually_ this happens already in a special case and the lint is emitted outside of this function, if this function doesn't. I've cleared up the code/comments a bit, so it's more obvious :) r? ``@estebank``

Slightly tweak comments wrt `lint_overflowing_range_endpoint` From the review: rust-lang#101986 (comment) It _seemed_ that the lint was not emitted when the `if` check failed, but _actually_ this happens already in a special case and the lint is emitted outside of this function, if this function doesn't. I've cleared up the code/comments a bit, so it's more obvious :) r? ```@estebank```

…ottom, r=estebank Move lint level source explanation to the bottom So, uhhhhh r? `@estebank` ## User-facing change "note: `#[warn(...)]` on by default" and such are moved to the bottom of the diagnostic: ```diff - = note: `#[warn(unsupported_calling_conventions)]` on by default = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#87678 <rust-lang#87678> + = note: `#[warn(unsupported_calling_conventions)]` on by default ``` Why warning is enabled is the least important thing, so it shouldn't be the first note the user reads, IMO. ## Developer-facing change `struct_span_lint` and similar methods have a different signature. Before: `..., impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>)` After: `..., impl Into<DiagnosticMessage>, impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>` The reason for this is that `struct_span_lint` needs to edit the diagnostic _after_ `decorate` closure is called. This also makes lint code a little bit nicer in my opinion. Another option is to use `impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>) -> DiagnosticBuilder<'a, ()>` altough I don't _really_ see reasons to do `let lint = lint.build(message)` everywhere. ## Subtle problem By moving the message outside of the closure (that may not be called if the lint is disabled) `format!(...)` is executed earlier, possibly formatting `Ty` which may call a query that trims paths that crashes the compiler if there were no warnings... I don't think it's that big of a deal, considering that we move from `format!(...)` to `fluent` (which is lazy by-default) anyway, however this required adding a workaround which is unfortunate. ## P.S. I'm sorry, I do not how to make this PR smaller/easier to review. Changes to the lint API affect SO MUCH 😢

Remove a couple lifetimes that can be infered From the review: rust-lang/rust#101986 (comment) r? `@cjgillot`

rust-highfive assigned estebank Sep 18, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2022

This comment has been minimized.

Sign in to view

cjgillot reviewed Sep 19, 2022

View reviewed changes

oli-obk reviewed Sep 20, 2022

View reviewed changes

compiler/rustc_lint/src/context.rs Show resolved Hide resolved