Discussion: ways of implementing autofix and issues

[RDambrosio016]

This issue is meant for housing any discussions and implementations of autofix. I believe we should implement autofix relatively soon, this way we won't have a huge burden to add autofix to every rule once there are way more rules. As far as i see it, there are two ways of doing autofix:

• Purely text based (eslint)
• AST transformation based (rome)

Purely text based has the issue of often not being very flexible (i personally disagree), and AST transformation based has the issue of not being flexible for minor text based fixes and needing AST serialization. Collectively autofix has fundamental issues which must be overcome:

• All/Some rules need to be rerun after, i propose we add the ability to mark rules as fix-unsafe which will rerun only them, instead of needing all rules to rerun, which is expensive. This is absolutely required for stylistic rules, a fix cannot always conform to style and it should not either ways.

• How do we choose what rule's fixes take precedence, if a rule wants to delete a node but another wants to change something in the node, which one do we trust?

Im not an expert in autofix so i would love some other ideas. RSLint's syntax again shines in this because it allows us to:

• Incrementally reparse only the changed bits of code extremely fast and efficiently (linting is 60%+ parsing in RSLint's case), i am working on incremental reparsing but its not a top priority, but it will be widely used for vsc, file watching, and autofix.
• Make changes on individual tokens or nodes and delegate them to a rewriter which converts them to text edits

I for one think text based is the way to go, it allows for a wider range of edits, it provides a familiar UI to people who have used ESLint, and rslint's immutable tree makes it a little bit more difficult to apply AST transformations, moreover, RSLint does not have a way to make new nodes without the parser and serialize them.

As for implementation, i believe we should add a field to RuleCtx which is fixer: Option<Fixer>, rules can make a new fixer (similar to ESLint) and add it, then the linter can simply collect the fixers and apply them. I also thought about making a new provided method on RuleCtx but ended up scrapping it because rules will 100% want to know about context when fixing, so making it a separate method would mean that some of the linting logic would have to be repeated, this is very very ugly and i would rather not do that.

I for one am not sure how to get over the issue of "which rule do we trust when it comes to a fix, do we trust the one who wants to change the node or the one who wants to delete it?". I also believe we should not try applying fixes if the parser returned errors, that can cause an absolute ton of confusing errors which i would rather not meddle in, error tolerance is great for linting but as soon as you try changing the AST produced (which is potentially semantically wrong) you end up with confusion.

[Stupremee]

Is there a way to create a SyntaxNode without actually parsing something or making a whole tree?
My first idea was that every rule that fixes something, will create a new SyntaxNode and replace another node in the tree with it probably using the replace_with method in rowan. But then there's still the problem to somehow synchronize the fixes. I also don't know how complex or expensive replace_with is.

[RDambrosio016]

Not without manually adding tokens, no, but replace_with is more or less O(1)

[RDambrosio016]

I think we can get away with an amazing UI with a fixer, we could leverage syntax nodes and the new Span trait to allow things like fixer.replace(node, other_node) or fixer.wrap(node, T![')']) or fixer.delete(span). I dont really believe we need AST transformations to do this.

[Stupremee]

That would work too.

[RDambrosio016]

I looked at eslint's approach, it seems that they just do not apply fixes for a rule if the fix collides with another, i think this is fine for now, we should maybe issue an internal error if the fix spans overlap.

[Stupremee]

What if we re run one of the fixes with the new fixed content if both fixes collide?

Edit: that's probably going to be harder to implement.

[RDambrosio016]

That would be arbitrary selection of which rule is "right", which i would rather stay away from.

Edit: misunderstood what you meant, but It could also be exponentially complex and could likely throw the linter into tons of reruns which would be very expensive

[Stupremee]

Yea that's not ideal.

[RDambrosio016]

Eslint's approach seems to work well enough, they collect fixes and apply them sequentially, then if the next fix's range overlaps they don't apply the fix and just skip to the next iteration. I think this is good and we should try doing this. I would like to implement autofix for v0.2 once #16 has landed, since incremental reparsing will be a big part of the fix iterations.

[RDambrosio016]

I would also like to eventually add SSR (structural search and replace), SSR is a tool for search and replace which operates directly on the syntax tree, for example:

{ get $name:name() $body } -> function $name () $body

it could also be extended to match specific nodes like bin_expr, expr, etc. The premise of how it works is really simple and rslint's syntax makes it really easy (see rust analyzer's implementation). there are a few distinct steps:

1: template splitting, producing { get $name:name() $body } and function $name () $body

2: lexing, the templates are lexed to identify the patterns in the text

3: replacement, { get __rslint_match_name_0 () {__rslint_match_node_0} } and function __rslint_name_0 () __rslint_node_0

4: parsing, the template and the replacement are parsed with rslint_parser, they must parse or the process is exited instantly.

5: tree walking, we take the tree of the template and the tree of the file, for each descendant in the file root node, if the descendant has the same node type as the template we enter a separate walking function which will sequentially check each child in the tree and the template, if any child does not match 1 - 1 then the node is not a match, once it encounters something like __rslint_match_name_0 it will check for a name node, if there isnt one then the node isnt a match, if there is then it is bound to the 0th replacement binder. if the pattern has no hint then it will become {__rslint_match_node_0}, this is to allow for binding both block statements and expressions, there is no real way to bind ANY node because there isnt a piece of text which will parse in the place of any node.

6: replacement, we take the binders bound in the fifth step and use rowan's replace_with function to make a new tree with the replacement but replaced with each binder.

This could be very useful for a number of things:

• We could expose it to the user as a tool through rslint ssr
• We could use it for autofix and compile the templates and replacements at compile time using a proc macro
• We could use it for matching patterns in rules and compile the templates at compile time like above

[RDambrosio016]

Closing as it is implemented in #45