Rollup merge of #116787 - a-lafrance:span-internal-lint, r=oli-obk

Implement an internal lint encouraging use of `Span::eq_ctxt`

Adds a new Rustc internal lint that forbids use of `.ctxt() == .ctxt()` for spans, encouraging use of `.eq_ctxt()` instead (see https://github.com/rust-lang/rust/issues/49509).

Also fixed a few violations of the lint in the Rustc codebase (a fun additional way I could test my code). Edit: MIR opt folks I believe that's why you're CC'ed, just a heads up.

Two things I'm not sure about:
1. The way I chose to detect calls to `Span::ctxt`. I know adding diagnostic items to methods is generally discouraged, but after some searching and experimenting I couldn't find anything else that worked, so I went with it. That said, I'm happy to implement something different if there's a better way out there. (For what it's worth, if there is a better way, it might be worth documenting in the rustc-dev-guide, which I'm happy to take care of)
2. The error message for the lint. Ideally it would probably be good to give some context as to why the suggestion is made (e.g. `rustc::default_hash_types` tells the user that it's because of performance), but I don't have that context so I couldn't put it in the error message. Happy to iterate on the error message based on feedback during review.

r? ``@oli-obk``
This commit is contained in:
Matthias Krüger 2023-10-17 19:07:23 +02:00 committed by GitHub
commit 3ea438eb3a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
25 changed files with 89 additions and 21 deletions

View File

@ -650,7 +650,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
.sess
.source_map()
.is_multiline(call_expr.span.with_lo(callee_expr.span.hi()))
&& call_expr.span.ctxt() == callee_expr.span.ctxt();
&& call_expr.span.eq_ctxt(callee_expr.span);
if call_is_multiline {
err.span_suggestion(
callee_expr.span.shrink_to_hi(),

View File

@ -494,6 +494,8 @@ lint_renamed_lint = lint `{$name}` has been renamed to `{$replace}`
lint_requested_level = requested on the command line with `{$level} {$lint_name}`
lint_span_use_eq_ctxt = use `.eq_ctxt()` instead of `.ctxt() == .ctxt()`
lint_supertrait_as_deref_target = `{$t}` implements `Deref` with supertrait `{$target_principal}` as target
.label = target type is set here

View File

@ -3,14 +3,14 @@
use crate::lints::{
BadOptAccessDiag, DefaultHashTypesDiag, DiagOutOfImpl, LintPassByHand, NonExistentDocKeyword,
QueryInstability, TyQualified, TykindDiag, TykindKind, UntranslatableDiag,
QueryInstability, SpanUseEqCtxtDiag, TyQualified, TykindDiag, TykindKind, UntranslatableDiag,
UntranslatableDiagnosticTrivial,
};
use crate::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};
use rustc_ast as ast;
use rustc_hir::def::Res;
use rustc_hir::{def_id::DefId, Expr, ExprKind, GenericArg, PatKind, Path, PathSegment, QPath};
use rustc_hir::{HirId, Impl, Item, ItemKind, Node, Pat, Ty, TyKind};
use rustc_hir::{BinOp, BinOpKind, HirId, Impl, Item, ItemKind, Node, Pat, Ty, TyKind};
use rustc_middle::ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::hygiene::{ExpnKind, MacroKind};
@ -537,3 +537,33 @@ impl LateLintPass<'_> for BadOptAccess {
}
}
}
declare_tool_lint! {
pub rustc::SPAN_USE_EQ_CTXT,
Allow,
"forbid uses of `==` with `Span::ctxt`, suggest `Span::eq_ctxt` instead",
report_in_external_macro: true
}
declare_lint_pass!(SpanUseEqCtxt => [SPAN_USE_EQ_CTXT]);
impl<'tcx> LateLintPass<'tcx> for SpanUseEqCtxt {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'_>) {
if let ExprKind::Binary(BinOp { node: BinOpKind::Eq, .. }, lhs, rhs) = expr.kind {
if is_span_ctxt_call(cx, lhs) && is_span_ctxt_call(cx, rhs) {
cx.emit_spanned_lint(SPAN_USE_EQ_CTXT, expr.span, SpanUseEqCtxtDiag);
}
}
}
}
fn is_span_ctxt_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
match &expr.kind {
ExprKind::MethodCall(..) => cx
.typeck_results()
.type_dependent_def_id(expr.hir_id)
.is_some_and(|call_did| cx.tcx.is_diagnostic_item(sym::SpanCtxt, call_did)),
_ => false,
}
}

