I'm actually wondering if we should have a code fix at a naked x = 3; statement to change it to const x = 3;.

I added tests for for ... in and object destructuring. All errors that should come with this new codefix seem to now do so correctly.
However an existing fourslash test defined in incompleteFunctionCallCodefix3.ts and I fail to understand, how this change led to the test erroring. I have not yet commited the baselines due to this.

CC @DanielRosenwasser to review, and to consider whether we should have this change on the statement x = 0; where x is not defined.

Ping @DanielRosenwasser @Andy-MS
Just a friendly reminder, that this has been inactive for a while now.

to consider whether we should have this change on the statement x = 0; where x is not defined

If @DanielRosenwasser has no opinion, this sounds reasonable to me.

@rFlorian, sorry that this review went cold. Are you interested in continuing this? It looks like there are a few seemingly unnecessary/unrelated changes to some other fourslash tests that we’ll want to revert. I’d like to add the codefix for the standalone variable declaration statement, and I can help you look into the for ([x, y] of [[1,2]]) {} case if you still need help with that.

If you want to pick it back up, I’ll help get it to the finish line. Otherwise, I can just pick up where you left off (probably later this month).

@andrewbranch The delay is no big deal, thanks for having a look at the PR now! If the changes required for the additional codefix are something I can handle I'll be happy to add them.

I reverted the changes to the fourslash tests and could run them successfully locally afterwards. If I recall correctly they failed at an intermediate stage of this branch and I hadn't reverted them since.

The for ([x, y] of [[1,2]]) {} cases are tested in codeFixAddMissingConstInForLoopWithArrayDestructuring1 and ...2 respectively and look to be working as expected.

What would be the best course of action to add the codefix for the x = 0 with x being undefined? The diagnostic messages themselves can probably be reused since they are not bound to the context of for-statements but I am assuming a new codefix will need to be created to handle these cases.

How extensive should that additional codefix be? Some cases that I can come up with are:

• x = 0;
• x = 1, y = 1;
• {x} = {x: 2};
• [x] = [3];
  Which of these cases and possibly which additional cases do you consider to be in scope?

TL;DR:

• x = 0;—yes
• x = 1, y = 1;—give it a shot
• {x} = {x: 2};—perhaps not
• [x] = [3];—give it a shot

The examples after the first all depend on every would-be variable or binding element being an unresolved name, e.g.:

let x;
[x, y] = [0, 1];

wouldn’t be a candidate because x exists, so adding const before that would be an error for x even though it might be want you wanted for y.

Of those, all but the object destructuring produce a fairly tidy parse tree, so I think it shouldn’t be too messy to offer the quick fix there.

{ x } = { x: 2 }

Here, the = actually ends up in the trivia between two Block nodes, so detecting this particular situation would involve checking that the parse tree structure matches a very particular brand of “total garbage.” I would be hesitant to rely on the stability of that structure, as the exact results of garbage in → garbage out scenarios do change occasionally, especially in cases where we can actually improve the error messages associated with “garbage out” a bit.

So, I think it would be lovely if { x } = { x: 2 } worked, but my hunch is there’s not a clean way to do it that doesn’t assert a very specific invalid tree structure, and I would rather avoid that.

@andrewbranch I added fourslash test cases for the standalone and array-initialization cases. After that I also started implementing these changes and got the standalone case working as far as I can tell. My biggest problem to which I couldn't find a solution was a way to find all declared identifiers at the point where the error occurs. As you already pointed out that information is required to prevent false positives for changes where multiple identifiers are involved.

I skipped the comma separated case for now because it requires the same information of available identifiers like arrays and also produces a slightly more complex parse tree. However I think it can be done by recursing through the ExpressionStatement node and collecting all identifiers along the way.

A hint on how to retrieve available identifiers would be much appreciated.

how to retrieve available identifiers

There is a checker function that does something like this (checker.getSymbolsInScope()), but that sounds like more work than you need to do. Rather than starting with “give me all valid identifiers” and seeing if every array element is one of them, it’s much more efficient to start with each array element and see if we can tell whether it exists. There are probably a few ways to do it, but my guess is that the cheapest is checker.getSymbolAtLocation(identifier)—if it returns undefined, then the binder wasn’t able to find a declaration for this identifier. I think it’s pretty good because the checker has to do very little work to get this info; it’s mostly looking up work the binder has already done.

I just played around with it and it seems like this works:

if (isArrayLiteralExpression(parent)) {
    // You’ll need to pass `context.program` into `makeChange`
    const checker = program.getTypeChecker();
    if (!every(parent.elements, element => arrayElementCouldBeVariableDeclaration(element, checker))) {
        return;
    }

    return applyChange(changeTracker, parent, sourceFile, fixedNodes);
}

function arrayElementCouldBeVariableDeclaration(expression: Expression, checker: TypeChecker) {
    const identifier =
        isIdentifier(expression) ? expression :
        isAssignmentExpression(expression, /*excludeCompoundAssignment*/ true) && isIdentifier(expression.left) ? expression.left :
        undefined;
    return !!identifier && !checker.getSymbolAtLocation(identifier);
}

Note also that we’re being more specific about what syntax we can try to fix by making sure each array element could become a binding pattern—e.g. [x, y = 0] is a syntactic candidate but [x, y()] isn’t. (My approach here is, for each element, check syntax and then check for a symbol; it might be more efficient to do, check every element for syntax, then if it all looks good, check every element again for a symbol. But I think symbol lookup should be cheap enough that either way is ok.)

Other random note: I tried running the codefix inline in addMissingConst.ts where I was indented a few tab stops, and it looks like some formatting is wonky—you should maybe add a test where the code to be fixed is indented:

@andrewbranch Thanks for the quick reply and very useful hints. I added additional tests that check for existing indentation being untouched by this change.

The root issue of indentation getting messed up with the old version lied in ChangeTracker#getAdjustedStartPosition which due do the nature of the syntactically invalid identifier node interpreted that node as spanning multiple lines and thus added padding after it to 'match' the surroundings. Both of these checks fell through:

const fullStart = node.getFullStart();
const start = node.getStart(sourceFile);
if (fullStart === start) {
     return start;
}
const fullStartLine = getLineStartPositionForPosition(fullStart, sourceFile);
const startLine = getLineStartPositionForPosition(start, sourceFile);
if (startLine === fullStartLine) {
      return leadingTriviaOption === LeadingTriviaOption.IncludeAll ? fullStart : start;
}

That function was previously called from changeTracker.insertNodeBefore(...) which I could remove in favor of the simpler changeTracker.insertModifierBefore(...) which seems to be just the right tool for the job.

I will have a look at the comma separated initializer case tomorrow as it now looks a lot more achievable to support as well.

@andrewbranch It looks like I got the x = 0, y = 0 etc cases working. I dont like the way I am traversing the AST downwards to check for non-existence of all Identifiers contained within the leading ExpressionStatement but I could not find an available alternative and it seems to be doing the job fine for all test cases. As such I think the PR is now ready for review.

Looks like there are lint errors 🙈

Thanks for all your work on this! I look forward to reviewing it in full tomorrow ✨

Whoops forgot to remove a bit of whitespace 🙈

@andrewbranch It was a pleasure to push this PR over the finish line with your help 😄