Skip to content

Commit

Permalink
Rollup merge of rust-lang#128941 - GrigorenkoPV:internal-diagnostic-l…
Browse files Browse the repository at this point in the history 
…ints, r=davidtwco

 Improve diagnostic-related lints: `untranslatable_diagnostic` & `diagnostic_outside_of_impl`

Summary:
- Made `untranslatable_diagnostic` point to problematic arguments instead of the function call
  (I found this misleading while working on some `A-translation` PRs: my first impression was that
  the methods themselves were not translation-aware and needed to be changed,
  while in reality the problem was with the hardcoded strings passed as arguments).
- Made the shared pass of `untranslatable_diagnostic` & `diagnostic_outside_of_impl` more efficient.

``@rustbot`` label D-imprecise-spans A-translation
  • Loading branch information
@jieyouxu
jieyouxu authored Aug 22, 2024
2 parents 765a045 + e94a4ee commit 126a5c2
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 77 deletions.
152 changes: 84 additions & 68 deletions compiler/rustc_lint/src/internal.rs
View file
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
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
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

0 comments on commit 126a5c2

Please sign in to comment.