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

Improve diagnostic-related lints: untranslatable_diagnostic & diagnostic_outside_of_impl #128941

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Improve diagnostic-related lints: untranslatable_diagnostic & diagnostic_outside_of_impl #128941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
10ef666
Add more test cases for untranslatable_diagnostic lint
GrigorenkoPV Aug 10, 2024
3cc2a6f
`untranslatable_diagnostic` lint: point at the untranslated thing
GrigorenkoPV Aug 10, 2024
e94a4ee
Refactor: `diagnostic_outside_of_impl`, `untranslatable_diagnostic`
GrigorenkoPV Aug 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 84 additions & 68 deletions compiler/rustc_lint/src/internal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_hir::{
BinOp, BinOpKind, Expr, ExprKind, GenericArg, HirId, Impl, Item, ItemKind, Node, Pat, PatKind,
Path, PathSegment, QPath, Ty, TyKind,
};
use rustc_middle::ty::{self, Ty as MiddleTy};
use rustc_middle::ty::{self, GenericArgsRef, Ty as MiddleTy};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::hygiene::{ExpnKind, MacroKind};
use rustc_span::symbol::{kw, sym, Symbol};
Expand Down Expand Up @@ -415,14 +415,17 @@ declare_lint_pass!(Diagnostics => [UNTRANSLATABLE_DIAGNOSTIC, DIAGNOSTIC_OUTSIDE

impl LateLintPass<'_> for Diagnostics {
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
let collect_args_tys_and_spans = |args: &[Expr<'_>], reserve_one_extra: bool| {
let mut result = Vec::with_capacity(args.len() + usize::from(reserve_one_extra));
result.extend(args.iter().map(|arg| (cx.typeck_results().expr_ty(arg), arg.span)));
result
};
// Only check function calls and method calls.
let (span, def_id, fn_gen_args, call_tys) = match expr.kind {
let (span, def_id, fn_gen_args, arg_tys_and_spans) = match expr.kind {
ExprKind::Call(callee, args) => {
match cx.typeck_results().node_type(callee.hir_id).kind() {
&ty::FnDef(def_id, fn_gen_args) => {
let call_tys: Vec<_> =
args.iter().map(|arg| cx.typeck_results().expr_ty(arg)).collect();
(callee.span, def_id, fn_gen_args, call_tys)
(callee.span, def_id, fn_gen_args, collect_args_tys_and_spans(args, false))
}
_ => return, // occurs for fns passed as args
}
Expand All @@ -432,66 +435,94 @@ impl LateLintPass<'_> for Diagnostics {
else {
return;
};
let mut call_tys: Vec<_> =
args.iter().map(|arg| cx.typeck_results().expr_ty(arg)).collect();
call_tys.insert(0, cx.tcx.types.self_param); // dummy inserted for `self`
(span, def_id, fn_gen_args, call_tys)
let mut args = collect_args_tys_and_spans(args, true);
args.insert(0, (cx.tcx.types.self_param, _recv.span)); // dummy inserted for `self`
(span, def_id, fn_gen_args, args)
}
_ => return,
};

// Is the callee marked with `#[rustc_lint_diagnostics]`?
let has_attr = ty::Instance::try_resolve(cx.tcx, cx.param_env, def_id, fn_gen_args)
.ok()
.flatten()
.is_some_and(|inst| cx.tcx.has_attr(inst.def_id(), sym::rustc_lint_diagnostics));

// Closure: is the type `{D,Subd}iagMessage`?
let is_diag_message = |ty: MiddleTy<'_>| {
if let Some(adt_def) = ty.ty_adt_def()
&& let Some(name) = cx.tcx.get_diagnostic_name(adt_def.did())
&& matches!(name, sym::DiagMessage | sym::SubdiagMessage)
{
true
} else {
false
}
};
Self::diagnostic_outside_of_impl(cx, span, expr.hir_id, def_id, fn_gen_args);
Self::untranslatable_diagnostic(cx, def_id, &arg_tys_and_spans);
}
}

// Does the callee have one or more `impl Into<{D,Subd}iagMessage>` parameters?
let mut impl_into_diagnostic_message_params = vec![];
impl Diagnostics {
// Is the type `{D,Subd}iagMessage`?
fn is_diag_message<'cx>(cx: &LateContext<'cx>, ty: MiddleTy<'cx>) -> bool {
if let Some(adt_def) = ty.ty_adt_def()
&& let Some(name) = cx.tcx.get_diagnostic_name(adt_def.did())
&& matches!(name, sym::DiagMessage | sym::SubdiagMessage)
{
true
} else {
false
}
}

fn untranslatable_diagnostic<'cx>(
cx: &LateContext<'cx>,
def_id: DefId,
arg_tys_and_spans: &[(MiddleTy<'cx>, Span)],
) {
let fn_sig = cx.tcx.fn_sig(def_id).instantiate_identity().skip_binder();
let predicates = cx.tcx.predicates_of(def_id).instantiate_identity(cx.tcx).predicates;
for (i, &param_ty) in fn_sig.inputs().iter().enumerate() {
if let ty::Param(p) = param_ty.kind() {
if let ty::Param(sig_param) = param_ty.kind() {
// It is a type parameter. Check if it is `impl Into<{D,Subd}iagMessage>`.
for pred in predicates.iter() {
if let Some(trait_pred) = pred.as_trait_clause()
&& let trait_ref = trait_pred.skip_binder().trait_ref
&& trait_ref.self_ty() == param_ty // correct predicate for the param?
&& cx.tcx.is_diagnostic_item(sym::Into, trait_ref.def_id)
&& let ty1 = trait_ref.args.type_at(1)
&& is_diag_message(ty1)
&& Self::is_diag_message(cx, ty1)
{
impl_into_diagnostic_message_params.push((i, p.name));
// Calls to methods with an `impl Into<{D,Subd}iagMessage>` parameter must be passed an arg
// with type `{D,Subd}iagMessage` or `impl Into<{D,Subd}iagMessage>`. Otherwise, emit an
// `UNTRANSLATABLE_DIAGNOSTIC` lint.
let (arg_ty, arg_span) = arg_tys_and_spans[i];

// Is the arg type `{Sub,D}iagMessage`or `impl Into<{Sub,D}iagMessage>`?
let is_translatable = Self::is_diag_message(cx, arg_ty)
|| matches!(arg_ty.kind(), ty::Param(arg_param) if arg_param.name == sig_param.name);
if !is_translatable {
cx.emit_span_lint(
UNTRANSLATABLE_DIAGNOSTIC,
arg_span,
UntranslatableDiag,
);
}
}
}
}
}
}

// Is the callee interesting?
if !has_attr && impl_into_diagnostic_message_params.is_empty() {
fn diagnostic_outside_of_impl<'cx>(
cx: &LateContext<'cx>,
span: Span,
current_id: HirId,
def_id: DefId,
fn_gen_args: GenericArgsRef<'cx>,
) {
// Is the callee marked with `#[rustc_lint_diagnostics]`?
let Some(inst) =
ty::Instance::try_resolve(cx.tcx, cx.param_env, def_id, fn_gen_args).ok().flatten()
else {
return;
}
};
let has_attr = cx.tcx.has_attr(inst.def_id(), sym::rustc_lint_diagnostics);
if !has_attr {
return;
};

// Is the parent method marked with `#[rustc_lint_diagnostics]`?
let mut parent_has_attr = false;
for (hir_id, _parent) in cx.tcx.hir().parent_iter(expr.hir_id) {
for (hir_id, _parent) in cx.tcx.hir().parent_iter(current_id) {
if let Some(owner_did) = hir_id.as_owner()
&& cx.tcx.has_attr(owner_did, sym::rustc_lint_diagnostics)
{
parent_has_attr = true;
break;
// The parent method is marked with `#[rustc_lint_diagnostics]`
return;
}
}

Expand All @@ -500,37 +531,22 @@ impl LateLintPass<'_> for Diagnostics {
// - inside a parent function that is itself marked with `#[rustc_lint_diagnostics]`.
//
// Otherwise, emit a `DIAGNOSTIC_OUTSIDE_OF_IMPL` lint.
if has_attr && !parent_has_attr {
let mut is_inside_appropriate_impl = false;
for (_hir_id, parent) in cx.tcx.hir().parent_iter(expr.hir_id) {
debug!(?parent);
if let Node::Item(Item { kind: ItemKind::Impl(impl_), .. }) = parent
&& let Impl { of_trait: Some(of_trait), .. } = impl_
&& let Some(def_id) = of_trait.trait_def_id()
&& let Some(name) = cx.tcx.get_diagnostic_name(def_id)
&& matches!(name, sym::Diagnostic | sym::Subdiagnostic | sym::LintDiagnostic)
{
is_inside_appropriate_impl = true;
break;
}
}
debug!(?is_inside_appropriate_impl);
if !is_inside_appropriate_impl {
cx.emit_span_lint(DIAGNOSTIC_OUTSIDE_OF_IMPL, span, DiagOutOfImpl);
let mut is_inside_appropriate_impl = false;
for (_hir_id, parent) in cx.tcx.hir().parent_iter(current_id) {
debug!(?parent);
if let Node::Item(Item { kind: ItemKind::Impl(impl_), .. }) = parent
&& let Impl { of_trait: Some(of_trait), .. } = impl_
&& let Some(def_id) = of_trait.trait_def_id()
&& let Some(name) = cx.tcx.get_diagnostic_name(def_id)
&& matches!(name, sym::Diagnostic | sym::Subdiagnostic | sym::LintDiagnostic)
{
is_inside_appropriate_impl = true;
break;
}
}

// Calls to methods with an `impl Into<{D,Subd}iagMessage>` parameter must be passed an arg
// with type `{D,Subd}iagMessage` or `impl Into<{D,Subd}iagMessage>`. Otherwise, emit an
// `UNTRANSLATABLE_DIAGNOSTIC` lint.
for (param_i, param_i_p_name) in impl_into_diagnostic_message_params {
// Is the arg type `{Sub,D}iagMessage`or `impl Into<{Sub,D}iagMessage>`?
let arg_ty = call_tys[param_i];
let is_translatable = is_diag_message(arg_ty)
|| matches!(arg_ty.kind(), ty::Param(p) if p.name == param_i_p_name);
if !is_translatable {
cx.emit_span_lint(UNTRANSLATABLE_DIAGNOSTIC, span, UntranslatableDiag);
}
debug!(?is_inside_appropriate_impl);
if !is_inside_appropriate_impl {
cx.emit_span_lint(DIAGNOSTIC_OUTSIDE_OF_IMPL, span, DiagOutOfImpl);
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions tests/ui-fulldeps/internal-lints/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,11 @@ pub fn skipped_because_of_annotation<'a>(dcx: DiagCtxtHandle<'a>) {
fn f(_x: impl Into<DiagMessage>, _y: impl Into<SubdiagMessage>) {}
fn g() {
f(crate::fluent_generated::no_crate_example, crate::fluent_generated::no_crate_example);
f("untranslatable diagnostic", crate::fluent_generated::no_crate_example);
//~^ ERROR diagnostics should be created using translatable messages
f(crate::fluent_generated::no_crate_example, "untranslatable diagnostic");
//~^ ERROR diagnostics should be created using translatable messages
f("untranslatable diagnostic", "untranslatable diagnostic");
//~^ ERROR diagnostics should be created using translatable messages
//~^^ ERROR diagnostics should be created using translatable messages
}
42 changes: 33 additions & 9 deletions tests/ui-fulldeps/internal-lints/diagnostics.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: diagnostics should be created using translatable messages
--> $DIR/diagnostics.rs:43:9
--> $DIR/diagnostics.rs:43:31
|
LL | Diag::new(dcx, level, "untranslatable diagnostic")
| ^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/diagnostics.rs:7:9
Expand All @@ -11,16 +11,16 @@ LL | #![deny(rustc::untranslatable_diagnostic)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: diagnostics should be created using translatable messages
--> $DIR/diagnostics.rs:64:14
--> $DIR/diagnostics.rs:64:19
|
LL | diag.note("untranslatable diagnostic");
| ^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: diagnostics should be created using translatable messages
--> $DIR/diagnostics.rs:85:14
--> $DIR/diagnostics.rs:85:19
|
LL | diag.note("untranslatable diagnostic");
| ^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: diagnostics should only be created in `Diagnostic`/`Subdiagnostic`/`LintDiagnostic` impls
--> $DIR/diagnostics.rs:99:21
Expand All @@ -41,10 +41,34 @@ LL | let _diag = dcx.struct_err("untranslatable diagnostic");
| ^^^^^^^^^^

error: diagnostics should be created using translatable messages
--> $DIR/diagnostics.rs:102:21
--> $DIR/diagnostics.rs:102:32
|
LL | let _diag = dcx.struct_err("untranslatable diagnostic");
| ^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: diagnostics should be created using translatable messages
--> $DIR/diagnostics.rs:120:7
|
LL | f("untranslatable diagnostic", crate::fluent_generated::no_crate_example);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: diagnostics should be created using translatable messages
--> $DIR/diagnostics.rs:122:50
|
LL | f(crate::fluent_generated::no_crate_example, "untranslatable diagnostic");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: diagnostics should be created using translatable messages
--> $DIR/diagnostics.rs:124:7
|
LL | f("untranslatable diagnostic", "untranslatable diagnostic");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: diagnostics should be created using translatable messages
--> $DIR/diagnostics.rs:124:36
|
LL | f("untranslatable diagnostic", "untranslatable diagnostic");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 6 previous errors
error: aborting due to 10 previous errors

Loading