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(linter): declare_oxc_lint proc_macro #70

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

YoniFeng
Copy link
Contributor

@YoniFeng YoniFeng commented Feb 27, 2023

Initial implementation for #60

TODO/open questions:

  1. What kind of tests did you have in mind? End to end tests for the proc macro, or something else? (Most misuses of the proc macro will result in a compile-time error/warning)
  2. This version of the proc macro declares the empty struct (i.e. gets passed pub struct my_struct). It doesn't allow for customization of the struct, like you've done with the allow_empty_catch field in the "no_empty" rule.
    It's probably possible for the proc macro to parse an arbitrary struct, but it sounds overly complex (it might not be, though).
    What do you think about pre-defining the struct, then using the macro.
pub struct SomeRule {
      field1: bool,
      field2: u32,
      ...
}

declare_oxc_lint!(
   /// ### What it does
    /// Checks for usage of the `debugger` statement
    ///
    /// ### Why is this bad?
    /// `debugger` statements do not affect functionality when a debugger isn't attached.
    /// They're most commonly an accidental debugging leftover.
    ///
    ///
    /// ### Example
    /// ```javascript
    /// const data = await getData();
    /// const result = complexCalculation(data);
    /// debugger;
    /// ```
    SomeRule
);

I may have misunderstood what you want the macro to do/what role you want it to play in creating rules.

@YoniFeng YoniFeng marked this pull request as draft February 27, 2023 10:56
@Boshen
Copy link
Member

Boshen commented Feb 27, 2023

This is amazing! Thank you for making the time for this. This is exactly what we need.

What kind of tests did you have in mind

I think some integrations test for getting the metadata is more than enough, because macros are just implementation details. So probably just a dummy rule where we call and check rule.documentation(). We'll need this infrastructure when we add more metadata.

What do you think about pre-defining the struct, then using the macro.

I think this is fine here, because it will expand into the equivalent code, it doesn't really better the order we put it.

Of course we can do more macro magic to make it super nice, but I'm eager to try out your implementation. We can iterate and make changes later.

It doesn't allow for customization of the struct, like you've done with the allow_empty_catch field in the "no_empty" rule.

Struct with fields is super ergonomic here, are we able to make some kind of round trip to make this happen? Clippy has round trip: impl_lint_pass!(AlmostCompleteRange => [ALMOST_COMPLETE_RANGE]);

Edit:

If I understand correctly, pre-defining the struct is the solution for "customization of the struct" then I'm all for it!

@Boshen Boshen self-requested a review February 27, 2023 12:34
@YoniFeng
Copy link
Contributor Author

YoniFeng commented Feb 27, 2023

If I understand correctly, pre-defining the struct is the solution for "customization of the struct" then I'm all for it!

Yeah, cause then its identifier can just be referenced.
The downside is the the Derive attributes need to be added manually..

I'm not sure how useful the macro is currently (well, I guess it might be more future-oriented/useful when there's further metadata and actual usage of it).
RULE_NAME seems like a prime candidate to be moved to this metadata but it's used statically by the Rules enum so I didn't do it yet (I think it should be easy, but I haven't taken a good look).
Given ESLint rules follow a "word-hyphen-word" pattern, the name can be auto-generated inserting hyphens based on capitalization (i.e. "MyRuleName" -> "my-rule-name")

@YoniFeng
Copy link
Contributor Author

Just noting - I'm not getting the latest lint errors (https://github.com/Boshen/oxc/actions/runs/4284464848/jobs/7461457381) when I run cargo lint -- --deny warnings locally.

@Boshen
Copy link
Member

Boshen commented Feb 28, 2023

On rule name: it can be derived from the struct name, let's do it in the next PR.

@Boshen Boshen marked this pull request as ready for review February 28, 2023 03:37
crates/oxc_macros/src/lib.rs Outdated Show resolved Hide resolved
crates/oxc_macros/src/lib.rs Show resolved Hide resolved
@Boshen
Copy link
Member

Boshen commented Feb 28, 2023

Let's follow clippy from the CI for now, I'll check later and see if CI is actually broken.

@YoniFeng
Copy link
Contributor Author

Let's follow clippy from the CI for now, I'll check later and see if CI is actually broken.

Don't bother, I'm suddenly getting the clippy errors locally. CI is probably fine.
Maybe rust-lang/rust-clippy#5094 is relevant (haven't properly read through it, don't intend to either for now).

@Boshen
Copy link
Member

Boshen commented Feb 28, 2023

Squash commits and we'll be good to go 😄

@YoniFeng YoniFeng changed the title Add macro for declaring lint rules Feat(linter): declare_oxc_lint proc_macro Feb 28, 2023
@Boshen Boshen merged commit f859ee0 into oxc-project:main Mar 1, 2023
@Boshen
Copy link
Member

Boshen commented Mar 1, 2023

Thank you! This is one step closer to be like clippy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants