Rework `untranslatable_diagnostic` lint #121382

nnethercote · 2024-02-21T08:54:55Z

Currently it only checks calls to functions marked with #[rustc_lint_diagnostics]. This PR changes it to check calls to any function with an impl Into<{D,Subd}iagnosticMessage> parameter. This greatly improves its coverage and doesn't rely on people remembering to add #[rustc_lint_diagnostics]. It also lets us add #[rustc_lint_diagnostics] to a number of functions that don't have an impl Into<{D,Subd}iagnosticMessage>, such as Diag::span.

r? @davidtwco

rustbot · 2024-02-21T08:55:04Z

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

cc @rust-lang/rust-analyzer

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

nnethercote · 2024-02-21T08:57:15Z

@davidtwco: this isn't ready to merge, mostly because the lint code itself is still very hacky. But I'd like to hear what you think of the general idea. It's clear that the new version of the lint finds lots of things the old version doesn't.

I also wonder if the diagnostic_outside_of_impl lint is even necessary any more. I'm struggling to think of what problematic code it could find that the expanded untranslated_diagnostic couldn't find.

davidtwco · 2024-02-21T14:59:31Z

I think this is a great idea, I hadn't thought to check for the Into<DiagnosticMessage> when writing this initially and it is much less fragile.

diagnostic_outside_of_impl was just intended to steer new diagnostics towards using structs instead of the builder API - it's unclear how much we want this, we're going to be discussing that in wg-diagnostics soon, but for now it should probably stay.

nnethercote · 2024-02-21T20:56:37Z

diagnostic_outside_of_impl was just intended to steer new diagnostics towards using structs instead of the builder API

I guess you could do something like let diag = dcx.struct_err(crate::fluent_generated::blah_blah_blah). Doesn't seem to happen much in practice, though I'm happy to leave it unchanged for now.

nnethercote · 2024-02-28T10:11:32Z

r? @Nilstrieb, mostly for the check_expr code which is the part I'm not sure about and has some njn comments indicating things that need polish.

nnethercote · 2024-02-29T00:58:25Z

I rebased.

nnethercote · 2024-03-04T02:01:35Z

@Nilstrieb I have polished this up and it's ready for review. It's possible that there is a better way to implement parts of check_expr. Nonetheless, as written in the PR it's a large improvement over the existing lint implementation, so I think it would be fine to merge in its current state. Please let me know if you are not the right person to review this, and we can find someone more appropriate.

compiler/rustc_lint/src/internal.rs

From `impl Into<DiagnosticMessage>` to `impl Into<Cow<'static, str>>`. Because these functions don't produce user-facing output and we don't want their strings to be translated.

Currently it only checks calls to functions marked with `#[rustc_lint_diagnostics]`. This commit changes it to check calls to any function with an `impl Into<{D,Subd}iagMessage>` parameter. This greatly improves its coverage and doesn't rely on people remembering to add `#[rustc_lint_diagnostics]`. The commit also adds `#[allow(rustc::untranslatable_diagnostic)`] attributes to places that need it that are caught by the improved lint. These places that might be easy to convert to translatable diagnostics. Finally, it also: - Expands and corrects some comments. - Does some minor formatting improvements. - Adds missing `DecorateLint` cases to `tests/ui-fulldeps/internal-lints/diagnostics.rs`.

Prior to the previous commit, `#[rust_lint_diagnostics]` attributes could only be used on methods with an `impl Into<{D,Subd}iagMessage>` parameter. But there are many other nearby diagnostic methods (e.g. `Diag::span`) that don't take such a parameter and should have the attribute. This commit adds the missing attribute to these `Diag` methods. This requires adding some missing `#[allow(rustc::diagnostic_outside_of_impl)]` markers at call sites to these methods.

nnethercote · 2024-03-06T04:12:50Z

I have updated this to use fn_sig, which now means the checking is precise and everything looks much nicer. I think this is ready to go.

davidtwco · 2024-03-06T13:31:16Z

@bors r

bors · 2024-03-06T13:31:18Z

📌 Commit 3591e77 has been approved by davidtwco

It is now in the queue for this repository.

…diagnostic-lint, r=davidtwco Rework `untranslatable_diagnostic` lint Currently it only checks calls to functions marked with `#[rustc_lint_diagnostics]`. This PR changes it to check calls to any function with an `impl Into<{D,Subd}iagnosticMessage>` parameter. This greatly improves its coverage and doesn't rely on people remembering to add `#[rustc_lint_diagnostics]`. It also lets us add `#[rustc_lint_diagnostics]` to a number of functions that don't have an `impl Into<{D,Subd}iagnosticMessage>`, such as `Diag::span`. r? `@davidtwco`

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113518 (bootstrap/libtest: print test name eagerly on failure even with `verbose-tests=false` / `--quiet`) - rust-lang#117199 (Change the documented implicit value of `-C instrument-coverage` to `=yes`) - rust-lang#121190 (avoid overlapping privacy suggestion for single nested imports) - rust-lang#121382 (Rework `untranslatable_diagnostic` lint) - rust-lang#121833 (Suggest correct path in include_bytes!) - rust-lang#121959 (Removing absolute path in proc-macro) - rust-lang#122038 (Fix linting paths with qself in `unused_qualifications`) - rust-lang#122051 (cleanup: remove zero-offset GEP) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#113518 (bootstrap/libtest: print test name eagerly on failure even with `verbose-tests=false` / `--quiet`) - rust-lang#117199 (Change the documented implicit value of `-C instrument-coverage` to `=yes`) - rust-lang#121190 (avoid overlapping privacy suggestion for single nested imports) - rust-lang#121382 (Rework `untranslatable_diagnostic` lint) - rust-lang#121959 (Removing absolute path in proc-macro) - rust-lang#122038 (Fix linting paths with qself in `unused_qualifications`) - rust-lang#122051 (cleanup: remove zero-offset GEP) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#121382 - nnethercote:rework-untranslatable_diagnostic-lint, r=davidtwco Rework `untranslatable_diagnostic` lint Currently it only checks calls to functions marked with `#[rustc_lint_diagnostics]`. This PR changes it to check calls to any function with an `impl Into<{D,Subd}iagnosticMessage>` parameter. This greatly improves its coverage and doesn't rely on people remembering to add `#[rustc_lint_diagnostics]`. It also lets us add `#[rustc_lint_diagnostics]` to a number of functions that don't have an `impl Into<{D,Subd}iagnosticMessage>`, such as `Diag::span`. r? ``@davidtwco``

