Fix#48281 - Indentation not respected when executing various refactorings (TypeScript/JavaScript)

[MQuy]

Fixes #48281

[andrewbranch]

The next non-undefined return in this function a few lines down has indentation: undefined. Does that one need to be updated too? Or, what happens if you just remove indentation: 0 here instead of explicitly setting it? I don’t immediately see a reason why these two returns should be so different when it comes to indentation. I would suspect that either both are broken and both need the fix you applied here, or that just returning an undefined indentation is enough to fix this one. But I’m not sure without running the code.

[MQuy]

If we remove indendation: 0 -> indentation: undefined which leads to the scenario below

f(() => {
    g({});
});

• in this function (which is called later for applying changes) https://github.com/microsoft/TypeScript/blob/main/src/services/textChanges.ts#L1082, indentation will be 12 (8 from https://github.com/microsoft/TypeScript/blob/main/src/services/formatting/smartIndenter.ts#L73 and 4 from https://github.com/microsoft/TypeScript/blob/main/src/services/textChanges.ts#L1096).

For the line below, I haven't found test cases yet to double check so I think it would be safe to keep them as they are (until there is a test case, I can create another pr to fix it?).

       if (isIdentifier(token) && hasInitializer(parent) && parent.initializer && isObjectLiteralExpression(parent.initializer)) {
            const properties = arrayFrom(checker.getUnmatchedProperties(checker.getTypeAtLocation(parent.initializer), checker.getTypeAtLocation(token), /* requireOptionalProperties */ false, /* matchDiscriminantProperties */ false));
            if (!length(properties)) return undefined;
            return { kind: InfoKind.ObjectLiteral, token, properties, indentation: undefined, parentDeclaration: parent.initializer };
        }

[andrewbranch]

I think the best change would be to fix the formatter/textChanges logic that causes this indentation to be off. Codefixes generally shouldn’t need to worry about these things in their own logic. It seems like the formatter should be capable of figuring out the right indentation here. That will probably be a more difficult bug to figure out though 😬

[MQuy]

Since this case is a little bit special I guess (new properties are indented with g not {), I am not sure if it is possible and if possible like you said it might requires changes and difficult 🙈.

f(() => {
    g({});
});

What do you think? Should we merge this one to fix the issue first (we have a test case for this bug to make sure it won't happen again) and later to I will dig into formatter/textChanges (or someone else).

[andrewbranch]

I think I’m ok with this targeted fix for now, but it would be nice to know if we need the same fix a few lines down. You could try replacing indentation: undefined with something like indentation: 42 or something definitely wrong in order to find tests that touch that code.

[MQuy]

hi @andrewbranch, thank for your thought. After digging, I think it makes more sense to set a fixed indentation in fixing object literal file. When applying textChanges https://github.com/microsoft/TypeScript/blob/main/src/services/textChanges.ts#L1078, it does

• Format input code without indentation.
• Indent the output code above with the indentation like 4 (every line gets 4 more leading whitespaces).

When fixing object literal, it is a little special, we want to use Block (there are two other indent styles which are None, Smart <- default for object literal fixing) to find a indentation. Besides, if { starts in any position (can be in the middle of line), the following indentations should treat { as starting of that line (including leading whitespace).

    const a: { x: undefined, y: undefined } = {}       // leading 4 whitespaces and { starts in the middle of line
// ->
    const a: { x: undefined, y: undefined } = {
        x: undefined,
        y: undefined,
    }
// ---------------------
    const a: {x : undefined, y: undefined } =
     {}
// ->
    const a: { x: undefined, y: undefined } =
     {                                                  // leading 5 whitespaces and { starts at 6 column
         x: undefined,
         y: undefined,
     }

[andrewbranch]

I really think something is wrong here:

TypeScript/src/services/formatting/smartIndenter.ts

Line 73 in bf0eef4

return getActualIndentationForListStartLine(containerList, sourceFile, options) + options.indentSize!; // TODO: GH#18217

Deleting the addition fixes most of your test without having to use the indentation options in the codefix itself. (But it breaks other tests in ways I can’t understand without spending all day on it.) There is just no reason why the logic you added should need to be in a specific codefix. TextChanges or the formatter is doing something wrong. Many other codefixes and refactors might incidentally print an object literal, so I don’t think the codefix is the right place to make changes (other than deleting indentation: 0).

[MQuy]

@andrewbranch, I think you are right. It is better to fix in textChanges/smartIndenter. I pushed changes for consider curly brace (same for object or function) as block when formatting, what do you think? 🤔 (I tried to run tests locally, it passed)

[andrewbranch]

This makes sense to me. Thanks for the revisions!

[jakebailey]

LGTM