When you 'clone call' in javascript it results in a syntax error

[jaresty]

Issue: Clone Call in TypeScript Adds Spaces Causing Syntax Errors

Description:

When using the "clone call" feature in TypeScript, the new call is separated from the old call with spaces. This behavior results in a syntax error, as TypeScript does not permit multiple function calls separated by spaces without proper delimiters. Consequently, users must manually intervene to correct this by adding a line break or a semicolon after cloning the call.

Example:

Consider the following TypeScript code:

function example() {
  myFunction();
  myFunction(); // Cloned call
}

Upon using "clone call," the result is:

function example() {
  myFunction();
  myFunction() myFunction(); // Syntax error
}

This occurs because the cloned call is appended with spaces, causing a syntax error.

Expected Behavior:

The expected behavior when using "clone call" is either to:

1. Insert a line break after the cloned call:

function example() {
  myFunction();
  myFunction();
  myFunction(); // Cloned call with line break
}

3. Append a semicolon after the cloned call:

function example() {
  myFunction();
  myFunction();; // Cloned call with semicolon
}

Impact:

This issue frequently arises when editing files with multiple function calls, such as in vitest files where each spec is its own call. The need for manual correction after each "clone call" operation disrupts workflow and reduces productivity.

Proposed Solution:

To address this issue, modify the "clone call" functionality to automatically insert a line break or a semicolon after the cloned call. While adding semicolons may be more complex to implement, it could provide a more robust solution.

Additional Notes:

This issue requires careful consideration as multiple function calls on the same line are possible, and the chosen solution should not break valid code patterns.

[phillco]

Should be fixed soon, although out of curiosity, is there a reason you were using "clone call" instead of "clone state"? Clone state would be more versatile since you can use it for all statements, but wondering if I missed anything.

One nuance is if you had code like this:

a = myFunction();

clone call gives myFunction(); (but not take call), clone state would give a = myFunction();, so conceivably it would be useful if you didn't want the assignment, but I think in most cases, you either would or there wouldn't be much of a difference between them. As state is more broadly useful I would think you would want the muscle memory for it.

[phillco]

Actually we are going to keep that PR open a bit longer because it's unclear if we should change call to do this. Can you elaborate why you are not using clone state?

[jaresty]

I have been using clone call because, particularly in typescript tests it is very common to have a long callback when working with BDD style tests so the most obvious distinct syntax object to refer to is the call. I often use this using a token from inside the spec where the call refers to the whole spec but the state would refer to just the statement on the line where the token is, so I would have to count the nesting and use "grand" to get the call I want to clone. Fwiw, a workaround I have found is to use "clone state call ". Generally, however, I have appreciated the way that cursorless usually is opinionated about syntax and does not let you clone things in a way that would be syntactically incorrect (consider how cloning items appends commas and such, rather than just cloning the item itself, or how cloning functions in Python also grabs the annotations before the function). I considered this to be a similar situation so I raised the issue here.

[phillco]

Could you give a code example of those so I can see it a bit more clearly?

Generally, however, I have appreciated the way that cursorless usually is opinionated about syntax

Right, my preference would be for this to actually raise an error rather than insert space as the delimiter (which is always syntactically wrong I think?), and rather than assuming that you want a statement, as there are different situations where calls could be cloned that require different delimiters (for example, inside of a list or dictionary, where ;\n is also wrong) and we unfortunately don't have the ability to change the insertion delimiter contextually.

But I really need to see the example to see if there's not a simpler way to achieve that.

[jaresty]

One question that occurs to me-is there a legitimate use case for cloning a call that does not start a new statement? I couldn't think of one, but I could be mistaken.

[jaresty]

Example Vitest Test
Here is a simple example of a unit test using Vitest, a fast unit testing framework:

import { describe, it, expect } from 'vitest';

// Function to be tested
function add(a, b) {
    return a + b;
}

// Test suite
describe('add function', () => {
    // Test case 1
    it('should return the sum of two numbers', () => {
        const result = add(1, 2);
        expect(result).toBe(3);
    });

    // Test case 2
    it('should return a negative number when the sum is negative', () => {
        const result = add(-3, 1);
        expect(result).toBe(-2);
    });

    // Test case 3
    it('should work with zero', () => {
        const result = add(0, 5);
        expect(result).toBe(5);
    });
});

The calls would be the "it" calls and they have long callbacks as a second argument. I can "clone call" on the const at the start of the callback and it will clone the "it" call. The semicolons are optional.

[AndreasArvidsson]

In that example I would just use "clone state", but I do agree that starting a new statement probably is the best, but not optimal, way of handling "clone call"
I think inserting ;\n is the best we can do.

[jaresty]

I can try adjusting my habit to use clone state instead. Makes sense. The main reason I raised this issue was that clone call resulted in a syntax error, not to prescribe the solution. Adding a semicolon is perfectly reasonable.

[phillco]

@jaresty clone state does what you are trying to do today, right? You can use grand or state call to refine in the case of nesting.

I think Andreas and I agree with pokey that just picking a statement type for cloning a call adds more confusion than it helps with, because it can't be context aware. Ideally doing what you did would actually raise an error (so you would know to look for a different way) instead of inserting synthetically incorrect code, but we don't have that ability currently.

If so, I'll close this.

[jaresty]

"Clone state call " works for me. I'm not a fan of the fact that "clone call" can result in broken syntax and would prefer if it raised an error since cursorless behavior is unpredictable in an buffer that has a syntax error (tree sitter cannot parse the buffer, so I don't know what it will do if you subsequently try to clone or chuck the call that was just cloned).

[jaresty]

I think cursorless calls ideally should have an invariant that they do not break subsequent cursorless calls, basically. That's really what I'm after.

[phillco]

I think that would prevent you from using many of the text-based queries to do aggressive changes if the tree-sitter tree had to always be valid between calls. Either way I think that's outside the scope of this issue. Feel free to open another one if you feel strongly.

[jaresty]

To clarify I don't believe that it is necessary for the tree sitter tree to be valid between calls, but it is frustrating to use 'clone call' and then have 'clone call' not work again because it is no longer valid syntax. I still think this behavior is not user friendly and this is the only case that I have discovered in cursorless where cloning a syntactical object (as opposed to a textual one) does not result in valid syntax. I don't really think this is outside of the scope of this issue, but given that there is disagreement about what the correct behavior is here I'm not gonna worry about it-the workaround to just use clone state instead is sufficient for my needs.

Update-I did find one other case where clone can have unpredictable behavior when working with the syntax object. 'clone branch' can result in a syntax error in some cases, though I think there isn't a good fix for that (as far as I know).

[AndreasArvidsson]

There is definitely more scopes where clone would result in invalid syntax. For example the fact that you can clone individual lines, tokens and characters. If we're talking about parse tree scopes "clone name" and "clone type" will generate invalid code in most cases. Cursorless is just a toolbox. How you use those tools are up to the user. :)