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.

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

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

Open
Veetaha opened this issue Oct 7, 2023 · 1 comment
Labels
A-api Area: Stable API C-enhancement Category: New feature or request
Milestone

Comments

@Veetaha
Copy link
Contributor

Veetaha commented Oct 7, 2023

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.

@Veetaha Veetaha added C-enhancement Category: New feature or request S-needs-triage Status: This issue needs triage labels Oct 7, 2023
@xFrednet
Copy link
Member

xFrednet commented Oct 7, 2023

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

@xFrednet xFrednet added A-api Area: Stable API and removed S-needs-triage Status: This issue needs triage labels Oct 7, 2023
@xFrednet xFrednet added this to the v0.4.0 milestone Oct 7, 2023
@xFrednet xFrednet modified the milestones: v0.4.0, v0.5.0 Nov 16, 2023
@xFrednet xFrednet modified the milestones: v0.5.0, v0.6.0 Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-api Area: Stable API C-enhancement Category: New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants