Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework untranslatable_diagnostic lint #121382

Merged

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Feb 21, 2024

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 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
@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2024

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 nnethercote marked this pull request as draft February 21, 2024 08:55
@nnethercote
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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 nnethercote force-pushed the rework-untranslatable_diagnostic-lint branch from 6a12129 to ccd24fa Compare February 23, 2024 06:18
@rust-log-analyzer

This comment has been minimized.

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

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.

@rustbot rustbot assigned Noratrieb and unassigned davidtwco Feb 28, 2024
@nnethercote nnethercote marked this pull request as ready for review February 28, 2024 10:11
@bors

This comment was marked as resolved.

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

I rebased.

@rust-log-analyzer

This comment has been minimized.

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

@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 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
@bors

This comment was marked as resolved.

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 nnethercote force-pushed the rework-untranslatable_diagnostic-lint branch from 79fb0a9 to 3591e77 Compare March 6, 2024 04:10
@nnethercote
Copy link
Contributor Author

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
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 6, 2024

📌 Commit 3591e77 has been approved by davidtwco

It is now in the queue for this repository.

@bors 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 added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2024
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 6, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 6, 2024
…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
@bors bors merged commit efe9dea into rust-lang:master Mar 6, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 6, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 6, 2024
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``
@nnethercote nnethercote deleted the rework-untranslatable_diagnostic-lint branch March 7, 2024 01:27
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 22, 2024
…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``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants