search

denoland / deno_lint

Blazing fast linter for JavaScript and TypeScript written in Rust
https://lint.deno.land/
MIT License
1.53k stars 172 forks source link

Implement autofix #1181

Closed yacinehmito closed 7 months ago

yacinehmito commented 1 year ago

This PR implements autofix for a small selection of rules.

Related PR for Deno CLI: https://github.com/denoland/deno/pull/19872

Closes #1013.

CLAassistant commented 1 year ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

yacinehmito commented 1 year ago

This is very much WIP. At the moment I only added the ability to test for the output of a fix. I am still figuring out how to do this.

@bartlomieju I see that ESLint implements fixes by letting rules provide snippets of text to insert in the source file at the location of an AST node, while Rome does so by lettings rules provide a description of AST mutations to perform. I am still unsure which path to take. The former (operate on text) seems easier to implement but fraught with correctness issues, while the later (operate on the AST) is the opposite. Any advice?

magurotuna commented 1 year ago

I see that ESLint implements fixes by letting rules provide snippets of text to insert in the source file at the location of an AST node, while Rome does so by lettings rules provide a description of AST mutations to perform. I am still unsure which path to take. The former (operate on text) seems easier to implement but fraught with correctness issues, while the later (operate on the AST) is the opposite. Any advice?

That's very interesting, and I agree that text transformation is easier to implement but less robust in some cases, and AST-based way is the opposite. I would spend some time trying to figure out if the latter is something doable

bartlomieju commented 1 year ago

I see that ESLint implements fixes by letting rules provide snippets of text to insert in the source file at the location of an AST node, while Rome does so by lettings rules provide a description of AST mutations to perform. I am still unsure which path to take. The former (operate on text) seems easier to implement but fraught with correctness issues, while the later (operate on the AST) is the opposite. Any advice?

That's very interesting, and I agree that text transformation is easier to implement but less robust in some cases, and AST-based way is the opposite. I would spend some time trying to figure out if the latter is something doable

I agree with @magurotuna here, but it's hard to decide right now. The latter is certainly doable - SWC has infrastructure to modify AST nodes and then write them back as string. The problem I see here is that after fixing errors in certain rule, we would have to re-parse and re-lint all the files that were fixed. Not sure if that's something that can be avoided but we should keep that in mind.

@yacinehmito thanks for starting the PR, I will give it some thought over the next week and come back with some suggestions on how to proceed.

tlgimenes commented 11 months ago

This would be really nice. It would allow me to automatically run autofix in a pre-commit hook