Surprising precedence when stringifying concatenations of alternations #516
Labels
Comments
|
Hmm, yeah, I agree, that does not look right to me. Looking at the HIR, this looks like it might be a bug in the printer because the HIR looks right to me: https://play.rust-lang.org/?gist=cea5cb88a7985fc36a94e73bcc221c7a&version=stable&mode=debug&edition=2015 It seems plausible that the printer is probably resisting putting a non-capturing group into the concrete syntax that does not itself exist in the HIR. So the question here is whether the HIR construction should be updated to insert a group itself or if the printer should recognize that When faced with these decisions, I think I've generally leaned towards fixing this at construction time, such that certain combinations of HIR are never possible. For example, it should be impossible to get a |
|
For what it's worth I've encountered this when playing around with regex
syntax and found the behavior surprising. My assumption was that the
printer would take care of extra noncaptapturing groups. When that didn't
happen I just injected a bunch of extra noncapturing groups at construction
time. For my use case (dynamically assembling arbitrarily user supplied
regex) I don't think those groups were always required. If consideration of
groups gets pushed to construction time, it would be nice to help the
caller avoid these extra groups by handling them automatically or something.
On Sep 18, 2018 6:50 AM, "Andrew Gallant" <notifications@github.com> wrote:
Hmm, yeah, I agree, that does not look right to me. Looking at the HIR,
this looks like it might be a bug in the printer because the HIR looks
right to me:
https://play.rust-lang.org/?gist=cea5cb88a7985fc36a94e73bcc221c7a&version=stable&mode=debug&edition=2015
It seems plausible that the printer is probably resisting putting a
non-capturing group into the concrete syntax that does not itself exist in
the HIR. So the question here is whether the HIR construction should be
updated to insert a group itself or if the printer should recognize that
Hir::Something followed by a Hir::Alternation requires a non-capturing
group.
When faced with these decisions, I think I've generally leaned towards
fixing this at construction time, such that certain combinations of HIR are
never possible. For example, it should be impossible to get a Concat(Expr)
since it is always equivalent to just Expr.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#516 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEGx5f5jPennIQD5gllnvJxw5W7uEIm1ks5ucNAIgaJpZM4WtSxN>
.
|
|
I have a fix incoming that will land as part of the #656 work. Here's the commit message that explains everything and my fix. TL;DR - I tweaked the printer. |
BurntSushi
added a commit
that referenced
this issue
Sep 20, 2022
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Sep 25, 2022
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Nov 5, 2022
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Jan 6, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Jan 13, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Mar 2, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Mar 4, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Mar 5, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Mar 15, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Mar 21, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Apr 15, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Apr 17, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Apr 17, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Apr 17, 2023
This fixes some corner cases in the HIR printer where it would print the
concrete syntax of a regex that does not match the natural
interpretation of the HIR. One such example of this is:
concat(a, alt(b, c))
This would get printed as
ab|c
But clearly, it should be printed as:
a(?:b|c)
The issue here is that the printer only considers the current HirKind
when determining how to print it. Sometimes a group is needed to print
an alt (and even a concat, in the case of 'rep(+, concat(a, b))'), but
sometimes it isn't.
We could address this in a few different ways:
1) Always print concats and alts inside a non-capturing group.
2) Make the printer detect precisely the cases where a non-capturing
group is needed.
3) Make the HIR smart constructors insert non-capturing groups when
needed.
4) Do some other thing to change the HIR to prevent these sorts of
things by construction.
This patch goes with (1). The reason in favor of it is that HIR printer
was always about printing an equivalent regex and never about trying to
print a "nice" regex. Indeed, the HIR printer can't print a nice regex,
because the HIR represents a rigorously simplifed view of a regex to
make analysis easier. (The most obvious such example are Unicode
character classes. For example, the HIR printer never prints '\w'.) So
inserting some extra groups (which it already does) even when they
aren't strictly needed is perfectly okay.
But still, it's useful to say why we didn't do the other choices:
2) Modifying the printer to only print groups when they're actually
needed is pretty difficult. I tried this briefly, and handling this
case requires some notion of what the parent expression is. This
winds up being a possible but hairy change.
3) Making the HIR more complicated to make the printer correct seems
like it's optimizing for the wrong thing. Inserting extra groups in
places just obfuscates HIR values that already have clear semantics.
That is, use concat(a, alt(b, c)) over concat(a, group(alt(b, c))).
4) It's not clear how we would change the HIR to guarantee this sort of
thing wouldn't happen. At the very least, it seems likely it would
require a more complex data type.
At first, I had thought (1) seemed inelegant. But the more I thought
about it, the more it seemed quite consistent with how the HIR printer
already worked. So that's the path I took here.
Closes #516, Closes #731
BurntSushi
added a commit
that referenced
this issue
Apr 20, 2023
1.8.0 (2023-04-20) ================== This is a sizeable release that will be soon followed by another sizeable release. Both of them will combined close over 40 existing issues and PRs. This first release, despite its size, essentially represent preparatory work for the second release, which will be even bigger. Namely, this release: * Increases the MSRV to Rust 1.60.0, which was released about 1 year ago. * Upgrades its dependency on `aho-corasick` to the recently release 1.0 version. * Upgrades its dependency on `regex-syntax` to the simultaneously released `0.7` version. The changes to `regex-syntax` principally revolve around a rewrite of its literal extraction code and a number of simplifications and optimizations to its high-level intermediate representation (HIR). The second release, which will follow ~shortly after the release above, will contain a soup-to-nuts rewrite of every regex engine. This will be done by bringing [`regex-automata`](https://github.com/BurntSushi/regex-automata) into this repository, and then changing the `regex` crate to be nothing but an API shim layer on top of `regex-automata`'s API. These tandem releases are the culmination of about 3 years of on-and-off work that [began in earnest in March 2020](#656). Because of the scale of changes involved in these releases, I would love to hear about your experience. Especially if you notice undocumented changes in behavior or performance changes (positive *or* negative). Most changes in the first release are listed below. For more details, please see the commit log, which reflects a linear and decently documented history of all changes. New features: * [FEATURE #501](#501): Permit many more characters to be escaped, even if they have no significance. More specifically, any ASCII character except for `[0-9A-Za-z<>]` can now be escaped. Also, a new routine, `is_escapeable_character`, has been added to `regex-syntax` to query whether a character is escapeable or not. * [FEATURE #547](#547): Add `Regex::captures_at`. This filles a hole in the API, but doesn't otherwise introduce any new expressive power. * [FEATURE #595](#595): Capture group names are now Unicode-aware. They can now begin with either a `_` or any "alphabetic" codepoint. After the first codepoint, subsequent codepoints can be any sequence of alpha-numeric codepoints, along with `_`, `.`, `[` and `]`. Note that replacement syntax has not changed. * [FEATURE #810](#810): Add `Match::is_empty` and `Match::len` APIs. * [FEATURE #905](#905): Add an `impl Default for RegexSet`, with the default being the empty set. * [FEATURE #908](#908): A new method, `Regex::static_captures_len`, has been added which returns the number of capture groups in the pattern if and only if every possible match always contains the same number of matching groups. * [FEATURE #955](#955): Named captures can now be written as `(?<name>re)` in addition to `(?P<name>re)`. * FEATURE: `regex-syntax` now supports empty character classes. * FEATURE: `regex-syntax` now has an optional `std` feature. (This will come to `regex` in the second release.) * FEATURE: The `Hir` type in `regex-syntax` has had a number of simplifications made to it. * FEATURE: `regex-syntax` has support for a new `R` flag for enabling CRLF mode. This will be supported in `regex` proper in the second release. * FEATURE: `regex-syntax` now has proper support for "regex that never matches" via `Hir::fail()`. * FEATURE: The `hir::literal` module of `regex-syntax` has been completely re-worked. It now has more documentation, examples and advice. * FEATURE: The `allow_invalid_utf8` option in `regex-syntax` has been renamed to `utf8`, and the meaning of the boolean has been flipped. Performance improvements: * PERF: The upgrade to `aho-corasick 1.0` may improve performance in some cases. It's difficult to characterize exactly which patterns this might impact, but if there are a small number of longish (>= 4 bytes) prefix literals, then it might be faster than before. Bug fixes: * [BUG #514](#514): Improve `Debug` impl for `Match` so that it doesn't show the entire haystack. * BUGS [#516](#516), [#731](#731): Fix a number of issues with printing `Hir` values as regex patterns. * [BUG #610](#610): Add explicit example of `foo|bar` in the regex syntax docs. * [BUG #625](#625): Clarify that `SetMatches::len` does not (regretably) refer to the number of matches in the set. * [BUG #660](#660): Clarify "verbose mode" in regex syntax documentation. * BUG [#738](#738), [#950](#950): Fix `CaptureLocations::get` so that it never panics. * [BUG #747](#747): Clarify documentation for `Regex::shortest_match`. * [BUG #835](#835): Fix `\p{Sc}` so that it is equivalent to `\p{Currency_Symbol}`. * [BUG #846](#846): Add more clarifying documentation to the `CompiledTooBig` error variant. * [BUG #854](#854): Clarify that `regex::Regex` searches as if the haystack is a sequence of Unicode scalar values. * [BUG #884](#884): Replace `__Nonexhaustive` variants with `#[non_exhaustive]` attribute. * [BUG #893](#893): Optimize case folding since it can get quite slow in some pathological cases. * [BUG #895](#895): Reject `(?-u:\W)` in `regex::Regex` APIs. * [BUG #942](#942): Add a missing `void` keyword to indicate "no parameters" in C API. * [BUG #965](#965): Fix `\p{Lc}` so that it is equivalent to `\p{Cased_Letter}`. * [BUG #975](#975): Clarify documentation for `\pX` syntax.
Merged
BurntSushi
added a commit
that referenced
this issue
Apr 20, 2023
1.8.0 (2023-04-20) ================== This is a sizeable release that will be soon followed by another sizeable release. Both of them will combined close over 40 existing issues and PRs. This first release, despite its size, essentially represent preparatory work for the second release, which will be even bigger. Namely, this release: * Increases the MSRV to Rust 1.60.0, which was released about 1 year ago. * Upgrades its dependency on `aho-corasick` to the recently release 1.0 version. * Upgrades its dependency on `regex-syntax` to the simultaneously released `0.7` version. The changes to `regex-syntax` principally revolve around a rewrite of its literal extraction code and a number of simplifications and optimizations to its high-level intermediate representation (HIR). The second release, which will follow ~shortly after the release above, will contain a soup-to-nuts rewrite of every regex engine. This will be done by bringing [`regex-automata`](https://github.com/BurntSushi/regex-automata) into this repository, and then changing the `regex` crate to be nothing but an API shim layer on top of `regex-automata`'s API. These tandem releases are the culmination of about 3 years of on-and-off work that [began in earnest in March 2020](#656). Because of the scale of changes involved in these releases, I would love to hear about your experience. Especially if you notice undocumented changes in behavior or performance changes (positive *or* negative). Most changes in the first release are listed below. For more details, please see the commit log, which reflects a linear and decently documented history of all changes. New features: * [FEATURE #501](#501): Permit many more characters to be escaped, even if they have no significance. More specifically, any ASCII character except for `[0-9A-Za-z<>]` can now be escaped. Also, a new routine, `is_escapeable_character`, has been added to `regex-syntax` to query whether a character is escapeable or not. * [FEATURE #547](#547): Add `Regex::captures_at`. This filles a hole in the API, but doesn't otherwise introduce any new expressive power. * [FEATURE #595](#595): Capture group names are now Unicode-aware. They can now begin with either a `_` or any "alphabetic" codepoint. After the first codepoint, subsequent codepoints can be any sequence of alpha-numeric codepoints, along with `_`, `.`, `[` and `]`. Note that replacement syntax has not changed. * [FEATURE #810](#810): Add `Match::is_empty` and `Match::len` APIs. * [FEATURE #905](#905): Add an `impl Default for RegexSet`, with the default being the empty set. * [FEATURE #908](#908): A new method, `Regex::static_captures_len`, has been added which returns the number of capture groups in the pattern if and only if every possible match always contains the same number of matching groups. * [FEATURE #955](#955): Named captures can now be written as `(?<name>re)` in addition to `(?P<name>re)`. * FEATURE: `regex-syntax` now supports empty character classes. * FEATURE: `regex-syntax` now has an optional `std` feature. (This will come to `regex` in the second release.) * FEATURE: The `Hir` type in `regex-syntax` has had a number of simplifications made to it. * FEATURE: `regex-syntax` has support for a new `R` flag for enabling CRLF mode. This will be supported in `regex` proper in the second release. * FEATURE: `regex-syntax` now has proper support for "regex that never matches" via `Hir::fail()`. * FEATURE: The `hir::literal` module of `regex-syntax` has been completely re-worked. It now has more documentation, examples and advice. * FEATURE: The `allow_invalid_utf8` option in `regex-syntax` has been renamed to `utf8`, and the meaning of the boolean has been flipped. Performance improvements: * PERF: The upgrade to `aho-corasick 1.0` may improve performance in some cases. It's difficult to characterize exactly which patterns this might impact, but if there are a small number of longish (>= 4 bytes) prefix literals, then it might be faster than before. Bug fixes: * [BUG #514](#514): Improve `Debug` impl for `Match` so that it doesn't show the entire haystack. * BUGS [#516](#516), [#731](#731): Fix a number of issues with printing `Hir` values as regex patterns. * [BUG #610](#610): Add explicit example of `foo|bar` in the regex syntax docs. * [BUG #625](#625): Clarify that `SetMatches::len` does not (regretably) refer to the number of matches in the set. * [BUG #660](#660): Clarify "verbose mode" in regex syntax documentation. * BUG [#738](#738), [#950](#950): Fix `CaptureLocations::get` so that it never panics. * [BUG #747](#747): Clarify documentation for `Regex::shortest_match`. * [BUG #835](#835): Fix `\p{Sc}` so that it is equivalent to `\p{Currency_Symbol}`. * [BUG #846](#846): Add more clarifying documentation to the `CompiledTooBig` error variant. * [BUG #854](#854): Clarify that `regex::Regex` searches as if the haystack is a sequence of Unicode scalar values. * [BUG #884](#884): Replace `__Nonexhaustive` variants with `#[non_exhaustive]` attribute. * [BUG #893](#893): Optimize case folding since it can get quite slow in some pathological cases. * [BUG #895](#895): Reject `(?-u:\W)` in `regex::Regex` APIs. * [BUG #942](#942): Add a missing `void` keyword to indicate "no parameters" in C API. * [BUG #965](#965): Fix `\p{Lc}` so that it is equivalent to `\p{Cased_Letter}`. * [BUG #975](#975): Clarify documentation for `\pX` syntax.
crapStone
added a commit
to Calciumdibromid/CaBr2
that referenced
this issue
May 2, 2023
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [regex](https://github.com/rust-lang/regex) | dependencies | minor | `1.7.3` -> `1.8.1` | --- ### Release Notes <details> <summary>rust-lang/regex</summary> ### [`v1.8.1`](https://github.com/rust-lang/regex/blob/HEAD/CHANGELOG.md#​181-2023-04-21) \================== This is a patch release that fixes a bug where a regex match could be reported where none was found. Specifically, the bug occurs when a pattern contains some literal prefixes that could be extracted *and* an optional word boundary in the prefix. Bug fixes: - [BUG #​981](rust-lang/regex#981): Fix a bug where a word boundary could interact with prefix literal optimizations and lead to a false positive match. ### [`v1.8.0`](https://github.com/rust-lang/regex/blob/HEAD/CHANGELOG.md#​180-2023-04-20) \================== This is a sizeable release that will be soon followed by another sizeable release. Both of them will combined close over 40 existing issues and PRs. This first release, despite its size, essentially represents preparatory work for the second release, which will be even bigger. Namely, this release: - Increases the MSRV to Rust 1.60.0, which was released about 1 year ago. - Upgrades its dependency on `aho-corasick` to the recently released 1.0 version. - Upgrades its dependency on `regex-syntax` to the simultaneously released `0.7` version. The changes to `regex-syntax` principally revolve around a rewrite of its literal extraction code and a number of simplifications and optimizations to its high-level intermediate representation (HIR). The second release, which will follow ~shortly after the release above, will contain a soup-to-nuts rewrite of every regex engine. This will be done by bringing [`regex-automata`](https://github.com/BurntSushi/regex-automata) into this repository, and then changing the `regex` crate to be nothing but an API shim layer on top of `regex-automata`'s API. These tandem releases are the culmination of about 3 years of on-and-off work that [began in earnest in March 2020](rust-lang/regex#656). Because of the scale of changes involved in these releases, I would love to hear about your experience. Especially if you notice undocumented changes in behavior or performance changes (positive *or* negative). Most changes in the first release are listed below. For more details, please see the commit log, which reflects a linear and decently documented history of all changes. New features: - [FEATURE #​501](rust-lang/regex#501): Permit many more characters to be escaped, even if they have no significance. More specifically, any ASCII character except for `[0-9A-Za-z<>]` can now be escaped. Also, a new routine, `is_escapeable_character`, has been added to `regex-syntax` to query whether a character is escapeable or not. - [FEATURE #​547](rust-lang/regex#547): Add `Regex::captures_at`. This filles a hole in the API, but doesn't otherwise introduce any new expressive power. - [FEATURE #​595](rust-lang/regex#595): Capture group names are now Unicode-aware. They can now begin with either a `_` or any "alphabetic" codepoint. After the first codepoint, subsequent codepoints can be any sequence of alpha-numeric codepoints, along with `_`, `.`, `[` and `]`. Note that replacement syntax has not changed. - [FEATURE #​810](rust-lang/regex#810): Add `Match::is_empty` and `Match::len` APIs. - [FEATURE #​905](rust-lang/regex#905): Add an `impl Default for RegexSet`, with the default being the empty set. - [FEATURE #​908](rust-lang/regex#908): A new method, `Regex::static_captures_len`, has been added which returns the number of capture groups in the pattern if and only if every possible match always contains the same number of matching groups. - [FEATURE #​955](rust-lang/regex#955): Named captures can now be written as `(?<name>re)` in addition to `(?P<name>re)`. - FEATURE: `regex-syntax` now supports empty character classes. - FEATURE: `regex-syntax` now has an optional `std` feature. (This will come to `regex` in the second release.) - FEATURE: The `Hir` type in `regex-syntax` has had a number of simplifications made to it. - FEATURE: `regex-syntax` has support for a new `R` flag for enabling CRLF mode. This will be supported in `regex` proper in the second release. - FEATURE: `regex-syntax` now has proper support for "regex that never matches" via `Hir::fail()`. - FEATURE: The `hir::literal` module of `regex-syntax` has been completely re-worked. It now has more documentation, examples and advice. - FEATURE: The `allow_invalid_utf8` option in `regex-syntax` has been renamed to `utf8`, and the meaning of the boolean has been flipped. Performance improvements: - PERF: The upgrade to `aho-corasick 1.0` may improve performance in some cases. It's difficult to characterize exactly which patterns this might impact, but if there are a small number of longish (>= 4 bytes) prefix literals, then it might be faster than before. Bug fixes: - [BUG #​514](rust-lang/regex#514): Improve `Debug` impl for `Match` so that it doesn't show the entire haystack. - BUGS [#​516](rust-lang/regex#516), [#​731](rust-lang/regex#731): Fix a number of issues with printing `Hir` values as regex patterns. - [BUG #​610](rust-lang/regex#610): Add explicit example of `foo|bar` in the regex syntax docs. - [BUG #​625](rust-lang/regex#625): Clarify that `SetMatches::len` does not (regretably) refer to the number of matches in the set. - [BUG #​660](rust-lang/regex#660): Clarify "verbose mode" in regex syntax documentation. - BUG [#​738](rust-lang/regex#738), [#​950](rust-lang/regex#950): Fix `CaptureLocations::get` so that it never panics. - [BUG #​747](rust-lang/regex#747): Clarify documentation for `Regex::shortest_match`. - [BUG #​835](rust-lang/regex#835): Fix `\p{Sc}` so that it is equivalent to `\p{Currency_Symbol}`. - [BUG #​846](rust-lang/regex#846): Add more clarifying documentation to the `CompiledTooBig` error variant. - [BUG #​854](rust-lang/regex#854): Clarify that `regex::Regex` searches as if the haystack is a sequence of Unicode scalar values. - [BUG #​884](rust-lang/regex#884): Replace `__Nonexhaustive` variants with `#[non_exhaustive]` attribute. - [BUG #​893](rust-lang/regex#893): Optimize case folding since it can get quite slow in some pathological cases. - [BUG #​895](rust-lang/regex#895): Reject `(?-u:\W)` in `regex::Regex` APIs. - [BUG #​942](rust-lang/regex#942): Add a missing `void` keyword to indicate "no parameters" in C API. - [BUG #​965](rust-lang/regex#965): Fix `\p{Lc}` so that it is equivalent to `\p{Cased_Letter}`. - [BUG #​975](rust-lang/regex#975): Clarify documentation for `\pX` syntax. </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS42MS4wIiwidXBkYXRlZEluVmVyIjoiMzUuNjYuMyIsInRhcmdldEJyYW5jaCI6ImRldmVsb3AifQ==--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Co-authored-by: crapStone <crapstone01@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1874 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In the
hirmodule of theregex-syntaxcrate, concatenating onto an alternation and callingto_stringon the resulting concatenation currently produces a pattern string where the concatenation binds more tightly than the alternation. This is most easily understood with an example: https://play.rust-lang.org/?gist=36767c381c97f169e219c8c707c001ad&version=stable&mode=debug&edition=2015I don't know if this current behavior is intended or not, but I originally expected that the pattern string printed in the example above would look something like
A(?:B|C), notAB|C, as the concatenation is logically the root of the HIR tree. Would it be possible to at least document this behavior?The text was updated successfully, but these errors were encountered: