This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Add neighboring annotations for -dump-input-filter=error
AbandonedPublic

Authored by jdenny on Feb 13 2021, 11:39 AM.

Details

Summary

For example:

$ cat check
CHECK: start
CHECK-NEXT: end

$ FileCheck -v check < input |& tail -27
<<<<<<
         1: start
check:1     ^~~~~
         2: foo0
         3: foo1
         4: foo2
         5: foo3
         6: foo4
         .
         .
         .
        18: foo16
        19: foo17
        20: foo18
        21: foo19
        22: foo20
        23: end
next:2      !~~ error: match on wrong line
        24: bar0
        25: bar1
        26: bar2
        27: bar3
        28: bar4
         .
         .
         .
>>>>>>

Without this patch, input lines 1-6 are not shown. This patch reveals
those lines because that's where the actual problem is because that's
where the CHECK-NEXT directive was expected to match but didn't.
Seeing the line where CHECK-NEXT actually matched, line 23, is
typically less useful information in my experience.

In general, this patch depends on the following heuristics:

  1. Any annotation that neighbors an error annotation is likely useful in debugging that error.
  2. Revealing these annotations usually does not increase verbosity significantly.

To point 1, this patch facilitates debugging of several kinds of
errors, examples of which this patch adds to the test suite:

  • A CHECK-NEXT or CHECK-SAME match on the wrong line.
  • A CHECK-NOT unexpected match because a neighboring directive matched at an unexpected point, affecting the search range.
  • An unmatched CHECK because a subsequent CHECK-LABEL matched at an unexpected point, affecting the search range.

To point 2:

  • This patch is intended to reveal at most two neighboring annotations per error.
  • This patch has no effect when neither -v or -vv is specified because annotations for successful directives are then not generated. Generally, I keep -vv in my environment's FILECHECK_OPTS, but I'm not sure other LLVM developers will want it enabled by default.
  • This patch usually doesn't affect the most common kind of failure: a positive directive that doesn't match anything and that isn't followed by a CHECK-LABEL. First, its error annotation already starts at the preceding match, so context typically already reveals the preceding match's annotation. Second, no more directives are processed after the failure, so there are no subsequent annotations to reveal.
  • This patch has no effect when -dump-input-filter is set to something other than error (the default). Other values usually produce more verbose input dumps already.

Diff Detail

Event Timeline

jdenny requested review of this revision.Feb 13 2021, 11:39 AM
jdenny created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2021, 11:39 AM
thopre added inline comments.Feb 15 2021, 3:36 AM
llvm/test/FileCheck/dump-input/filter-error-neighbor/multiline.txt
2–12

Could you do like the other tests and explain what input lines are revealed by this code?

llvm/utils/FileCheck/FileCheck.cpp
157–160

Isn't there one too many "annotations" in that sentence?

517–518

Is this saying that for a LABEL match ending at line L, the annotation for an error on line L-1 would be after the annotation for the LABEL match end at line L? Or is the "that precedes it" redundant and this is just saying that in the case of an error end and a LABEL end on line L, the label is first?

jdenny added inline comments.Feb 15 2021, 7:04 AM
llvm/utils/FileCheck/FileCheck.cpp
157–160

Semantically and grammatically, I think it's correct. Is your concern that so many occurrences of "annotations" makes it confusing?

517–518

An error can start on an earlier line than a label and end on the same line as the label, but, on that last line, the error is shown after the label. I agree it's confusing, I'll add an example to the comments.

jdenny updated this revision to Diff 324112.Feb 16 2021, 2:01 PM
jdenny marked 3 inline comments as done.

Addressed @thopre's comments.

llvm/test/FileCheck/dump-input/filter-error-neighbor/multiline.txt
2–12

Done. In most tests, I also clarified which lines are specifically revealed by -dump-input-filter=error vs. the lines added by -dump-input-context=2.

llvm/utils/FileCheck/FileCheck.cpp
157–160

I've made an attempt at clarifying the sentence. Please let me know if it helped.

This seems reasonable in general. I haven't got time to review the actual implementation though right now. If it's still open come next week, and I have the time, I'll try to take a look, but don't wait for me, if others are happy.

