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

Add project-specific lints with ast-grep #5637

Merged
merged 4 commits into from
Aug 1, 2023
Merged

Conversation

wbinnssmith
Copy link
Member

ast-grep[0] is a tool that uses tree-sitter[1] and a pattern language to match against code using its ast. It implements code transformation, querying, and linting.

Since clippy isn't extensible for project-specific lints, this adds a first ast-grep lint disallowing context as a variable name. Currently it's set to warning as usage is addressed.

To run, install ast-grep, then run ast-grep scan. Example output:

warning[no-context]: Don't name variables `context`.
    ┌─ ./crates/turbopack-ecmascript-hmr-protocol/src/lib.rs:132:9
    │
132 │     pub context: &'a str,
    │     ----^^^^^^^---------
    │
    = Use a more specific name, such as chunking_context, asset_context, etc.

[0] https://ast-grep.github.io
[1] https://tree-sitter.github.io/tree-sitter/

ast-grep[0] is a tool that uses tree-sitter[1] and a pattern language to match against code using its ast. It implements code transformation, querying, and linting.

Since clippy isn't extensible for project-specific lints, this adds a first ast-grep lint disallowing `context` as a variable name. Currently it's set to warning as usage is addressed.

To run, install ast-grep, then run `ast-grep scan`. Example output:

```
warning[no-context]: Don't name variables `context`.
    ┌─ ./crates/turbopack-ecmascript-hmr-protocol/src/lib.rs:132:9
    │
132 │     pub context: &'a str,
    │     ----^^^^^^^---------
    │
    = Use a more specific name, such as chunking_context, asset_context, etc.
```

[0] https://ast-grep.github.io
[1] https://tree-sitter.github.io/tree-sitter/
@vercel
Copy link

vercel bot commented Jul 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-native-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback Jul 31, 2023 5:22pm
examples-tailwind-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback Jul 31, 2023 5:22pm
9 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Jul 31, 2023 5:22pm
examples-cra-web ⬜️ Ignored (Inspect) Visit Preview Jul 31, 2023 5:22pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Jul 31, 2023 5:22pm
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview Jul 31, 2023 5:22pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Jul 31, 2023 5:22pm
examples-nonmonorepo ⬜️ Ignored (Inspect) Visit Preview Jul 31, 2023 5:22pm
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Jul 31, 2023 5:22pm
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Jul 31, 2023 5:22pm
turbo-site ⬜️ Ignored (Inspect) Visit Preview Jul 31, 2023 5:22pm

@github-actions
Copy link
Contributor

github-actions bot commented Jul 28, 2023

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

@NicholasLYang
Copy link
Contributor

I've been following ast-grep so it's neat that we're using it here. Are we hooking this up to CI?

@wbinnssmith
Copy link
Member Author

Are we hooking this up to CI?

Yeah, I'll add that here.

@github-actions
Copy link
Contributor

✅ This change can build next-swc

@github-actions
Copy link
Contributor

Linux Benchmark for 746c94f

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 5426.50µs ± 18.65µs 5430.09µs ± 15.16µs +0.07%
bench_hmr_to_eval/Turbopack CSR/1000 modules 5098.97µs ± 24.56µs 5086.03µs ± 30.29µs -0.25%
bench_startup/Turbopack CSR/1000 modules 987.35ms ± 1.52ms 988.35ms ± 0.81ms +0.10%

@github-actions
Copy link
Contributor

MacOS Benchmark for 746c94f

Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 27.28ms ± 0.10ms 24.60ms ± 0.81ms -9.82% -3.19%
bench_startup/Turbopack CSR/1000 modules 9874.06ms ± 1649.94ms 4244.78ms ± 89.31ms -57.01% -32.72%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 27.28ms ± 0.10ms 24.60ms ± 0.81ms -9.82% -3.19%
bench_hmr_to_eval/Turbopack CSR/1000 modules 25.29ms ± 0.52ms 24.62ms ± 1.20ms -2.62%
bench_startup/Turbopack CSR/1000 modules 9874.06ms ± 1649.94ms 4244.78ms ± 89.31ms -57.01% -32.72%

Copy link
Contributor

@NicholasLYang NicholasLYang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. We're intentionally having the ast-grep output be warnings and not errors, right?

@wbinnssmith
Copy link
Member Author

It moves to error + ignore allowlist in #5640

@wbinnssmith wbinnssmith merged commit f5c848b into main Aug 1, 2023
@wbinnssmith wbinnssmith deleted the wbinnssmith/ast-grep-lint branch August 1, 2023 18:52
@HerringtonDarkholme
Copy link

Hi Vercel team. It is my honor that turbo is using ast-grep for project-specific linting. I really appreciate your trust!

By the way, ast-grep supported a new argument --format in sg scan. This will instruct the CLI to output GitHub Action compatible messages and shows linting issues in the pull request.

This page has a screenshot of the option.
https://ast-grep.github.io/guide/tooling-overview.html#use-ast-grep-in-github-action

wbinnssmith added a commit that referenced this pull request Jun 3, 2024
This uses the official ast-grep GitHub Action [0] instead of installing it from npm in the clippy job.

Thanks for the suggestion from @HerringtonDarkholme!

[0] https://github.com/ast-grep/action
[1] #5637 (comment)
wbinnssmith added a commit that referenced this pull request Jun 11, 2024
This uses the official ast-grep GitHub Action [0] instead of installing it from npm in the clippy job.

Thanks for the suggestion from @HerringtonDarkholme!

[0] https://github.com/ast-grep/action
[1] #5637 (comment)
wbinnssmith added a commit that referenced this pull request Jun 11, 2024
This uses the official ast-grep GitHub Action [0] instead of installing it from npm in the clippy job.

Thanks for the suggestion from @HerringtonDarkholme!

[0] https://github.com/ast-grep/action
[1] #5637 (comment)
wbinnssmith added a commit that referenced this pull request Jun 11, 2024
This uses the official ast-grep GitHub Action [0] instead of installing it from npm in the clippy job.

Thanks for the suggestion from @HerringtonDarkholme!

[0] https://github.com/ast-grep/action
[1] #5637 (comment)
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
ast-grep[0] is a tool that uses tree-sitter[1] and a pattern language to
match against code using its ast. It implements code transformation,
querying, and linting.

Since clippy isn't extensible for project-specific lints, this adds a
first ast-grep lint disallowing `context` as a variable name. Currently
it's set to warning as usage is addressed.

To run, install ast-grep, then run `ast-grep scan`. Example output:

```
warning[no-context]: Don't name variables `context`.
    ┌─ ./crates/turbopack-ecmascript-hmr-protocol/src/lib.rs:132:9
    │
132 │     pub context: &'a str,
    │     ----^^^^^^^---------
    │
    = Use a more specific name, such as chunking_context, asset_context, etc.
```

[0] https://ast-grep.github.io
[1] https://tree-sitter.github.io/tree-sitter/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants