[PATCH] D58784: [FileCheck]Remove assertions that prevent matching an empty string at file start before CHECK-NEXT/SAME

Fri Mar 1 09:56:50 PST 2019

jdenny added a comment.

In D58784#1414631 <https://reviews.llvm.org/D58784#1414631>, @probinson wrote:

> In D58784#1414574 <https://reviews.llvm.org/D58784#1414574>, @jdenny wrote:
>
> > Here's how I see it, but I realize it's subjective.  If we say that an initial CHECK-SAME matches the first line, then, to be consistent with the relationship CHECK-SAME and CHECK-NEXT have everywhere else, an initial CHECK-NEXT should skip a new line and thus match the second line.  That means that, in order to match consecutive lines starting with the first line, we have:
> >
> >   CHECK-SAME: first line
> >   CHECK-NEXT: second line
> >   CHECK-NEXT: third line
> >
>
>
> That matches my intuition.
>
> > But the following seems more intuitive to me:
> > 
> >   CHECK-NEXT: first line
> >   CHECK-NEXT: second line
> >   CHECK-NEXT: third line
> > 
> > 
> > To say it more abstractly:
> > 
> > - CHECK-NEXT means match on the next line after the previous match.
> > - CHECK-SAME means match on the same line as the previous match.
> > 
> >   If there is no previous match because this is the first directive, then that translates to:
> > - CHECK-NEXT means match on the next line after no match yet, and that intuitively sounds like the first line to me.
> > - CHECK-SAME means match on the same line as no match yet, but that sounds impossible to me.
>
> I would say it this way:
>
> - CHECK-NEXT means match on the next line after the starting point
> - CHECK-SAME means match on the same line as the starting point
>
>   which is more in line with the search-range model.

You feel an initial CHECK-NEXT should target the second line, right?  I agree that is more in line with the formal model we have now: CHECK-NEXT requires a single newline to appear in the search range before the match.

By that reasoning, an initial CHECK-EMPTY should also target the second line: CHECK-EMPTY skips a newline in the search range before trying to match an empty line.  Perhaps I misunderstood you, but I don't think that matches the intuition you expressed in your first post.  Moreover, it feels unintuitive to me that you must use `CHECK-SAME: {{^$}}`` not CHECK-EMPTY to check for an empty first line.

And what happens after CHECK-LABEL?  CHECK-SAME matches on the line of the label, but CHECK-NEXT/CHECK-EMPTY requires/skips a newline before the match, so they target the first line after the label.  All that's true upstream already.

It seems conceptually inconsistent to me that CHECK-NEXT/CHECK-EMPTY should target the first line after a label but the second line at the beginning of the input.  I also recall your formal model says there's an implicit label at the beginning of the input, and that highlights this inconsistency more in my mind.

Again, this is a conceptual inconsistency, and I understand that it's actually consistent with the formal model.  What I'm suggesting is that these directives are conceptually defined (based on their names) in terms of the previous match, so we should adjust the formal model to make sense for the case when there is no previous match (or the previous match is an implicit label before the first line): CHECK-SAME is then undefined, and CHECK-NEXT/CHECK-EMPTY don't require/skip a newline in that case because the next line after no matches is the first line.

In D58784#1414728 <https://reviews.llvm.org/D58784#1414728>, @thopre wrote:

> If we are talking about matching a specific line (in this case the first line), I wonder if we could not extend the syntax of directives to accept a line number, e.g. CHECK-EMPTY-L1. Or if we think only the first line deserve to be checked explicitely simply a CHECK-FIRST. The fact that we are arguing about what would the meaning of CHECK-SAME and CHECK-NEXT on the first line be suggests that it would confuse lots of users. After all you are the most familiar people with that piece of software.

You might be right that, to avoid confusion, we need another solution altogether.  I assume your proposed directives would report errors if the previous match was already past the specified line.  What would be the semantics after a CHECK-LABEL or CHECK-DAG?  Do we use CHECK-FIRST at the beginning of the input but CHECK-NEXT after a CHECK-LABEL?

In D58784#1414755 <https://reviews.llvm.org/D58784#1414755>, @jhenderson wrote:

> I don't think it's wise to allow CHECK-SAME or CHECK-NEXT as the first match, due to the ambiguity that might cause as you two have been discussing. I don't have an intuitive idea of which it should be, if I'm honest.

One nice property of my interpretation is that an initial CHECK-SAME is an error.  Thus, if we implement that but a user assumes Paul's interpretation of an initial CHECK-SAME (and I can definitely foresee users doing that), FileCheck must always supply a diagnostic, and that diagnostic could suggest that they actually want CHECK-NEXT.  The confusion is easily resolved.

The trouble with implementing my interpretation would be if a user assumes Paul's interpretation of an initial CHECK-NEXT/CHECK-EMPTY.  But how often do people actually want to ignore the first line of input and demand the initial match be specifically on line two?  It seems like an unlikely use case to me, and it seems even more unlikely people would try to do this with an initial CHECK-NEXT or CHECK-EMPTY directive, so I'm not sure there's much potential for confusion.  But it's hard to know for sure.

> @probinson/@jdenny, can I just confirm that you're happy with this patch going in? I just want to make sure given the chatter re. CHECK-NEXT/CHECK-SAME...

I'm afraid I haven't examined the history of those asserts carefully to determine if there's a correct version to replace them.  I probably need to first understand what semantics we want for these initial CHECK-{NEXT,SAME,EMPTY} directives.  So, for now, I'm mostly neutral about your patch but lean toward pushing it if the asserts are causing you trouble.  Of course, we should also see what Paul thinks.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58784/new/

https://reviews.llvm.org/D58784