This seems reasonable in general. I haven't got time to review the actual implementation though right now. If it's still open come next week, and I have the time, I'll try to take a look, but don't wait for me, if others are happy.

Thanks. This patch could affect many people, especially if many people are using -v or -vv as I always do, so I'd prefer more than one accept. In other words, I'm happy to wait a bit.

jdenny added inline comments.Feb 17 2021, 9:31 AM
llvm/utils/FileCheck/FileCheck.cpp
160

Oops, I added tabs. Will fix when there are more changes to make.

Would the neighbouring work if the error is on a CHECK-NOT and there's another CHECK-NOT immediately preceding and/or following it? Wouldn't the diagnostics for those other CHECK-NOT be the neighbouring diagnostics when what we would want is the neighbouring positive match (i.e. ignore CHECK-NOT)? In other word, with the following directives:

CHECK: start
CHECK-NOT: foo
CHECK-NOT: 42
CHECK-NOT: bar
CHECK: end

and an input consisting of:

start
lots of stuff
42
lots more stuff
end

would that reveal lines start and end or some of the stuff because the neighbouring code is looking at the neighbouring CHECK-NOT diagnostics?

llvm/test/FileCheck/dump-input/filter-error-neighbor/check-label-follows.txt
2
llvm/test/FileCheck/dump-input/filter-error-neighbor/check-next-same.txt
1

Why is the error on input line 2 for a CHECK-SAME?

llvm/test/FileCheck/dump-input/filter-error-neighbor/multiline.txt
2–12

That's very nice, thank you

3

I think the error line is confusing because it is not clear that the error refer to an input error as opposed to an annotation error. And that is precisely what confused me on my first reading of this patch. If I understood correctly the code, a NOT annotation error as in the range of lines from the first line after the preceding positive match directive (CHECK or CHECK-LABEL here) to the last line before the following positive match directive. i.e. the input error is in input line 16 but the annotation error goes from line 6 to line 27.

14
15

I'm confused. Doesn't the CHECK-NOT diagnostic run from line 6 to 27 and that's why line 3-7(5+2 context lines) and 25-29 (27+2 context lines) are revealed? Should that be talking about the span of the CHECK-NOT annotation/diagnostic instead of the CHECK-NOT match (line 16-17)?

My understanding is that the special handling does not cover the first label because it is only for a label immediately after the last line of a NOT, there's no symmetric special handling for before the error. Forgive me if I've misunderstood again.

Would the neighbouring work if the error is on a CHECK-NOT and there's another CHECK-NOT immediately preceding and/or following it?

Ah, you're right. There are probably ways to fix this but....

I'm becoming convinced this neighboring heuristic is the wrong approach. First, the implementation is confusing, as evidenced by your inline comments. Second, and more importantly, it's growing special cases: CHECK-LABEL and now CHECK-NOT. I fear it will become increasingly brittle as FileCheck evolves.

The real point, which doesn't seem brittle or confusing, is that we want to see search ranges for errors (but probably not for successful patterns as that could get quite noisy). Actually, explicitly showing those search ranges rather than implying them via neighboring annotations is probably better for people who are still learning FileCheck's logic.

Currently, we show search ranges for unmatched patterns using X~~, and sometimes a fuzzy match appears within marked by ?~~. So now I guess I'm suggesting adding X~~ for unexpected matches as well, so !~~ would appear within. However, X~~ is currently documented to mean no match, which is wrong in the latter case. Moreover, I already find X~~ to be noisy for long search ranges, which most errors have. CHECK-NOT blocks are especially cluttered.

A less noisy (in most cases) possibility is for each end of the search range to be a separate annotation. The markers might be [ and ], but there are other bracketing symbols we could choose. For example:

<<<<<<
         . 
         . 
         . 
        28: ....
        29: ....
        30: start
check:1     ^~~~~
not:2'0          [ search range start
        31: ....   
        32: ....
         .
         . 
         . 
        59: ....
        60: ....
        61: foo
not:2'1     !~~  error: no match expected
        62: ....
        63: ....
         .
         .
         .
        88: ....
        89: ....
not:2'2         ] search range end
        90: end
check:3     ^~~
>>>>>>

