rust-marker/marker

[Feat]: Make it possible to use `AstPathSegment` and `Ident` as an emission node

Opened this issue · 1 comments

Summary

I tried to update my ToStringOnCowStr lint to 0.3.0, but I couldn't make it work seamlessly as I wanted.

Here is the code that I have and I want to focus on:

ctx.emit_lint(
    TO_STRING_ON_COW_STR,
    method,
    "replace this with .into_owned()",
)
.span(method.method().ident().span());

And here is the full code for more context:

Details
use marker_api::prelude::*;
use marker_api::{LintPass, LintPassInfo, LintPassInfoBuilder};

#[derive(Default)]
struct ToStringOnCowStrPass {}
marker_api::export_lint_pass!(ToStringOnCowStrPass);

marker_api::declare_lint! {
    /// # What it does
    /// This lints detects the usage of `.to_string()` on a `Cow<str>`.
    ///
    /// # Example
    /// ```rs
    /// PathBuf::from("foo").to_string_lossy().to_string()
    /// ```
    ///
    /// Use instead:
    /// ```rs
    /// PathBuf::from("foo").to_string_lossy().into_owned()
    /// ```
    ///
    /// If you intended to clone the `Cow`, then use `.clone()` or `.to_owned()`.
    TO_STRING_ON_COW_STR,
    Deny,
}

impl LintPass for ToStringOnCowStrPass {
    fn info(&self) -> LintPassInfo {
        LintPassInfoBuilder::new(Box::new([TO_STRING_ON_COW_STR])).build()
    }

    // Comment here
    fn check_expr<'ast>(
        // comment there
        &mut self,
        ctx: &'ast MarkerContext<'ast>,
        expr: ExprKind<'ast>,
    ) {
        let ExprKind::Method(method) = expr else {
            return;
        };

        if method.method().ident().name() != "to_string" {
            return;
        }

        let sem::TyKind::Adt(reciever) = method.receiver().ty() else {
            return;
        };

        if !ctx
            .resolve_ty_ids("std::borrow::Cow")
            .contains(&reciever.def_id())
        {
            return;
        }

        let [sem::GenericArgKind::Ty(str)] = reciever.generics().args() else {
            return;
        };

        if !matches!(str, sem::TyKind::Text(str) if str.is_str()) {
            return;
        }

        ctx.emit_lint(
            TO_STRING_ON_COW_STR,
            method,
            "replace this with .into_owned()",
        )
        .span(method.method().ident().span());
    }
}

I couldn't just write this:

ctx.emit_lint(
    TO_STRING_ON_COW_STR,
    method.method(),
    "replace this with .into_owned()",
)

or this:

ctx.emit_lint(
    TO_STRING_ON_COW_STR,
    method.method().ident(),
    "replace this with .into_owned()",
)

because neither AstPathSegment nor Ident implement EmissionNode. If I just use method then the span used in the error message is this:

6 |     let val = cow.to_string();
  |               ^^^^^^^^^^^^^^^

But what I want it to be is this:

6 |     let val = cow.to_string();
  |                   ^^^^^^^^^

Intuitively I wanted to use the method name with the generics included (AstPathSegment) or at least the method's identifier as the span, but I couldn't, so I had to manually override the span.

Just checked and the rustc equivalent of AstPathSegment has a HirId, meaning that it can be used for lint emission. For Ident's might be more complicated. We could just always take the NodeId of the parent. That would mean that we had to always store a NodeId in Idents where I'm not sure if that's the best.

For #242 I wanted to add new IDs and probably a generic ExprPartId or NodePartId for these kinds of nodes. This should make it easy to implement EmissionNode for AstPathSegment