View File

@ -531,6 +531,8 @@ fn register_internals(store: &mut LintStore) {
store.register_late_mod_pass(|_| Box::new(BadOptAccess));
store.register_lints(&PassByValue::get_lints());
store.register_late_mod_pass(|_| Box::new(PassByValue));
store.register_lints(&SpanUseEqCtxt::get_lints());
store.register_late_mod_pass(|_| Box::new(SpanUseEqCtxt));
// FIXME(davidtwco): deliberately do not include `UNTRANSLATABLE_DIAGNOSTIC` and
// `DIAGNOSTIC_OUTSIDE_OF_IMPL` here because `-Wrustc::internal` is provided to every crate and
// these lints will trigger all of the time - change this once migration to diagnostic structs
@ -548,6 +550,7 @@ fn register_internals(store: &mut LintStore) {
LintId::of(USAGE_OF_QUALIFIED_TY),
LintId::of(EXISTING_DOC_KEYWORD),
LintId::of(BAD_OPT_ACCESS),
LintId::of(SPAN_USE_EQ_CTXT),
],
);
}

View File

@ -900,6 +900,10 @@ pub struct QueryInstability {
pub query: Symbol,
}
#[derive(LintDiagnostic)]
#[diag(lint_span_use_eq_ctxt)]
pub struct SpanUseEqCtxtDiag;
#[derive(LintDiagnostic)]
#[diag(lint_tykind_kind)]
pub struct TykindKind {

View File

@ -404,7 +404,7 @@ impl<'a> CoverageSpansGenerator<'a> {
let Some(visible_macro) = curr.visible_macro(self.body_span) else { return };
if let Some(prev) = &self.some_prev
&& prev.expn_span.ctxt() == curr.expn_span.ctxt()
&& prev.expn_span.eq_ctxt(curr.expn_span)
{
return;
}

View File

@ -231,7 +231,7 @@ impl<'a, 'hir> CheckLoopVisitor<'a, 'hir> {
AsyncClosure(closure_span) => {
self.sess.emit_err(BreakInsideAsyncBlock { span, closure_span, name });
}
UnlabeledBlock(block_span) if is_break && block_span.ctxt() == break_span.ctxt() => {
UnlabeledBlock(block_span) if is_break && block_span.eq_ctxt(break_span) => {
let suggestion = Some(OutsideLoopSuggestion { block_span, break_span });
self.sess.emit_err(OutsideLoop { span, name, is_break, suggestion });
}

View File

@ -212,6 +212,7 @@ impl Span {
/// This function is used as a fast path when decoding the full `SpanData` is not necessary.
/// It's a cut-down version of `data_untracked`.
#[cfg_attr(not(test), rustc_diagnostic_item = "SpanCtxt")]
#[inline]
pub fn ctxt(self) -> SyntaxContext {
if self.len_with_tag_or_marker != BASE_LEN_INTERNED_MARKER {

View File

@ -303,6 +303,7 @@ symbols! {
SliceIndex,
SliceIter,
Some,
SpanCtxt,
String,
StructuralEq,
StructuralPartialEq,

View File

@ -701,7 +701,7 @@ fn deref_method_same_type<'tcx>(result_ty: Ty<'tcx>, arg_ty: Ty<'tcx>) -> bool {
fn in_postfix_position<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> bool {
if let Some(parent) = get_parent_expr(cx, e)
&& parent.span.ctxt() == e.span.ctxt()
&& parent.span.eq_ctxt(e.span)
{
match parent.kind {
ExprKind::Call(child, _) | ExprKind::MethodCall(_, child, _, _) | ExprKind::Index(child, _, _)

View File

@ -241,7 +241,7 @@ fn try_parse_contains<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Optio
},
],
_,
) if key_span.ctxt() == expr.span.ctxt() => {
) if key_span.eq_ctxt(expr.span) => {
let id = cx.typeck_results().type_dependent_def_id(expr.hir_id)?;
let expr = ContainsExpr {
negated,

View File

@ -274,7 +274,7 @@ fn check_array(cx: &EarlyContext<'_>, expr: &Expr) {
for element in array {
if_chain! {
if let ExprKind::Binary(ref op, ref lhs, _) = element.kind;
if has_unary_equivalent(op.node) && lhs.span.ctxt() == op.span.ctxt();
if has_unary_equivalent(op.node) && lhs.span.eq_ctxt(op.span);
let space_span = lhs.span.between(op.span);
if let Some(space_snippet) = snippet_opt(cx, space_span);
let lint_span = lhs.span.with_lo(lhs.span.hi());

View File

@ -31,7 +31,7 @@ impl LateLintPass<'_> for UnderscoreTyped {
if !in_external_macro(cx.tcx.sess, local.span);
if let Some(ty) = local.ty; // Ensure that it has a type defined
if let TyKind::Infer = &ty.kind; // that type is '_'
if local.span.ctxt() == ty.span.ctxt();
if local.span.eq_ctxt(ty.span);
then {
// NOTE: Using `is_from_proc_macro` on `init` will require that it's initialized,
// this doesn't. Alternatively, `WithSearchPat` can be implemented for `Ty`

View File

@ -59,7 +59,7 @@ impl<'tcx> QuestionMark {
let Some(init) = local.init &&
local.els.is_none() &&
local.ty.is_none() &&
init.span.ctxt() == stmt.span.ctxt() &&
init.span.eq_ctxt(stmt.span) &&
let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init)
{
match if_let_or_match {

View File

@ -57,7 +57,7 @@ fn check_arm<'tcx>(
}
},
};
if outer_pat.span.ctxt() == inner_scrutinee.span.ctxt();
if outer_pat.span.eq_ctxt(inner_scrutinee.span);
// match expression must be a local binding
// match <local> { .. }
if let Some(binding_id) = path_to_local(peel_ref_operators(cx, inner_scrutinee));

View File

@ -119,7 +119,7 @@ where
// it's being passed by value.
let scrutinee = peel_hir_expr_refs(scrutinee).0;
let (scrutinee_str, _) = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app);
let scrutinee_str = if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX {
let scrutinee_str = if scrutinee.span.eq_ctxt(expr.span) && scrutinee.precedence().order() < PREC_POSTFIX {
format!("({scrutinee_str})")
} else {
scrutinee_str.into()
@ -130,7 +130,7 @@ where
if_chain! {
if !some_expr.needs_unsafe_block;
if let Some(func) = can_pass_as_func(cx, id, some_expr.expr);
if func.span.ctxt() == some_expr.expr.span.ctxt();
if func.span.eq_ctxt(some_expr.expr.span);
then {
snippet_with_applicability(cx, func.span, "..", &mut app).into_owned()
} else {

View File

@ -56,7 +56,7 @@ pub(super) fn check<'tcx>(
// lint, with note if neither arg is > 1 line and both map() and
// unwrap_or_else() have the same span
let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
let same_span = map_arg.span.ctxt() == unwrap_arg.span.ctxt();
let same_span = map_arg.span.eq_ctxt(unwrap_arg.span);
if same_span && !multiline {
let var_snippet = snippet(cx, recv.span, "..");
span_lint_and_sugg(

View File

@ -125,7 +125,7 @@ fn check(cx: &LateContext<'_>, expr: &Expr<'_>) {
if let ExprKind::Match(inner_expr_with_q, _, MatchSource::TryDesugar(_)) = &arg.kind;
if let ExprKind::Call(called, [inner_expr]) = &inner_expr_with_q.kind;
if let ExprKind::Path(QPath::LangItem(LangItem::TryTraitBranch, ..)) = &called.kind;
if expr.span.ctxt() == inner_expr.span.ctxt();
if expr.span.eq_ctxt(inner_expr.span);
let expr_ty = cx.typeck_results().expr_ty(expr);
let inner_ty = cx.typeck_results().expr_ty(inner_expr);
if expr_ty == inner_ty;

View File

@ -51,7 +51,7 @@ impl<'tcx> LateLintPass<'tcx> for NonOctalUnixPermissions {
|| (path.ident.name == sym!(set_mode)
&& cx.tcx.is_diagnostic_item(sym::FsPermissions, adt.did()));
if let ExprKind::Lit(_) = param.kind;
if param.span.ctxt() == expr.span.ctxt();
if param.span.eq_ctxt(expr.span);
then {
let Some(snip) = snippet_opt(cx, param.span) else {
@ -70,7 +70,7 @@ impl<'tcx> LateLintPass<'tcx> for NonOctalUnixPermissions {
if let Some(def_id) = cx.qpath_res(path, func.hir_id).opt_def_id();
if match_def_path(cx, def_id, &paths::PERMISSIONS_FROM_MODE);
if let ExprKind::Lit(_) = param.kind;
if param.span.ctxt() == expr.span.ctxt();
if param.span.eq_ctxt(expr.span);
if let Some(snip) = snippet_opt(cx, param.span);
if !snip.starts_with("0o");
then {

View File

@ -48,7 +48,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantAsyncBlock {
let Some(body_expr) = desugar_async_block(cx, expr) &&
let Some(expr) = desugar_await(peel_blocks(body_expr)) &&
// The await prefix must not come from a macro as its content could change in the future.
expr.span.ctxt() == body_expr.span.ctxt() &&
expr.span.eq_ctxt(body_expr.span) &&
// An async block does not have immediate side-effects from a `.await` point-of-view.
(!expr.can_have_side_effects() || desugar_async_block(cx, expr).is_some()) &&
let Some(shortened_span) = walk_span_to_context(expr.span, span.ctxt())

View File

@ -50,7 +50,7 @@ impl EarlyLintPass for DerefAddrOf {
if_chain! {
if let ExprKind::Unary(UnOp::Deref, ref deref_target) = e.kind;
if let ExprKind::AddrOf(_, ref mutability, ref addrof_target) = without_parens(deref_target).kind;
if deref_target.span.ctxt() == e.span.ctxt();
if deref_target.span.eq_ctxt(e.span);
if !addrof_target.span.from_expansion();
then {
let mut applicability = Applicability::MachineApplicable;

View File

@ -33,7 +33,7 @@ impl LateLintPass<'_> for ConfusingXorAndPow {
if !in_external_macro(cx.sess(), expr.span)
&& let ExprKind::Binary(op, left, right) = &expr.kind
&& op.node == BinOpKind::BitXor
&& left.span.ctxt() == right.span.ctxt()
&& left.span.eq_ctxt(right.span)
&& let ExprKind::Lit(lit_left) = &left.kind
&& let ExprKind::Lit(lit_right) = &right.kind
&& matches!(lit_right.node, LitKind::Int(..) | LitKind::Float(..))

View File

@ -245,7 +245,7 @@ impl<'a> PanicExpn<'a> {
return None;
};
let result = match name {
"panic" if arg.span.ctxt() == expr.span.ctxt() => Self::Empty,
"panic" if arg.span.eq_ctxt(expr.span) => Self::Empty,
"panic" | "panic_str" => Self::Str(arg),
"panic_display" | "panic_cold_display" => {
let ExprKind::AddrOf(_, _, e) = &arg.kind else {

View File

@ -0,0 +1,13 @@
// Test the `rustc::span_use_eq_ctxt` internal lint
// compile-flags: -Z unstable-options
#![feature(rustc_private)]
#![deny(rustc::span_use_eq_ctxt)]
#![crate_type = "lib"]
extern crate rustc_span;
use rustc_span::Span;
pub fn f(s: Span, t: Span) -> bool {
s.ctxt() == t.ctxt() //~ ERROR use `.eq_ctxt()` instead of `.ctxt() == .ctxt()`
}

View File

@ -0,0 +1,14 @@
error: use `.eq_ctxt()` instead of `.ctxt() == .ctxt()`
--> $DIR/span_use_eq_ctxt.rs:12:5
|
LL | s.ctxt() == t.ctxt()
| ^^^^^^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/span_use_eq_ctxt.rs:5:9
|
LL | #![deny(rustc::span_use_eq_ctxt)]
| ^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error