At first, we might do this only for unexpected matches (CHECK-SAME/CHECK-NEXT match on the wrong line, or CHECK-NOT match). If we like it, we might then consider it for unmatched directives as well. In other words, X~~ would go away, and we'd have, for example:

<<<<<<
           .
           .
           .
          28: ....
          29: ....
          30: start
check:1       ^~~~~
check:2'0          [    error: no match found in search range starting here
          31: ....
          32: ....
          33: ....
           .
           .
           .
          87: ....
          88: ....
check:2'1         ] search range end
          89: end
check:3       ^~~
>>>>>>

(Again, there might be a fuzzy match marked by ?~~ within, but I didn't put together an example.)

What do you think? Looking at the implementation, I think search ranges would be easy to capture for errors.

In any case, thanks for your helpful review. I started writing responses to each of your inline comments, but now I'm thinking it might be best to just reboot.

What do you think? Looking at the implementation, I think search ranges would be easy to capture for errors.

Sorry for not spotting this question before. Your proposals seem okay to me, but I'm not sure I personally have any particular useful opinions on the topic, as I tend not to use the --dump-input option for various reasons, which are outside the scope of this work.

Sorry for the late review.

Would the neighbouring work if the error is on a CHECK-NOT and there's another CHECK-NOT immediately preceding and/or following it?

Ah, you're right. There are probably ways to fix this but....

I'm becoming convinced this neighboring heuristic is the wrong approach. First, the implementation is confusing, as evidenced by your inline comments. Second, and more importantly, it's growing special cases: CHECK-LABEL and now CHECK-NOT. I fear it will become increasingly brittle as FileCheck evolves.

Though I agree with the problem of special casing, I'd like to add that the disctinction between positive and negative match directives will be needed for supporting numeric substitution block using a variable defined on the same directive. Regarding CHECK-LABEL, can't we have a data structure that allow browsing annotation by start line/point and end line/point? Is the position of CHECK-LABEL annotations in the list of annotation an artifact of how CHECK-LABEL are processed or is there a need for those annotation to be before another directive starting on the next line?

What do you think? Looking at the implementation, I think search ranges would be easy to capture for errors.

So you'd show neighboring lines around search range start and end? That looks like a better characterisation of what this patch was trying to do so I agree it seems like a good idea.

<<<<<<
           .
           .
           .
          28: ....
          29: ....
          30: start
check:1       ^~~~~
check:2'0          [    error: no match found in search range starting here
          31: ....
          32: ....
          33: ....
           .
           .
           .
          87: ....
          88: ....
check:2'1         ] search range end
          89: end
check:3       ^~~
>>>>>>

Oops. This example is a bit broken. check:3 would have to be a label or else it wouldn't be processed due to the check:2 error.

What do you think? Looking at the implementation, I think search ranges would be easy to capture for errors.

Sorry for not spotting this question before. Your proposals seem okay to me,

Thanks for considering it.

but I'm not sure I personally have any particular useful opinions on the topic, as I tend not to use the --dump-input option for various reasons, which are outside the scope of this work.

I agree that should be a separate discussion, but I'd be interested to have it. Maybe it would lead to a better solution.

Though I agree with the problem of special casing, I'd like to add that the disctinction between positive and negative match directives will be needed for supporting numeric substitution block using a variable defined on the same directive.

I don't follow. I might have missed a discussion. Could you provide a quick example?

Regarding CHECK-LABEL, can't we have a data structure that allow browsing annotation by start line/point and end line/point? Is the position of CHECK-LABEL annotations in the list of annotation an artifact of how CHECK-LABEL are processed or is there a need for those annotation to be before another directive starting on the next line?

The former. D77607 caused this. I still feel like D77607 is generally right. The strange part to me is that a preceding directive's search range ends at the end of the CHECK-LABEL match. It seems like it ought to end at the start of the CHECK-LABEL match. -dump-input reveals that CHECK-LABEL behavior is odd:

$ cat input 
foo

$ cat check 
CHECK: foo
CHECK-LABEL: foo

$ FileCheck -vv check < input |& tail -6
<<<<<<
         1: foo
label:2     ^~~
check:1     ^~~
label:2        X error: no match found
>>>>>>

Per input line, annotations are sorted by time. The CHECK-LABEL matches first. Then the CHECK matches the same text. Then CHECK-LABEL runs again and fails. I suppose the reason for this behavior is to catch poorly chosen labels.

Hopefully you see the value of D77607's sort by time: it make the FileCheck behavior clearer even when that behavior is odd.

What do you think? Looking at the implementation, I think search ranges would be easy to capture for errors.

So you'd show neighboring lines around search range start and end?

They should be included via --dump-input-context=N... except sometimes in the bizarre case someone sets N=0, but I'm not too worried about that.

That looks like a better characterisation of what this patch was trying to do so I agree it seems like a good idea.

Thanks for the review. I'll work on this.

Though I agree with the problem of special casing, I'd like to add that the disctinction between positive and negative match directives will be needed for supporting numeric substitution block using a variable defined on the same directive.

I don't follow. I might have missed a discussion. Could you provide a quick example?

I was referring to https://lists.llvm.org/pipermail/llvm-dev/2020-June/142188.html . We already have a distinction between positive and negative match since we do PrintMatch or PrintNoMatch depending on that.

Regarding CHECK-LABEL, can't we have a data structure that allow browsing annotation by start line/point and end line/point? Is the position of CHECK-LABEL annotations in the list of annotation an artifact of how CHECK-LABEL are processed or is there a need for those annotation to be before another directive starting on the next line?

The former. D77607 caused this. I still feel like D77607 is generally right. The strange part to me is that a preceding directive's search range ends at the end of the CHECK-LABEL match. It seems like it ought to end at the start of the CHECK-LABEL match. -dump-input reveals that CHECK-LABEL behavior is odd:

$ cat input 
foo

$ cat check 
CHECK: foo
CHECK-LABEL: foo

$ FileCheck -vv check < input |& tail -6
<<<<<<
         1: foo
label:2     ^~~
check:1     ^~~
label:2        X error: no match found
>>>>>>

Per input line, annotations are sorted by time. The CHECK-LABEL matches first. Then the CHECK matches the same text. Then CHECK-LABEL runs again and fails. I suppose the reason for this behavior is to catch poorly chosen labels.

Hopefully you see the value of D77607's sort by time: it make the FileCheck behavior clearer even when that behavior is odd.

Ah yes that makes sense. Might be worth seaching why CHECK-NOT range ends at the end of the first line of the next CHECK-LABEL. Even if that were changed I do think focusing on search range rather than neighbouring annotations makes more sense.

Though I agree with the problem of special casing, I'd like to add that the disctinction between positive and negative match directives will be needed for supporting numeric substitution block using a variable defined on the same directive.

I don't follow. I might have missed a discussion. Could you provide a quick example?

I was referring to https://lists.llvm.org/pipermail/llvm-dev/2020-June/142188.html . We already have a distinction between positive and negative match since we do PrintMatch or PrintNoMatch depending on that.

Thanks for the link. I remember that discussion.

Is your point that there are multiple parts of FileCheck's implementation where we must distinguish between positive and negative matches, and so whatever solution we choose for the current issue may have to make that distinction as well? If so, point taken.

However, it seems we're in agreement that search ranges are the better approach anyway. I hope that approach will permit the -dump-input presentation layer (which I suppose is not well delineated) and FileCheck's matching logic to be less tightly coupled than the current patch requires.

Though I agree with the problem of special casing, I'd like to add that the disctinction between positive and negative match directives will be needed for supporting numeric substitution block using a variable defined on the same directive.

I don't follow. I might have missed a discussion. Could you provide a quick example?

I was referring to https://lists.llvm.org/pipermail/llvm-dev/2020-June/142188.html . We already have a distinction between positive and negative match since we do PrintMatch or PrintNoMatch depending on that.

Thanks for the link. I remember that discussion.

Is your point that there are multiple parts of FileCheck's implementation where we must distinguish between positive and negative matches, and so whatever solution we choose for the current issue may have to make that distinction as well? If so, point taken.

Not quite. My point is that you should not worry about having to distinguish between positive Vs negative for whatever solution to this problem because that distinction is going to be required for other features as well.

However, it seems we're in agreement that search ranges are the better approach anyway. I hope that approach will permit the -dump-input presentation layer (which I suppose is not well delineated) and FileCheck's matching logic to be less tightly coupled than the current patch requires.

I hope so too :-)

