fix legacy_numeric_constants suggestion when call is wrapped in parens

[ComputerDruid]

Fixes #15008

changelog: [legacy_numeric_constants]: fix suggestion when call is inside parentheses like (i32::max_value())

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

The fix gives good results, but in presence of i32::max_value(), it posts the lint on the i32::max_value node (not on the call itself), and underlines only i32::max_value and not i32::max_value().

Also, the usage of span_lint_hir_and_then() is unjustified, as it always posts it on expr which is where span_lint_and_then() would post it by default. par_expr should be used, both for the span and the node to post on.

Rather than looking for the parent in the case of a function call, it is more efficient to look for a function call then to check if a legacy {min,max}_value() function is used.

Could you try this alternative for check_expr() and tell me what you think of it?

    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx>) {
        // `std::<integer>::<CONST>` check
        let (sugg, msg) = if let ExprKind::Path(qpath) = &expr.kind
            && let QPath::Resolved(None, path) = qpath
            && let Some(def_id) = path.res.opt_def_id()
            && is_numeric_const(cx, def_id)
            && let [.., mod_name, name] = &*cx.get_def_path(def_id)
            // Skip linting if this usage looks identical to the associated constant,
            // since this would only require removing a `use` import (which is already linted).
            && !is_numeric_const_path_canonical(path, [*mod_name, *name])
        {
            (
                vec![(expr.span, format!("{mod_name}::{name}"))],
                "usage of a legacy numeric constant",
            )
        // `<integer>::xxx_value` check
        } else if let ExprKind::Call(func, []) = &expr.kind
            && let ExprKind::Path(qpath) = &func.kind
            && let QPath::TypeRelative(ty, last_segment) = qpath
            && let Some(def_id) = cx.qpath_res(qpath, func.hir_id).opt_def_id()
            && is_integer_method(cx, def_id)
        {
            let mut sugg = vec![
                // Replace the function name up to the end by the constant name
                (
                    last_segment.ident.span.to(expr.span.shrink_to_hi()),
                    last_segment.ident.name.as_str()[..=2].to_ascii_uppercase(),
                ),
            ];
            let before_span = expr.span.shrink_to_lo().until(ty.span);
            if !before_span.is_empty() {
                // Remove everything before the type name
                sugg.push((before_span, String::new()));
            }
            // Use `::` between the type name and the constant
            let between_span = ty.span.shrink_to_hi().until(last_segment.ident.span);
            if !between_span.check_source_text(cx, |s| s == "::") {
                sugg.push((between_span, String::from("::")));
            }
            (sugg, "usage of a legacy numeric method")
        } else {
            return;
        };

        if !expr.span.in_external_macro(cx.sess().source_map())
            && self.msrv.meets(cx, msrvs::NUMERIC_ASSOCIATED_CONSTANTS)
            && !is_from_proc_macro(cx, expr)
        {
            span_lint_and_then(cx, LEGACY_NUMERIC_CONSTANTS, expr.span, msg, |diag| {
                diag.multipart_suggestion_verbose(
                    "use the associated constant instead",
                    sugg,
                    Applicability::MaybeIncorrect,
                );
            });
        }
    }

Also, could you add those two extra tests?

    ((i32::max_value)());
    //~^ ERROR: usage of a legacy numeric method
    //~| HELP: use the associated constant instead
    type Ω = i32;
    Ω::max_value();
    //~^ ERROR: usage of a legacy numeric method
    //~| HELP: use the associated constant instead

[ComputerDruid]

Thanks for taking a look. I probably won't be able to get back to this until next week, but I should be able to get back to this then

[github-actions]

Lintcheck changes for 9457d64

Lint	Added	Removed	Changed
clippy::legacy_numeric_constants	0	0	24

This comment will be updated if you push new changes

[ComputerDruid]

The fix gives good results, but in presence of i32::max_value(), it posts the lint on the i32::max_value node (not on the call itself), and underlines only i32::max_value and not i32::max_value().

Underlining the whole call is actually a simplification, so folded that in to the main commit.

Also, could you add those two extra tests?

This is now commit 2.

Also, the usage of span_lint_hir_and_then() is unjustified, as it always posts it on expr which is where span_lint_and_then() would post it by default. par_expr should be used, both for the span and the node to post on.

Switching to span_lint_and_then is now commit 3. (although it doesn't post it on the right node until commit 4)

Rather than looking for the parent in the case of a function call, it is more efficient to look for a function call then to check if a legacy {min,max}_value() function is used.

Could you try this alternative for check_expr() and tell me what you think of it?

I like it! I split it into:

• commit 4: "refactor legacy_numeric_constants to check calls instead of paths", which also results in posting the lint on the call instead of the path
• commit 5: "refactor legacy_numeric_constants to use multipart suggestion", which gets rid of the need to use the source snippet.

@rustbot ready

[samueltardieu]

Thanks. I'm still bugged a little by the fact that we talk about a "legacy numeric method" instead of "legacy numeric associated function", but this is not introduced by this PR, and might even be easier to read despite being technically incorrect.