clang-tidy readability-else-after-return fix generates invalid code with LF files

[sean-mcmanus]

Use a test.cpp file with LF line endings (it doesn't repro with CLRF line endings). I used clang-tidy 14.0.0.

clang-tidy.exe --checks=readability-else-after-return --fix test.cpp

void func(int ii) {
    if (ii == 0) {
        return;
    }
    else {//dfd
        //aaa
    }//dfd
}

Bug: Output is invalid code

void func(int ii) {
    if (ii == 0) {
        return;
    }
    /dfd
        //aaa
   //dfd
}

The comment right after the { case is arguably "artificial", and in "normal" cases it would just result in incorrect/unusual whitespace, which could potentially be fixed via formatting.

Seems like an off-by-one error with code that is expecting CLRF line endings. The bug also repros with output from the --export-fixes yaml file.

The LF line endings are normal on Linux/Mac, but on Windows it is not "normal" but can occur when bypassing git's normal conversion to CLRF or when a user opt for that.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[sean-mcmanus]

I originally reported this bug on Windows, but I just hit it with Linux/Mac too, in which case the LF line ending is the default.

[sean-mcmanus]

Also, this case below isn't fixed via formatting, because clang-format doesn't add a newline.

    if (true) {
        return 0;
    } else {
        // comment
        return 0;
    }

so the result is

    if (true)
    {
        return 0;
    } // comment
    return 0;

This bug doesn't repro with CLRF line ending files (i.e. Windows default).

[njames93]

This is an annoying hack due to limitations in clang-tidy how it handles reformatting the changes it makes, it's on a long Todo list atm, especially now that there are more checks that would require a similar hack to format properly.

[sean-mcmanus]

I'm guessing you're referring to my last comment in which I do an additional step of running clang-format after applying the clang-tidy fix and not to the original issue with the clang-tidy fix. I'm not familiar with the implementation, but the root bug doesn't seem like a hack with reformatting to me, since it works fine with CLRF files and normally clang-tidy fixes aren't supposed to apply formatting when --export-fixes is used (e.g. other fixes generate new code with no indentation), and normally formatting isn't supposed to delete a /, only whitespace. It seems like it could check for a LF line ending and then adjust the fix so it works like the CLRF case. I agree that once the fix has been applied then I wouldn't expect clang-format to have the necessary info to add a newline after-the-fact.