The strange part to me is that a preceding directive's search range ends at the end of the CHECK-LABEL match. It seems like it ought to end at the start of the CHECK-LABEL match.

I'd expect the behavior of CHECK-NOT ranges to work the same for CHECK-LABEL as for a regular CHECK.

(I deeply regret not following through on the FileCheck model stuff... will put it on my to-do list, again.)

jdenny added a comment.Mar 1 2021, 2:24 PM

The strange part to me is that a preceding directive's search range ends at the end of the CHECK-LABEL match. It seems like it ought to end at the start of the CHECK-LABEL match.

I'd expect the behavior of CHECK-NOT ranges to work the same for CHECK-LABEL as for a regular CHECK.

As far as I know, it does. However, before CHECK-LABEL, a CHECK-NOT range is not the same as a CHECK range:

$ cat input 
...
...
bar

$ cat check1
CHECK-NOT: foo
CHECK-LABEL: bar

$ cat check2
CHECK: bar
CHECK-LABEL: bar

$ FileCheck -dump-input=always -vv check1  < input |& tail -9
<<<<<<
           1: ...
not:1         X~~
           2: ...
not:1         ~~~
           3: bar
label:2'0     ^~~
label:2'1     ^~~
>>>>>>

$ FileCheck -vv check2  < input |& tail -8
<<<<<<
         1: ...
         2: ...
         3: bar
label:2     ^~~
check:1     ^~~
label:2        X error: no match found
>>>>>>

(I deeply regret not following through on the FileCheck model stuff... will put it on my to-do list, again.)

That discussion really helped me. It would be great to see it in the docs.

As far as I know, it does. However, before CHECK-LABEL, a CHECK-NOT range is not the same as a CHECK range:

So if you have

CHECK-NOT: bar
CHECK-LABEL: bar

it will invariably fail? (Trying to distinguish behavior of the directives from behavior of the dump output.)

jdenny added a comment.Mar 2 2021, 8:45 AM

As far as I know, it does. However, before CHECK-LABEL, a CHECK-NOT range is not the same as a CHECK range:

So if you have

CHECK-NOT: bar
CHECK-LABEL: bar

it will invariably fail? (Trying to distinguish behavior of the directives from behavior of the dump output.)

For the example input I gave, it does not fail. I have no reason to believe the input dump is misrepresenting anything. It's the FileCheck matching behavior that's strange: CHECK-NOT's search range before CHECK-LABEL does not overlap with the CHECK-LABEL match. CHECK's search range before CHECK-LABEL does overlap with the CHECK-LABEL match. At least in the experiments I've tried. I've not investigated the implementation to determine why.

thopre added a comment.Mar 2 2021, 9:04 AM

As far as I know, it does. However, before CHECK-LABEL, a CHECK-NOT range is not the same as a CHECK range:

So if you have

CHECK-NOT: bar
CHECK-LABEL: bar

it will invariably fail? (Trying to distinguish behavior of the directives from behavior of the dump output.)

For the example input I gave, it does not fail. I have no reason to believe the input dump is misrepresenting anything. It's the FileCheck matching behavior that's strange: CHECK-NOT's search range before CHECK-LABEL does not overlap with the CHECK-LABEL match. CHECK's search range before CHECK-LABEL does overlap with the CHECK-LABEL match. At least in the experiments I've tried. I've not investigated the implementation to determine why.

What does the label:2 X error: no match found mean then? I thought this was (despite the prefix) applying to the CHECK since the CHECK is searched after the LABEL.

jdenny added a comment.Mar 2 2021, 9:27 AM

What does the label:2 X error: no match found mean then? I thought this was (despite the prefix) applying to the CHECK since the CHECK is searched after the LABEL.

CHECK-LABEL runs twice: once before all other directives, and once with them. It's the second run that fails in the case you quoted.

jdenny planned changes to this revision.Mar 4 2021, 2:25 PM

As discussed, I'll try a different solution for this issue this patch addresses, probably in a different review.

jdenny abandoned this revision.May 31 2022, 7:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 7:35 AM
Herald added a subscriber: kosarev. · View Herald Transcript