LSP: action to remove redundant tuple wrapper from case expression's subject

[giacomocavalieri]

The compiler warns about unnecessary wrapping of tuples in a case's subject, it would be nice if an action could automatically fix that:

case #(wibble, wobble) {
//   ~~~~~~~~~~~~~~~~~ Action to remove redundant tuple
}

And an action to batch remove it from all cases in case there's more than one, much like with unused imports

[lpil]

Good idea!

[nicklimmm]

This looks interesting and might be simple enough for newcomers, I'd like to work on this!

[lpil]

Awesome! Though I think it might be quite hard to write the code that edits the existing expression to remove the tuple. We don't have any mechanism for that today.

[nicklimmm]

I'm thinking of using the Visitor pattern to visit the case expressions (need to implement this before continuing). I believe the pattern could be reused for future use.

An example: syn crate for Rust syntax tree - syn::visit and syn::visit_mut

A general idea to solve this issue:

1. Visit each TypedExpr::Case and UntypedExpr::Case
2. For each subject, if it is a tuple -> add lsp_types::TextEdit with the corresponding range and new text without #()

I've found something similar to walking the AST in UntypedExprFolder, I'm not sure why it exists though.

[nicklimmm]

I'm currently writing a proof of concept and looks promising so far. Will push a draft PR soon.

[lpil]

The untyped AST isn't available here, and if it were it's an AST not a CST so a parse->modify-print loop would result in formatting and comments to all be discarded.

[nicklimmm]

The untyped AST isn't available here, and if it were it's an AST not a CST so a parse->modify-print loop would result in formatting and comments to all be discarded.

Does this mean that we shouldn't walk on TypedModule or TypedExpr to identify the case expression nodes of concern?

[lpil]

You can identify them but there's not an easy to way to perform the edit after that.

[nicklimmm]

You can identify them but there's not an easy to way to perform the edit after that.

I think I get what you mean now, I'm guessing that we should modify the (typed) AST -> prettified string output -> final text edits.

But from what I've observed, the prettifier is only implemented for untyped AST, while we only have typed AST in this case. 🤔

The AST modification looks something like this:

flowchart LR
    before --> |collapse| after
    subgraph before
    C[TypedExpr::Case] --> S{subjects}
    S --> T1
    S --> T2
    T1[TypedExpr::Tuple]
    T1 --> E1[TypedExpr]
    T1 --> E2[TypedExpr]
    T2[TypedExpr::Tuple]
    T2 --> E3[TypedExpr]
    T2 --> E4[TypedExpr]
    end

    subgraph after
    C2[TypedExpr::Case] --> S2{subjects}
    S2 --> E21[TypedExpr]
    S2 --> E22[TypedExpr]
    S2 --> E23[TypedExpr]
    S2 --> E24[TypedExpr]
    end

[lpil]

The formatter modifies the formatting of the code. We don't want to change any code beyond removing the tuple so it's not possible to implement it by editing and printing an AST. If we had a CST we could modify and print that, but we don't have a CST or a parser for one yet.

[nicklimmm]

Should we implement CSTs before tackling this issue (and possibly many more LSP-related ones)? Will definitely take a lot of effort to build the infrastructure.

I've found out that rust-analyzer uses rowan for their lossless syntax trees and ungrammar to define their CST structure (which is then used for codegen the types and trait impls to Rust code).

[lpil]

Whether it's worthwhile to implement CSTs for this isn't clear to me. It would be a very large amount of work and it's only one of the options. I thing it is likely that something else may be more appropriate. Something more lightweight which outputs patches to the text file.

Rather I was explaining why printing an AST wouldn't work, a CST would be required.

[nicklimmm]

I'll try to explore the available options and share some insights later on.

[lpil]

Just had a thought. We can always delete the first 2 bytes from the start and last byte from the end as we dont need to remove the commas. No need to know anything but the code span of each tuple!

[giacomocavalieri]

Mmh would that work even if I wrote the code like this? # ( 1,2 ) (notice the space between the hashtag and the open parentheses) because technically that's a valid tuple and I guess we can't assume the code we're analysing is well formatted

[nicklimmm]

Mmh would that work even if I wrote the code like this? # ( 1,2 ) (notice the space between the hashtag and the open parentheses) because technically that's a valid tuple and I guess we can't assume the code we're analysing is well formatted

We can delete from the start span of the tuple to the start span of the first element (removing the opening), and from the end span of the last item to the end span of the tuple (removing the closing).

We need to handle comments in between those to prevent any deletion.

[nicklimmm]

After some thought, I'm thinking of a simpler method to retain comments and newlines: delete #, (, a trailing comma for the final tuple element (if applicable), and ).

Determining both # and ) is trivial, which is just at the start and the end of the tuple span, respectively.

To get the correct ( even with comments after the #, we scan for the first ( that doesn't belong to a comment. Similar idea for the trailing comma, but we scan from right to left.

Example

Even though this looks cursed, but this is valid:

// Essentially: `case #(1) { a -> 0 }`
case #
  
// ( <- should not delete this one
(
// (
1
// ,
,
// , <- should not delete this one
)

{
    a -> 0
}

The result after the change should look like this:

// Essentially: `case 1 { a -> 0 }`
case 
  
// ( <- should not delete this one

// (
1
// ,

// , <- should not delete this one


{
    a -> 0
}

Could there be any edge case that I miss?

[lpil]

Ah! I thought #( was one token. My bad! I forgot about trailing commas too.

[nicklimmm]

Seems like the method of deleting each of #, (, and ) works. The (linked) PR is up.