<regex>: Revise caret parsing in basic and grep mode

[muellerj2]

This PR revises how carets are parsed in regular expressions in basic and grep mode. Specifically:

• According to Sections 9.3.3 and 9.3.5 of the POSIX standard, [^a] should be the character class that does not contain a, but it is incorrectly parsed as the class that contains a and ^. This PR fixes this.
• While the POSIX standard leaves it to the implementation whether to treat carets at the beginning of subexpressions as caret anchors or ordinary characters (see Section 9.3.8), we now follow libstdc++ and Boost and consistently treat these carets as anchors. Note that libc++ interprets them as ordinary characters instead (as permitted by the standard as well). Furthermore, libstdc++ and Boost treat carets as anchors everywhere in basic regular expressions, which is incorrect according to the POSIX standard.
• Carets are also treated as anchors now in grep mode following a new line (which is the alternation operator). Similarly, it is now valid to use stars (*) following a new line/alternation (maybe after an optional caret).

As a bonus, _Parser::_Next() and _Parser::_Trans() no longer try to guess the parsing context from the state of the NFA to adjust the tokenization to the current context. This eliminates some brittle coupling between lexer, parser and NFA builder, such as the order dependency between NFA changes and tokenization results. This order dependency was also the subtle cause of the unexpected behavior change that previously led to this PR being dropped from a merge batch.

Correctness of recognizing caret anchor positions in basic regular expressions and in grep mode

The POSIX standard says that carets are to be treated as anchors at the beginning of a regular expression and may be treated so at the beginning of any subexpressions (which this PR opts to do in accordance with libstdc++ and Boost). But strictly speaking, this PR seems to actually implement something subtly different: The changed parser interprets carets as anchors when they appear at the beginning of an alternative in a disjunction. However, it turns out that this is actually equivalent:

• The beginning of the regular expression and the beginning of any subexpression is the beginning of the first alternative in a disjunction and vice versa. Thus, the implementation certainly treats all carets at the beginning of (sub-)expressions as an anchor in basic regular expressions.
• There is no alternation operator basic regular expressions, so there are no second, third or n-th alternatives. Moreover, the beginning of any first alternative is also the beginning of the regular expression or some subexpression. Thus, the beginning of some alternative, where carets are treated as anchors by this PR, is exactly the beginning of a regular expression and subexpression, where carets should be treated as anchors, and vice versa.
• There is a kind of alternation operator outside of subexpressions in grep mode, the new line. But grep is actually specified to parse each line individually as a basic regular expression (see the specification of grep's pattern_list argument in the POSIX standard), so a caret at the beginning of a line is to be treated as an anchor. Thus, carets at the beginning of a second, third or n-th alternative are anchors in grep mode. Hence, carets at the beginnings of any alternative should be treated as anchors in grep mode, which is what this PR implements.

The preceding argument essentially also applies to the treatment of initial stars as ordinary characters, but with the caveat that an optional caret anchor is allowed between the beginning of the alternative/expression/subexpression and the star.

[StephanTLavavej]

Thanks @muellerj2! 😻 After looking at this, I believe I found a much simpler way to fix this. Please double-check and meow if you think this is worse than your original fix.

[muellerj2]

@StephanTLavavej I think your fix is a suitable solution for ^.

But because of your remarks I noticed that ] is also not handled correctly at the beginning of a character class. It seems only libstdc++ interprets basic regular expressions like []-_] correctly (https://godbolt.org/z/4WE8PdGPE). I think this should be handled in a follow-up, though, because the fix seems to require a different approach.

Edit: Filed llvm/llvm-project#119432 about the mislanding of []-_] in libc++.

[StephanTLavavej]

This is introducing a behavior change that's causing failures in the internal perennial test suite. I haven't analyzed whether the product code is incorrect or whether the internal tests need to be updated. Due to time constraints before the holidays, I'm going to drop this PR from this merge batch and revisit it later.

D:\GitHub\STL\out\x64>type meow.cpp

#include <print>
#include <regex>
using namespace std;

void test(const char* const pattern, const char* const str) {
    const regex r{pattern, regex::basic};
    println("    pattern: {}", pattern);
    println("        str: {}", str);
    println("regex_match: {}", regex_match(str, r));
}

int main() {
    test(R"(0\(^a\))", "0^a");
}

D:\GitHub\STL\out\x64>cl /EHsc /nologo /W4 /std:c++latest /MTd /Od meow.cpp && meow
meow.cpp

Before:

    pattern: 0\(^a\)
        str: 0^a
regex_match: true

After:

    pattern: 0\(^a\)
        str: 0^a
regex_match: false

[muellerj2]

Something very strange is going on here: https://godbolt.org/z/GsfWEfbhE

All standard libraries disagree whether 0^a should be matched by 0\(^a\) or 0\(\(^a\)\). I'm not sure yet whether libstdc++ or libc++ is right here, but I'm certain that MSVC STL gets one of the match results wrong because there is no reasonable explanation why surrounding a capture group by another capture group should change the result.

I think this PR is correct, it just exposed some other bug in <regex> by chance.

[muellerj2]

I think I have grasped the underlying issue now, but I need maintainer direction to proceed because there are three possible solutions with different pros and cons.

1. Section 9.3.8 of the POSIX standard says that a caret at the beginning of a subexpression (capture or non-capture group) may be interpreted as an anchor or as an ordinary character.
2. Thus, both libc++ and libstdc++ conform to the POSIX standard, even though they behave completely differently: libc++ interprets a caret at the beginning of a subexpression is an ordinary character, while libstdc++ treats it as an anchor.
3. The test case is non-portable because it assumes that a caret at the beginning of a subexpression is treated as an ordinary character. To make it portable, the caret would have to be escaped by a backslash.
4. MSVC STL is peculiar: It treats a caret at the beginning of a subexpression as an ordinary character, but interprets it as an anchor if the subexpression in turn is also at the beginning of a subexpression. I don't think this violates the letter of the POSIX standard, but it is some very odd behavior. (I think the parser code clearly suggests that treatment as an anchor was intended, it's just that the implementation suffers from a bug.)
5. (Even weirder, MSVC STL always interprets the dollar sign as an ordinary character at the end of a subexpression, never as an anchor.)
6. This PR fixes the bug in the parser code as an unintended side effect: With this PR, _Parser::_Beg_expr() now returns the correct result because it is called after the node for the capture or non-capture group has been added to the NFA, while it was previously called before that node was created.

