Make early lints translatable #124417

Xiretza · 2024-04-26T18:39:31Z

~~Requires projectfluent/fluent-rs#353.~~ 5134a04

r? diagnostics

rustbot · 2024-04-26T18:39:41Z

Some changes occurred in check-cfg diagnostics

cc @Urgau

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Some changes occurred in tests/ui/check-cfg

cc @Urgau

bors · 2024-04-27T03:22:53Z

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

bors · 2024-04-30T16:23:17Z

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

oli-obk · 2024-05-02T13:56:29Z

r? @davidtwco I'm going on vacation for two weeks

bors · 2024-05-14T10:55:10Z

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

fmease · 2024-05-17T16:54:33Z

Are there any blockers left apart from a rebase?

Edit: Ah, it seems like the patch to fluent-rs was only merged but not released yet.

Xiretza · 2024-05-17T18:03:18Z

Yep, I can also patch those NUMBER() calls out and put a TODO there instead, at the cost of slightly worse diagnostics until the next fluent release.

Xiretza · 2024-05-18T13:39:36Z

Rebased, and the use case for NUMBER() has since disappeared (5134a04), so there are no blockers now.

Would appreciate a review on this soon, rebasing this is getting quite tiring.

davidtwco · 2024-05-20T13:34:08Z

Apologies for the delay in my reviewing, had a busy couple weeks, will let @fmease handle this.

fmease

Thanks a lot. this is great work!

I've added a lot of review comments for things that are secondary or preexisting even. You don't need to address them in this PR, they can be fixed in follow-up PRs (by you or my me). Basically drive-by commentary.

I g2g rn unfortunately but I'd like to see emit_buffered_lint being made non-generic as noted in one of my comments. You should be able to pick out what is relevant and what isn't. In a few hours, I will be back again and can push it into the merge queue with a high priority

fmease · 2024-05-18T00:46:33Z

compiler/rustc_session/src/version.rs

@@ -18,3 +23,9 @@ impl Display for RustcVersion {
        write!(formatter, "{}.{}.{}", self.major, self.minor, self.patch)
    }
 }
+
+impl IntoDiagArg for RustcVersion {


(I've started reviewing this PR only a few seconds ago, so later commits may answer this)

How often is RustcVersion used inside diagnostic structs/enums? If it's just once or twice for example, I wonder if such an impl is warranted. Happy to be convinced otherwise.

I mean we probably display values of hundreds (?) of distinct types right now I think but that doesn't mean we should impl IntoDiagArg for them. We do a lot of manual .to_string()s when constructing diag structs and use String which seems fine by me.

fmease · 2024-05-20T12:24:55Z

compiler/rustc_macros/src/diagnostics/diagnostic_builder.rs

+            | SubdiagnosticKind::NoteOnce
+            | SubdiagnosticKind::Help
+            | SubdiagnosticKind::HelpOnce
+            | SubdiagnosticKind::Warn => {
                let inner = info.ty.inner_type();
                if type_matches_path(inner, &["rustc_span", "Span"])
                    || type_matches_path(inner, &["rustc_span", "MultiSpan"])


(preexisting) Oh, this doesn't account for rustc_errors::MultiSpan (pub reexport) and rustc_error_messages::MultiSpan (def site). Anyway, should be fixed at some other point.

compiler/rustc_lint/messages.ftl

compiler/rustc_lint/src/context/diagnostics/check_cfg.rs

fmease · 2024-05-20T14:48:02Z

compiler/rustc_lint/messages.ftl

@@ -395,13 +466,24 @@ lint_map_unit_fn = `Iterator::map` call that discard the iterator's values
    .map_label = after this call to map, the resulting iterator is `impl Iterator<Item = ()>`, which means the only information carried by the iterator is the number of items
    .suggestion = you might have meant to use `Iterator::for_each`

+lint_metavariable_still_repeating = variable '{$name}' is still repeating at this depth


(preexisting) backticks instead of single quotes, not blocking

compiler/rustc_lint/messages.ftl

compiler/rustc_lint/src/context.rs

fmease · 2024-05-20T15:29:59Z

compiler/rustc_lint/src/context/diagnostics.rs

-mod tests;
-
-pub(super) fn builtin(sess: &Session, diagnostic: BuiltinLintDiag, diag: &mut Diag<'_, ()>) {
+pub(super) fn emit_buffered_lint<C: LintContext + ?Sized>(


Could you rewrite you method in such a way that it's not generic? The body of this function is massive and currently, due to monomorphization, it gets duplicated per C (i.e., twice, once for EarlyContext, once for LateContext). While the optimizer and linker are able to throw out duplicate code in some cases, it's obviously not always possible. Furthermore, it's more work for LLVM and the linker (therefore, we have the experimental -Zpolymorphize, however it's still far from usable)

You might need to go back to the approach on master or sth. similar to it.

Uh, actually, this function is only instantiated once for EarlyContext - I honestly don't know why I made it generic in the first place, it compiles fine if I just make it take an EarlyContext?

Edit: ah, right, I moved it out of trait LintContext into EarlyContext, since that's the only type it was actually being called on. Could there be any third-party consumers?

Could there be any third-party consumers?

Clippy (src/tools/clippy, rust-lang/rust-clippy) uses rustc_lint::{LintContext, EarlyContext, LateContext} extensively but it doesn't seem to use buffer_lint

tests/ui/imports/no-pub-reexports-but-used.stderr

fmease · 2024-05-20T15:37:00Z

@bors p=1 prone to bitrotting

Xiretza

~~Still need to address the most pressing concern (making emit_buffered_lint not generic), but I got most of the rest done.~~

#124417 (comment)

compiler/rustc_lint/messages.ftl

tests/ui/imports/no-pub-reexports-but-used.stderr

compiler/rustc_lint/src/context/diagnostics.rs

compiler/rustc_lint_defs/src/lib.rs

compiler/rustc_session/src/parse.rs

Xiretza · 2024-05-20T18:09:05Z

compiler/rustc_expand/src/base.rs

+                                    PROC_MACRO_BACK_COMPAT,
+                                    item.ident.span,
+                                    ast::CRATE_NODE_ID,
+                                    BuiltinLintDiag::ProcMacroBackCompat {


I wasn't sure if other crates might be added here in the future (there were more in the past), so I went for the possibly overkill solution.

rust-timer · 2024-05-22T01:19:01Z

Finished benchmarking commit (791adf7): 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)	1.5%	[0.2%, 6.3%]	66
Regressions ❌ (secondary)	0.4%	[0.3%, 0.5%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-1.1%, -0.6%]	12
All ❌✅ (primary)	1.5%	[0.2%, 6.3%]	66

Max RSS (memory usage)

Results (primary 2.4%)

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)	2.4%	[1.1%, 3.1%]	17
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[1.1%, 3.1%]	17

Cycles

Results (primary 2.4%, secondary 2.2%)

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)	2.4%	[1.2%, 4.9%]	23
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[1.2%, 4.9%]	23

Binary size

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

Bootstrap: 671.076s -> 670.302s (-0.12%)
Artifact size: 315.42 MiB -> 316.14 MiB (0.23%)

nnethercote · 2024-05-22T06:06:00Z

Some sizeable perf regressions. Is that expected?

fmease · 2024-05-22T06:43:06Z

No, it's not expected. I think I know what's going wrong. Sending a fix shortly.

Xiretza · 2024-05-22T06:50:38Z

No, not really - would be interesting to know if this is just for lints that are actually emitted or even for ones that get silenced.

fmease · 2024-05-22T06:55:39Z

@Xiretza I think we no longer decorate the lint Diag lazily inside span_lint_with_diagnostics but eagerly in diagnostics::emit_buffered_lint which is not good because that leads to diags getting constructed(?) even for suppressed / silenced lints as you alluded to.

[perf] Delay construction of early lint diag structs cc rust-lang#124417 (comment) r? ghost

fmease · 2024-05-23T02:01:39Z

In #125410 I was able to ~~partially restore the old performance characteristics~~ tackle some of the regressions.
See #125410 (comment).
Doing some naive calculations in my head, I guess that's not sufficient. Cf:

This PR:   66 × +1.5%_[+0.2%, +6.3%],   7 × +0.4%_[+0.3%, +0.5%].
#125410:   12 × -0.6%_[-0.9%, -0.4%],  13 × -2.0%_[-2.8%, -1.1%].

Anyways, Xiretza and/or I can conduct some more perf experiments after #125410 has been merged. I should be able to squeeze this into my schedule.

[perf] Delay the construction of early lint diag structs Attacks some of the perf regressions from rust-lang#124417 (comment). See individual commits for details. The first three commits are not strictly necessary. However, the 2nd one (06bc4fc, *Remove `LintDiagnostic::msg`*) makes the main change way nicer to implement. It's also pretty sweet on its own if I may say so myself.

[perf] Delay the construction of early lint diag structs Attacks some of the perf regressions from rust-lang/rust#124417 (comment). See individual commits for details. The first three commits are not strictly necessary. However, the 2nd one (06bc4fc67145e3a7be9b5a2cf2b5968cef36e587, *Remove `LintDiagnostic::msg`*) makes the main change way nicer to implement. It's also pretty sweet on its own if I may say so myself.

[perf] Delay the construction of early lint diag structs Attacks some of the perf regressions from rust-lang/rust#124417 (comment). See individual commits for details. The first three commits are not strictly necessary. However, the 2nd one (06bc4fc, *Remove `LintDiagnostic::msg`*) makes the main change way nicer to implement. It's also pretty sweet on its own if I may say so myself.

…diags, r=Nadrieril Spruce up the diagnostics of some early lints Implement the various "*(note to myself) in a follow-up PR we should turn parts of this message into a subdiagnostic (help msg or even struct sugg)*" drive-by comments I left in rust-lang#124417 during my review. For context, before rust-lang#124417, only a few early lints touched/decorated/customized their diagnostic because the former API made it a bit awkward. Likely because of that, things that should've been subdiagnostics were just crammed into the primary message. This PR rectifies this.

Rollup merge of rust-lang#125913 - fmease:early-lints-spruce-up-some-diags, r=Nadrieril Spruce up the diagnostics of some early lints Implement the various "*(note to myself) in a follow-up PR we should turn parts of this message into a subdiagnostic (help msg or even struct sugg)*" drive-by comments I left in rust-lang#124417 during my review. For context, before rust-lang#124417, only a few early lints touched/decorated/customized their diagnostic because the former API made it a bit awkward. Likely because of that, things that should've been subdiagnostics were just crammed into the primary message. This PR rectifies this.

[perf] Delay the construction of early lint diag structs Attacks some of the perf regressions from rust-lang/rust#124417 (comment). See individual commits for details. The first three commits are not strictly necessary. However, the 2nd one (06bc4fc67145e3a7be9b5a2cf2b5968cef36e587, *Remove `LintDiagnostic::msg`*) makes the main change way nicer to implement. It's also pretty sweet on its own if I may say so myself.

[perf] Delay the construction of early lint diag structs Attacks some of the perf regressions from rust-lang/rust#124417 (comment). See individual commits for details. The first three commits are not strictly necessary. However, the 2nd one (06bc4fc, *Remove `LintDiagnostic::msg`*) makes the main change way nicer to implement. It's also pretty sweet on its own if I may say so myself.

rustbot assigned oli-obk Apr 26, 2024

This comment has been minimized.

Sign in to view

Xiretza force-pushed the translate-early-lints branch from 362f064 to 4edd58f Compare April 27, 2024 18:10

This comment has been minimized.

Sign in to view

rustbot assigned davidtwco and unassigned oli-obk May 2, 2024

Xiretza force-pushed the translate-early-lints branch from 4edd58f to eb5690a Compare May 9, 2024 13:16

This comment has been minimized.

Sign in to view

Xiretza mentioned this pull request May 17, 2024

Make lint diagnostics responsible for providing their primary span #125208

Closed

Xiretza force-pushed the translate-early-lints branch from eb5690a to c2d5646 Compare May 18, 2024 13:38

fmease assigned fmease and unassigned davidtwco May 18, 2024

fmease requested changes May 20, 2024

View reviewed changes

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 May 20, 2024

Xiretza commented May 20, 2024

View reviewed changes

Xiretza force-pushed the translate-early-lints branch from c2d5646 to 92749e6 Compare May 20, 2024 18:12

bors added the merged-by-bors This PR was explicitly merged by bors. label May 21, 2024

bors merged commit 791adf7 into rust-lang:master May 21, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 21, 2024

rustbot added the perf-regression Performance regression. label May 22, 2024

fmease mentioned this pull request May 22, 2024

[perf] Delay the construction of early lint diag structs #125410

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024

Auto merge of rust-lang#125410 - fmease:adj-lint-diag-api, r=<try>

c2769b7

[perf] Delay construction of early lint diag structs cc rust-lang#124417 (comment) r? ghost

bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024

Auto merge of rust-lang#125410 - fmease:adj-lint-diag-api, r=<try>

926e924

[perf] Delay construction of early lint diag structs cc rust-lang#124417 (comment) r? ghost

bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024

Auto merge of rust-lang#125410 - fmease:adj-lint-diag-api, r=<try>

7ff5601

[perf] Delay construction of early lint diag structs cc rust-lang#124417 (comment) r? ghost

fmease mentioned this pull request Jun 3, 2024

Spruce up the diagnostics of some early lints #125913

Merged

fmease mentioned this pull request Oct 25, 2024

rustc_macros: Make it possible to derive both Diagnostic and LintDiagnostic on the same type #125169

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make early lints translatable #124417

Make early lints translatable #124417

Xiretza commented Apr 26, 2024 •

edited

Loading

rustbot commented Apr 26, 2024

This comment has been minimized.

bors commented Apr 27, 2024

This comment has been minimized.

bors commented Apr 30, 2024

oli-obk commented May 2, 2024

This comment has been minimized.

bors commented May 14, 2024

fmease commented May 17, 2024 •

edited

Loading

Xiretza commented May 17, 2024

Xiretza commented May 18, 2024 •

edited

Loading

davidtwco commented May 20, 2024

fmease left a comment

fmease May 18, 2024

fmease May 20, 2024

fmease May 20, 2024

fmease May 20, 2024 •

edited

Loading

Xiretza May 20, 2024 •

edited

Loading

fmease May 21, 2024

fmease commented May 20, 2024

Xiretza left a comment •

edited

Loading

Xiretza May 20, 2024

rust-timer commented May 22, 2024

nnethercote commented May 22, 2024

fmease commented May 22, 2024

Xiretza commented May 22, 2024

fmease commented May 22, 2024 •

edited

Loading

fmease commented May 23, 2024 •

edited

Loading

Make early lints translatable #124417

Make early lints translatable #124417

Conversation

Xiretza commented Apr 26, 2024 • edited Loading

rustbot commented Apr 26, 2024

This comment has been minimized.

bors commented Apr 27, 2024

This comment has been minimized.

bors commented Apr 30, 2024

oli-obk commented May 2, 2024

This comment has been minimized.

bors commented May 14, 2024

fmease commented May 17, 2024 • edited Loading

Xiretza commented May 17, 2024

Xiretza commented May 18, 2024 • edited Loading

davidtwco commented May 20, 2024

fmease left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Xiretza May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease commented May 20, 2024

Xiretza left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-timer commented May 22, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

nnethercote commented May 22, 2024

fmease commented May 22, 2024

Xiretza commented May 22, 2024

fmease commented May 22, 2024 • edited Loading

fmease commented May 23, 2024 • edited Loading

Xiretza commented Apr 26, 2024 •

edited

Loading

fmease commented May 17, 2024 •

edited

Loading

Xiretza commented May 18, 2024 •

edited

Loading

fmease May 20, 2024 •

edited

Loading

Xiretza May 20, 2024 •

edited

Loading

Xiretza left a comment •

edited

Loading

fmease commented May 22, 2024 •

edited

Loading

fmease commented May 23, 2024 •

edited

Loading