Testing clang Fix-Its attached to notes

Clang Frontend
1

I am aware of two ways we use to test clang Fix-Its with FileCheck.

  1. Check directly the generated Fix-Its by using -fdiagnostics-parseable-fixits.
    “-fdiagnostics-parseable-fixits
    Print fix-its in machine parseable form”

  2. Check the end result after Fix-Its are applied to the source file by using -Xclang -fixit.
    “-fixit Apply fix-it advice to the input source”

FileCheck tests against the end result are both less prone to errors in test-cases and easier to create and maintain. However, -fixit ignores Fix-Its attached to notes.
Is there any way how to use the latter approach with Fix-Its attached to notes? If not - would there be interest in having one? I can imagine for example adding a hidden cc1 flag that would change the existing behavior.

1 Like
2

Both test strategies have their uses. FileCheck helps check precision of location information, -fixit helps check that the fixes are functionally applied properly so that the test is properly repaired.

The reason -fixit ignores fixes attached to notes is because the fixes on notes are not known to be correct (and sometimes will give conflicting fixes where the programmer has to pick between the two because they’re mutually exclusive). I don’t know that we should have a switch to apply fixes from notes, even as a hidden option. clang-tidy added --fix-notes as part of its public interface but it doesn’t seem to get used in practice (I did a search on sourcegraph and found 1-2 distinct uses of it (the rest are in clones, only ~10 uses overall), possibly due to the behavior being erratic.

3

Both test strategies have their uses. FileCheck helps check precision of location information, -fixit helps check that the fixes are functionally applied properly so that the test is properly repaired.

Let me just make sure we’re on the same page because the way I imagine using -fixit in tests is not really different from -fdiagnostics-parseable-fixit in its ability to precisely check locations in Fix-It transformations. I was thinking about something like this:

// RUN: cp %s %t
// RUN: %clang_cc1 -Wfoo -fixit %t
// RUN: grep -v CHECK %t | FileCheck %s

int foo;
// CHECK: int bar;

as compared to this:

// RUN: %clang_cc1 -Wfoo -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s

int foo;
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:8}:"bar"

The reason -fixit ignores fixes attached to notes is because the fixes on notes are not known to be correct (and sometimes will give conflicting fixes where the programmer has to pick between the two because they’re mutually exclusive). I don’t know that we should have a switch to apply fixes from notes, even as a hidden option. clang-tidy added --fix-notes as part of its public interface but it doesn’t seem to get used in practice (I did a search on sourcegraph and found 1-2 distinct uses of it (the rest are in clones, only ~10 uses overall), possibly due to the behavior being erratic.

I fully understand the concern and hear your doubts about value of having such user-facing feature. I am entirely motivated by the testing story and while this is a potential solution, it is not the only one.

Perhaps a different solution would be to have a testing utility that would take Fix-Its emitted by clang and apply them to the sources. What would you think about that?

4

Now I’m confused – that’s how -fixit works already in Clang:

F:\source\llvm-project>cat "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
void foo();

int main() {
  froo();
}

F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -cc1 -fixit "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:3: error: use of undeclared identifier 'froo'; did you mean 'foo'?
  froo();
  ^~~~
  foo
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:3: note: FIX-IT applied suggested code changes
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:6: note: 'foo' declared here
void foo();
     ^
1 error generated.

F:\source\llvm-project>cat "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
void foo();

int main() {
  foo();
}
5

Now I’m confused – that’s how -fixit works already in Clang:

That’s true but it doesn’t apply Fix-Its attached to notes which is what I am looking for.
Having a separate testing utility that would apply all Fix-Its regardless of to what diagnosit they are attached to could solve that problem. My first choice would probably be a flag that would teach -fixit to do this but a separate testing utility would work too.

6

Sure, but how does that handle this concern:

What happens when the note has multiple fix-its associated with it and you say “apply the fix to the source” (whether through -fixit or through a separate testing utility)?

7

What happens when the note has multiple fix-its associated with it and you say “apply the fix to the source” (whether through -fixit or through a separate testing utility)?

For such cases it doesn’t make sense to apply all the Fix-Its and testing would likely still have to rely on -fdiagnostics-parseable-fixit.

8

Okay, so to make sure I’m on the same page, what you’re proposing is:

Provide some way to allow a fixit from a note to be applied to source code when there’s only one fixit associated with the diagnostic. If there are multiple fixes, then the source is not changed for that diagnostic (do you give a warning “hey we didn’t apply these fixes because we didn’t know what you wanted” in this case?) And this functionality is only supported for testing purposes (so it’ll either be a standalone utility or a -cc1 only flag, rather than exposed through the driver).

Do I have it right?

9

Yes, that’s right.

Re: warning about multiple Fix-Its attached to a note - from the testing perspective use of this feature implies that a single Fix-It per note is expected and violation of that should be surfaced in a way that fails the test.
In case of a standalone testing utility an error return status sounds appropriate.
If we add -fixit-test-notes to clang I feel that a warning might be insufficient to avoid false negatives with CHECK-NOT in the intended workflow:

// RUN: cp %s %t
// RUN: %clang_cc1 -Wfoo -fixit-test-notes %t
// RUN: grep -v CHECK %t | FileCheck %s
// CHECK-NOT: /* incorrect code pattern */
…

Such test could have false positives if there’s multiple Fix-Its attached to a single note. This makes me think we would have to emit an error.

What do you think?

10

Re: warning about multiple Fix-Its attached to a note

I don’t think this is actually a thing. If you send multiple fixit objects into the note, they simply become one big fixit. I think Aaron was thinking only about multiple fixits attached to multiple notes.

1 Like
11

The situation I am thinking of is: Compiler Explorer

12

Yep, clang emits two fix-its attached to the same note and the user is supposed to pick zero or one of them.

That’s what I referred to in my reply above:

1 Like
13

Okay, I think the idea sounds reasonable to me.

14

@AaronBallman Just to clarify - are you ok with either of the solutions or just one of them?
a) new cc1 flag -fixit-test-notes that would emit an error when there are multiple fix-its attached to a single diagnostic
b) a new standalone testing utility that would take parseable fixits and path to the source file as inputs and apply all the fixits (including the ones attached to notes); it would return error code if are multiple fix-its attached to a single diagnostic

15

I think (a) sounds like the right approach to me. A standalone utility would be pretty heavy-weight and I’m not too worried if a user decides to apply fix-its through the -cc1 invocation (they’ll get whatever they get, as with passing any cc1 option manually).

1 Like