Update: support multiple fixes in a report (fixes #7348)

[mysticatea]

What is the purpose of this pull request? (put an "X" next to item)

[X] Add something to the core

Fixes #7348.

What changes did you make? (Give an overview)

This PR adds the ability to handle multiple fixes for each report. The fix methods can return an iterable object, includes arrays.

context.report({
    node,
    message,
    fix(fixer) {
        return [
            fixer.insertTextBefore(node, "("),
            fixer.insertTextAfter(node, ")")
        ];
    }
})

Those fixes are merged to one internally, so this does not change about the result of ESLint; each message has one fix property as same as the current behavior. This is not a breaking change.

Pros:

• We can make more readable fix functions.

Cons:

• Nothing in particular.

---

This PR includes 2 examples.

One is about arrow-body-style. The rule had a super complex fix function.

Before:

fix(fixer) {
    if (blockBody.length !== 1 || blockBody[0].type !== "ReturnStatement" || !blockBody[0].argument) {
        return null;
    }

    const sourceText = sourceCode.getText();
    const returnKeyword = sourceCode.getFirstToken(blockBody[0]);
    const firstValueToken = sourceCode.getTokenAfter(returnKeyword);
    let lastValueToken = sourceCode.getLastToken(blockBody[0]);

    if (lastValueToken.type === "Punctuator" && lastValueToken.value === ";") {

        /* The last token of the returned value is the last token of the ReturnExpression (if
         * the ReturnExpression has no semicolon), or the second-to-last token (if the ReturnExpression
         * has a semicolon).
         */
        lastValueToken = sourceCode.getTokenBefore(lastValueToken);
    }

    const tokenAfterArrowBody = sourceCode.getTokenAfter(arrowBody);

    if (tokenAfterArrowBody && tokenAfterArrowBody.type === "Punctuator" && /^[([/`+-]/.test(tokenAfterArrowBody.value)) {

        // Don't do a fix if the next token would cause ASI issues when preceded by the returned value.
        return null;
    }

    const textBeforeReturn = sourceText.slice(arrowBody.range[0] + 1, returnKeyword.range[0]);
    const textBetweenReturnAndValue = sourceText.slice(returnKeyword.range[1], firstValueToken.range[0]);
    const rawReturnValueText = sourceText.slice(firstValueToken.range[0], lastValueToken.range[1]);
    const returnValueText = firstValueToken.value === "{" ? `(${rawReturnValueText})` : rawReturnValueText;
    const textAfterValue = sourceText.slice(lastValueToken.range[1], blockBody[0].range[1] - 1);
    const textAfterReturnStatement = sourceText.slice(blockBody[0].range[1], arrowBody.range[1] - 1);

    /*
     * For fixes that only contain spaces around the return value, remove the extra spaces.
     * This avoids ugly fixes that end up with extra spaces after the arrow, e.g. `() =>   0 ;`
     */
    return fixer.replaceText(
        arrowBody,
        (textBeforeReturn + textBetweenReturnAndValue).replace(/^\s*$/, "") + returnValueText + (textAfterValue + textAfterReturnStatement).replace(/^\s*$/, "")
    );
}

After:

*fix(fixer) {
    if (blockBody.length !== 1 ||
        blockBody[0].type !== "ReturnStatement" ||
        !blockBody[0].argument ||
        hasASIProblem(sourceCode.getTokenAfter(arrowBody))
    ) {
        return;
    }

    const openingBrace = sourceCode.getFirstToken(arrowBody);
    const closingBrace = sourceCode.getLastToken(arrowBody);
    const firstValueToken = sourceCode.getFirstToken(blockBody[0], 1);
    const lastValueToken = sourceCode.getLastToken(blockBody[0]);
    const commentsExist =
        sourceCode.commentsExistBetween(openingBrace, firstValueToken) ||
        sourceCode.commentsExistBetween(lastValueToken, closingBrace);

    // Remove tokens around the return value.
    // If comments don't exist, remove extra spaces as well.
    if (commentsExist) {
        yield fixer.remove(openingBrace);
        yield fixer.remove(closingBrace);
        yield fixer.remove(sourceCode.getTokenAfter(openingBrace)); // return keyword
    } else {
        yield fixer.removeRange([openingBrace.range[0], firstValueToken.range[0]]);
        yield fixer.removeRange([lastValueToken.range[1], closingBrace.range[1]]);
    }

    // If the first token of the reutrn value is `{`,
    // enclose the return value by parentheses to avoid syntax error.
    if (astUtils.isOpeningBraceToken(firstValueToken)) {
        yield fixer.insertTextBefore(firstValueToken, "(");
        yield fixer.insertTextAfter(lastValueToken, ")");
    }

    // If the last token of the return statement is semicolon, remove it.
    // Non-block arrow body is an expression, not a statement.
    if (astUtils.isSemicolonToken(lastValueToken)) {
        yield fixer.remove(lastValueToken);
    }
}

.

I think the "After" is more readable. Because "Before" had to focus on the operation of the text which we want to not change mainly. On the other hand, "After" focuses on the operation of the text (token) that we want to change mainly. I think this is a huge difference.

Another one is semi. This is an example about to control conflictions of autofix.

After:

fix = function *(fixer) {
    yield fixer.remove(lastToken);

    // To avoid no-extra-semi removes the next semicolon.
    // https://github.com/eslint/eslint/issues/7928
    if (nextToken && astUtils.isSemicolonToken(nextToken)) {
        yield fixer.keep(nextToken);
    }
};

---

In addition, This PR adds some APIs.

• sourceCode.commentsExistBetween(left, right) returns true if comments exist between the left and the right. Otherwise it returns false. Since we frequently use this check to avoid removing comments by autofix, it would be useful.
• fixer.keep(nodeOrToken) and fixer.keepRange(range) returns a fix object which does not change code. Those would allow us to control conflictions of autofix, utilizing the mechanism that overlapped fixes are not applied at the same time.

Is there anything you'd like reviewers to focus on?

Direction. The original issue is not accepted yet, but I want to advance this feature. Currently, we have to focus on the text which we want to not change to keep comments when we implement autofix. This feature would allow us to focus on the text which we want to change.

I'm a champion for this.

[mention-bot]

@mysticatea, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alberto, @nzakas and @vitorbal to be potential reviewers.

[eslintbot]

LGTM

[eslintbot]

LGTM

[eslintbot]

LGTM

[mysticatea]

I will try to explain more details about this PR.

The purpose of this PR is that it supports multiple fix objects per report, as keeping no breaking change.

context.report({
    node,
    message,
    fix(fixer) {
        return [
            fixer.insertTextBefore(node, "("),
            fixer.insertTextAfter(node, ")")
        ];
    }
})

There are changes in RuleContext#report function.
If the fix function returns multiple fixes objects, RuleContext#report merges those to one.

For example:

/* eslint arrow-body-style: error */

const foo = () => {
    return {a: 1}
}

In this case, arrow-body-style of this PR generates 4 fix objects:

[ { "range": [ 56, 69 ], "text": "" },
  { "range": [ 69, 69 ], "text": "(" },
  { "range": [ 75, 75 ], "text": ")" },
  { "range": [ 75, 77 ], "text": "" } ]

Then RuleContext#report merges those to one:

{ "range": [ 56, 77 ], "text": "({a: 1})" }

After that, it does existing autofix process if --fix option is given.

---

• This does not effect performance if existing fix functions since this merging process is run only when a fix function returns multiple fix objects.
• This keeps the result of the report as currently.

[
    {
        "filePath":"C:\\Users\\t-nagashima.AD\\Documents\\GitHub\\eslint\\test.js",
        "messages":[
            {
                "ruleId":"arrow-body-style",
                "severity":2,
                "message":"Unexpected block statement surrounding arrow body.",
                "line":3,
                "column":19,
                "nodeType":"ArrowFunctionExpression",
                "source":"const foo = () => {",
                "fix":{
                    "range":[
                        56,
                        77
                    ],
                    "text":"({a: 1})"
                }
            }
        ],
        "errorCount":1,
        "warningCount":0,
        "source":"/* eslint arrow-body-style: error */\n\nconst foo = () => {\n    return {a: 1}\n}\n"
    }
]

---

I want to investigate the way of no-merging and overlap detection improvement after this PR. (maybe it makes a breaking change)
Related to #7928 (comment)

[eslintbot]

LGTM

[eslintbot]

LGTM

[gyandeeps]

LGTM 👍 as always nicely done @mysticatea ...

[alangpierce]

Looks good as far as I can tell, although I would love to be able to call keep on a larger range than my fix in order to fix #8026.

[alangpierce]

Should be "The first fix" and "The second fix"

[mysticatea]

Oops, it's my copy/paste mistake. Thank you!

[alangpierce]

Requiring the ranges to be disjoint could make fixer.keep a lot less useful, I think. In #8067 for the no-else-return/no-useless-return fix (#8026), I intentionally want to keep all of an outer node (the function containing the fix) while modifying something inside of it.

With this API, I guess what I'd do is compute fixStart and fixEnd and yield that fix, then yield keepRange(functionStart, fixStart) and keepRange(fixEnd, functionEnd). That isn't so bad, I suppose, but seems a lot more clunky than saying "preserve the function and replace this text within the function".

One possible solution, if we're ok with it being a little magical, is to allow fixes to be completely contained within other fixes and always use the most specific one. Or maybe the fixes could be applied in order in a reasonable way, with later ones taking precedence.

(Although I recognize that there's maybe some disagreement about whether keep should be used that way in my use case anyway.)

[mysticatea]

Honestly, I think it's rare case if we need locking the outer node.
In no-else-return case, it needs locking only the previous if block.
In no-useless-return case, it needs locking only after the return statement.

[alangpierce]

That makes sense, I can see how the fixes for no-else-return and no-useless-return would work in that case. (I'm still a little unsure about how to properly reason about the implementation of no-useless-return, but it certainly seems easy and correct to call keepRange from the end of the fix to the end of the function.)

I think locking the outer node can be nice because it's safer and makes the rule simpler, but I can see the argument that it's usually not necessary. And if the long-term plan is to allow a distinction between "fixing" and "locking" operations, then I think it shouldn't be hard to allow the two types to overlap, which would address my concerns there.

[alangpierce]

Worth noting that mergeFixes could be made an internal utility instead of a public API if there's any controversy around this API addition.

[mysticatea]

As public API, this is simple enhancement; the fix functions support for an iterable object as their return value.

There are 2 topics here.

• To not modify comments, this feature would be helpful.
• To avoid autofix conflict, this feature would be helpful.

As I mentioned in this post, currently we have a pain to remain comments. The purpose of the fix functions is "to modify code", but we have to focus on "to not modify code" mainly in order to not remove comments. This enhancement would resolve the problem. I think this is valuable for plugin ecosystem as well.

In addition, we can address locked ranges in the same mechanism.

---

In fact, this is the 1st step of my plan.

Because the report of linting has fix properties which have a fix object, this PR is merging the multiple fixes before it makes the report for now. I have to avoid a breaking change about the fix property (the shape of the report also are public API).

However, I have a plan (though it's not accepted yet) to change the fix property to fixes property that has an array of fix objects, so this mergeFixes function would be removed in future. It implies that fixing process can distinguish between fixing ranges and locking ranges. I expect it to allow us smarter conflict check.

[eslintbot]

LGTM

[mysticatea]

I updated this PR; now we can use fixer.keepRange(containingNode).

[ilyavolodin]

@mysticatea I will support this enhancement with one change request. I really would like to avoid generators in our code base. While I understand that they can be very helpful, but I feel like majority of JS community doesn't really understand them and they significantly increasing bar of entry. I would like to avoid both generators and yield keyword as much as humanly possible.

[mysticatea]

Thank you, @ilyavolodin !

Hmm, though I don't think generators make hurdles.
Anyway, I would remove yield for now.

[gyandeeps]

Marking this accepted as we have the support and @mysticatea will champion.
I think we are almost their accept that last part of the discussion. Can you please update on whats the plan is @mysticatea

[not-an-aardvark]

Technically, I think we would need TSC consensus for this because it's a core change.

I like the general idea of allowing multiple fixes, but I'm not sure we should add fixer.keep and fixer.keepRange to core yet. I'm wondering if we should try out the solution from #7928 for longer before we make a core change.

[mysticatea]

I'm sorry for late. I will get the time to work on this within this week end.

• I will remove yield in this PR. But I will not remove yield support.
• I will remove keep/keepRange. I will separate PR in order to continue discussion.

[eslintbot]

LGTM

[not-an-aardvark]

I think the autofixing and SourceCode documentation need to be updated to describe the new API.

(I'm also planning to review this PR later, assuming it's not going into the release today.)

[not-an-aardvark]

Thanks! This mostly looks good but I think there is a bug if a fix range contains -1 as an index.

[not-an-aardvark]

Some fix ranges, like unicode-bom, use -1 as the start to indicate that the fix should replace the BOM. I think that will cause a bug here because .slice(-1) will take the last character in the string.

[eslintbot]

LGTM

[eslintbot]

LGTM

[mysticatea]

Thank you for the review.

• I updated code to address negative indices.
• I added documents.

[not-an-aardvark]

Would this fail to detect a conflict if two fixes have ranges [-1, 0] and [-1, 5]? It seems like the assertion will always pass for fixes that start at -1.

[mysticatea]

Good catch.

[eslintbot]

LGTM

[not-an-aardvark]

LGTM, thanks!

[not-an-aardvark]

Maybe it would be better to use -Infinity in case someone uses extremely small numbers in fix ranges for whatever reason, but I think it's probably fine -- files usually don't have 2⁵³ BOMs. 😄

[mysticatea]

Yeah 😄

I expect that integer variables are faster by compiler optimization.