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.

Sign up for GitHub

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

Jump to bottom

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

Open
fmease opened this issue May 15, 2024 · 4 comments · May be fixed by #125208
Open

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

fmease opened this issue May 15, 2024 · 4 comments · May be fixed by #125208
Assignees
@fmease
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@fmease
Copy link
Member

fmease commented May 15, 2024 •
edited

In #117164 I had to copy the diagnostic structs TyParamFirstLocal and TyParamSome and define the separate structs TyParamFirstLocalLint and TyParamSomeLint to be able to use the translatable diagnostics hir_analysis_ty_param_first_local and hir_analysis_ty_param_some as both a Diagnostic and a LintDiagnostic:

rust/compiler/rustc_hir_analysis/src/errors.rs

Lines 1360 to 1406 in b21b74b

#[derive(Diagnostic)]
#[diag(hir_analysis_ty_param_first_local, code = E0210)]
#[note]
pub struct TyParamFirstLocal<'tcx> {
#[primary_span]
#[label]
pub span: Span,
#[note(hir_analysis_case_note)]
pub note: (),
pub param: Symbol,
pub local_type: Ty<'tcx>,
}
#[derive(LintDiagnostic)]
#[diag(hir_analysis_ty_param_first_local, code = E0210)]
#[note]
pub struct TyParamFirstLocalLint<'tcx> {
#[label]
pub span: Span,
#[note(hir_analysis_case_note)]
pub note: (),
pub param: Symbol,
pub local_type: Ty<'tcx>,
}
#[derive(Diagnostic)]
#[diag(hir_analysis_ty_param_some, code = E0210)]
#[note]
pub struct TyParamSome {
#[primary_span]
#[label]
pub span: Span,
#[note(hir_analysis_only_note)]
pub note: (),
pub param: Symbol,
}
#[derive(LintDiagnostic)]
#[diag(hir_analysis_ty_param_some, code = E0210)]
#[note]
pub struct TyParamSomeLint {
#[label]
pub span: Span,
#[note(hir_analysis_only_note)]
pub note: (),
pub param: Symbol,
}

In #116829, had to duplicate the lint diagnostic struct ReprConflicting and split it into the separate structs ReprConflicting and ReprConflictingLint to be able to use the translatable diagnostic passes_repr_conflicting as both a Diagnostic and a LintDiagnostic:

rust/compiler/rustc_passes/src/errors.rs

Lines 554 to 563 in b21b74b

#[derive(Diagnostic)]
#[diag(passes_repr_conflicting, code = E0566)]
pub struct ReprConflicting {
#[primary_span]
pub hint_spans: Vec<Span>,
}
#[derive(LintDiagnostic)]
#[diag(passes_repr_conflicting, code = E0566)]
pub struct ReprConflictingLint;

In all three cases, I could've probably turned the derivation of LintDiagnostic into a manual impl but that doesn't seem very enticing either for various reasons.

For context, this situation arises whenever we want to emit a diagnostic for something but can't report a hard error unconditionally in all cases for backward compatibility and therefore have to emit the very same diagnostic at different “levels of severity” depending on a set of conditions. With level of severity I'm specifically referring to the set {hard error, lint} here (where lint has its own level of course).

More concretely, the reason why you can't just #[derive(Diagnostic, LintDiagnostic)] struct Ty/*...*/ is because of #[primary_span]: If the diagnostic struct contains a #[primary_span] (which it does most of the time) for the derive macro Diagnostic, then the derive macro LintDiagnostic will reject #[primary_span] rendering the two derives incompatible with one another.

Individually, LintDiagnostic rejecting #[primary_span] makes sense because the span(s) for a lint are to be provided to the function that emits the lint like emit_span_lint, emit_node_span_lint.

There are probably multiple ways to approach this but I'm not super familiar with internals of rustc's linting API. Anyways, I'd like to see this “just work” because it won't be the last time this case will occur.
@fmease fmease added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic labels May 15, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 15, 2024
@fmease fmease removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 15, 2024
@fmease
Copy link
Member Author

fmease commented May 15, 2024 •
edited

I'll probably investigate getting rid of emit_span_lint, emit_node_span_lint and the like in favor of emit_lint etc. (no span / multi-span parameter) and storing the span(s) of LintDiagnostics in the lint diagnostic structs themselves via #[primary_span] which seems to be the most logical approach. However, it'll require updating all existing lint diagnostic structs.

cc @nnethercote

@compiler-errors
Copy link
Member

compiler-errors commented May 16, 2024 •
edited

Yep, I think that LintDiagnostics should be responsible for providing the primary_span to the underlying lint function, just like they do for their primary message. This should be pretty easy to do 👍

@fmease fmease self-assigned this May 16, 2024
@fmease fmease added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label May 16, 2024
@fmease
Copy link
Member Author

fmease commented May 16, 2024 •
edited

Ah, that's annoying. We can't simply leverage Diag::span (inside LintDiagnostic::decorate_lint) unlike Diagnostic because lint_level (lint_level_impl) needs access to the (multi) span before decorate'ing the Diag to suppress lints whose spans come from an external macro (and to disable suggestions):

rust/compiler/rustc_middle/src/lint.rs

Lines 333 to 351 in 4eedf73

if err.span.primary_spans().iter().any(|s| in_external_macro(sess, *s)) {
// Any suggestions made here are likely to be incorrect, so anything we
// emit shouldn't be automatically fixed by rustfix.
err.disable_suggestions();
// If this is a future incompatible that is not an edition fixing lint
// it'll become a hard error, so we have to emit *something*. Also,
// if this lint occurs in the expansion of a macro from an external crate,
// allow individual lints to opt-out from being reported.
let incompatible = future_incompatible.is_some_and(|f| f.reason.edition().is_none());
if !incompatible && !lint.report_in_external_macro {
err.cancel();
// Don't continue further, since we don't want to have
// `diag_span_note_once` called for a diagnostic that isn't emitted.
return;
}
}

By the way, I've already migrated all lint diagnostic structs to the new system.

The only solution I see is adding a span method to trait LintDiagnostic (similar to the existing msg method) which I really wanted to avoid because it makes the API more complicated and bloats the generated code.

@fmease
Copy link
Member Author

fmease commented May 16, 2024

Right now, my patched rustc fails to build std because of that >.<

@fmease fmease linked a pull request May 17, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fmease fmease

Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make lint diagnostics responsible for providing their primary span fmease/rust
3 participants
@compiler-errors @fmease @rustbot