Given that the POSIX standard does not mandate how a caret at the beginning of a subexpression is to be handled, we have three options how to proceed:

1. Always treat a caret as an ordinary character, aligning with libc++. This is unlikely to break users and leads to consistent treatment of carets, but restricts the expressiveness of the regular expressions more than the other options.
2. Always treat a caret as an anchor, aligning with libstdc++. This is probably the option to most likely break some users, but yields consistent treatment of carets and greater expressiveness of regular expressions.
3. Retain the old behavior. This won't break any users and is as expressive as always treating a caret as an anchor (with some weird rules attached to it), but the treatment of carets is just plain weird.

Which option do you consider appropriate? I'm willing to make the changes for any of them. (Even if we go for option 2, we should add tests to make sure we don't regress this behavior in the future. And I think there is still a minor bug in the handling of carets at the beginning of subexpressions, but it can be fixed by changing a single line.)

[CaseyCarter]

I suppose our current behavior is the result of someone thinking that caret should be an anchor when it could possibly match at the beginning of a string (e.g., \(\(^x\)|\(123\)\)), and not an anchor when it could not possibly match (0\(^a\)). I agree that this context-sensitivity seems like the worst possible choice when it comes to local reasoning and composability. I hate to break the poor hardy souls who use our <regex>, but making our behavior consistent seems to be for the best in the long run.

How does Boost handle caret at the beginning of a subexpression? If Boost and libc++ both agree, it seems like a no-brainer for us to be consistent. Even if they don't, I feel like expressions are more likely to migrate between our implementation and Boost's than ours and another standard library's. Consistency with Boost - which we've been recommending over our ABI-locked-in-a-very-poor-state implementation for a while - is probably the way to go.

I think I've convinced myself enough to remove the decision needed We need to choose something before working on this label. Contributors who disagree will have plenty of opportunity to comment on this PR.

[CaseyCarter]

This is introducing a behavior change that's causing failures in the internal perennial test suite. I haven't analyzed whether the product code is incorrect or whether the internal tests need to be updated.

How bad could it be? (Does a quick search of Perennial.) SIXTY occurrences??!? Oh my.

[StephanTLavavej]

basic/grep usage should be very rare, so I'm not too concerned about behavioral changes, and I'm willing to fix up Perennial if that's the only impact. (It's less annoying than fixing RWC.)

I agree with Casey that we should figure out what Boost does and if it's moderately sensible, align with it.

[muellerj2]

Boost treats carets as anchors.

But it seems both libstdc++ and Boost treat carets (and dollar signs) as anchors everywhere in a basic regular expression, contrary to the POSIX standard. Example: https://godbolt.org/z/b1q5Pefqn
Regular expression \(^^a\) should not match a, but libstdc++ and Boost claim it does.
Regular expression 0^a should match 0^a, but libstdc++ and Boost fail to match.

I think we should comply with the POSIX standard when users are explicitly asking for POSIX regex rules, so I believe MSVC STL shouldn't treat carets as anchors everywhere like libstdc++ and Boost do. But we come closest to Boost's behavior while being standard-compliant if carets are at least interpreted as anchors at the beginning of subexpressions. So I suggest we implement this behavior (and do the same for dollar signs in a follow-up).

After STL's changes, this behavior is already implemented by this PR anyway, except for a (pre-existing) miscompilation of double carets. But it's easy to fix this miscompilation.

[muellerj2]

I implemented the necessary changes to consistently treat carets as anchors at the beginning of subexpressions.

I also eliminated that weird and brittle dependency of the lexer and the parser on the NFA that they used to guess the parsing context of the caret (and the star). This dependency was central for the behavior change we failed to notice. Now lexer and parser operations no longer depend on how they are sequenced compared to operations performed on the NFA.

I repaired the double caret miscompilation, too. Finally, I noticed that the regex parser fails to treat carets after new lines as anchors in grep mode, so I fixed this as well.

I adjusted the title of the PR a bit because it includes much more now than just a fix for the negation of character classes.

[CaseyCarter]

I think we should comply with the POSIX standard when users are explicitly asking for POSIX regex rules, so I believe MSVC STL shouldn't treat carets as anchors everywhere like libstdc++ and Boost do.

+100 yes, our intent is that we should first conform to the spec, and only where the spec leaves things up to interpretation should we try to do as Boost does.