…diagnostic-lint, r=davidtwco Rework `untranslatable_diagnostic` lint Currently it only checks calls to functions marked with `#[rustc_lint_diagnostics]`. This PR changes it to check calls to any function with an `impl Into<{D,Subd}iagnosticMessage>` parameter. This greatly improves its coverage and doesn't rely on people remembering to add `#[rustc_lint_diagnostics]`. It also lets us add `#[rustc_lint_diagnostics]` to a number of functions that don't have an `impl Into<{D,Subd}iagnosticMessage>`, such as `Diag::span`. r? ``@davidtwco``

rustbot assigned davidtwco Feb 21, 2024

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 Feb 21, 2024

nnethercote marked this pull request as draft February 21, 2024 08:55

nnethercote force-pushed the rework-untranslatable_diagnostic-lint branch from 6a12129 to ccd24fa Compare February 23, 2024 06:18

This comment has been minimized.

Sign in to view

nnethercote force-pushed the rework-untranslatable_diagnostic-lint branch from ccd24fa to f8a3c6a Compare February 28, 2024 10:10

rustbot assigned Noratrieb and unassigned davidtwco Feb 28, 2024

nnethercote marked this pull request as ready for review February 28, 2024 10:11

This comment was marked as resolved.

Sign in to view

nnethercote force-pushed the rework-untranslatable_diagnostic-lint branch 2 times, most recently from 6d9d025 to de294af Compare February 29, 2024 00:58

This comment has been minimized.

Sign in to view

nnethercote force-pushed the rework-untranslatable_diagnostic-lint branch 2 times, most recently from 9e0a86b to 79fb0a9 Compare March 4, 2024 01:59

Noratrieb reviewed Mar 4, 2024

View reviewed changes

compiler/rustc_lint/src/internal.rs Outdated Show resolved Hide resolved

compiler/rustc_lint/src/internal.rs Outdated Show resolved Hide resolved

compiler/rustc_lint/src/internal.rs Outdated Show resolved Hide resolved

nnethercote commented Mar 4, 2024

View reviewed changes

compiler/rustc_lint/src/internal.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

nnethercote added 3 commits March 5, 2024 17:11

Change message type in bug functions.

d602394

From `impl Into<DiagnosticMessage>` to `impl Into<Cow<'static, str>>`. Because these functions don't produce user-facing output and we don't want their strings to be translated.

nnethercote force-pushed the rework-untranslatable_diagnostic-lint branch from 79fb0a9 to 3591e77 Compare March 6, 2024 04:10

davidtwco approved these changes Mar 6, 2024

View reviewed changes

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 Mar 6, 2024

matthiaskrgr mentioned this pull request Mar 6, 2024

Rollup of 8 pull requests #122097

Closed

matthiaskrgr mentioned this pull request Mar 6, 2024

Rollup of 7 pull requests #122111

Merged

bors merged commit efe9dea into rust-lang:master Mar 6, 2024
11 checks passed

rustbot added this to the 1.78.0 milestone Mar 6, 2024

nnethercote deleted the rework-untranslatable_diagnostic-lint branch March 7, 2024 01:27

y21 mentioned this pull request Mar 10, 2024

accept String in span_lint* functions directly to avoid unnecessary clones rust-lang/rust-clippy#12453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework `untranslatable_diagnostic` lint #121382

Rework `untranslatable_diagnostic` lint #121382

nnethercote commented Feb 21, 2024 •

edited

Loading

rustbot commented Feb 21, 2024

nnethercote commented Feb 21, 2024

davidtwco commented Feb 21, 2024

nnethercote commented Feb 21, 2024

This comment has been minimized.

nnethercote commented Feb 28, 2024

This comment was marked as resolved.

nnethercote commented Feb 29, 2024

This comment has been minimized.

nnethercote commented Mar 4, 2024

This comment was marked as resolved.

nnethercote commented Mar 6, 2024

davidtwco commented Mar 6, 2024

bors commented Mar 6, 2024

Rework untranslatable_diagnostic lint #121382

Rework untranslatable_diagnostic lint #121382

Conversation

nnethercote commented Feb 21, 2024 • edited Loading

rustbot commented Feb 21, 2024

nnethercote commented Feb 21, 2024

davidtwco commented Feb 21, 2024

nnethercote commented Feb 21, 2024

This comment has been minimized.

nnethercote commented Feb 28, 2024

This comment was marked as resolved.

nnethercote commented Feb 29, 2024

This comment has been minimized.

nnethercote commented Mar 4, 2024

This comment was marked as resolved.

nnethercote commented Mar 6, 2024

davidtwco commented Mar 6, 2024

bors commented Mar 6, 2024

Rework `untranslatable_diagnostic` lint #121382

Rework `untranslatable_diagnostic` lint #121382

nnethercote commented Feb 21, 2024 •

edited

Loading