Remove sub-expressions from parsing error #396
Conversation
|
@BurntSushi This PR is a follow up to our recent discussion on |
regex-syntax/src/lib.rs
Outdated
| @@ -1589,8 +1589,7 @@ impl fmt::Display for ErrorKind { | |||
| RepeaterExpectsExpr => | |||
| write!(f, "Missing expression for repetition operator."), | |||
| RepeaterUnexpectedExpr(ref e) => | |||
There was a problem hiding this comment.
I didn't remove the unused variable e because I assumed it can be put back into the error message again once the issues with the AST pretty printing are solved. But, if it makes sense, I can modify the code further to convert RepeaterUnexpectedExpr(Expr) to RepeaterUnexpectedExpr and get rid of the unused variable.
There was a problem hiding this comment.
@andrea-prearo Right, but this produces a compile warning, which causes the build to fail. You definitely don't want to change the error type itself, but using RepeaterUnexpectedExpr(_) is the right thing to do here.
There are other errors that try to print sub-expressions. Could you fix those too? e.g., InvalidClassEscape and maybe more.
There was a problem hiding this comment.
@BurntSushi I just noticed that warnings are treated as errors by looking at the CI log. I'll fix that by using RepeaterUnexpectedExpr(_) as you suggested. I'll also work on fixing other sub-expressions.
regex-syntax/src/lib.rs
Outdated
| @@ -1589,8 +1589,7 @@ impl fmt::Display for ErrorKind { | |||
| RepeaterExpectsExpr => | |||
| write!(f, "Missing expression for repetition operator."), | |||
| RepeaterUnexpectedExpr(ref e) => | |||
There was a problem hiding this comment.
@andrea-prearo Right, but this produces a compile warning, which causes the build to fail. You definitely don't want to change the error type itself, but using RepeaterUnexpectedExpr(_) is the right thing to do here.
There are other errors that try to print sub-expressions. Could you fix those too? e.g., InvalidClassEscape and maybe more.
|
I went through all the errors and didn't see other instances that try to print out sub-expressions. |
|
The build is failing on
|
|
@andrea-prearo Right, yeah, I'll submit a PR when I get a chance fixing that. |
|
@BurntSushi Sounds good. Thanks! |
|
@andrea-prearo Could you rebase this on master? That should hopefully get CI passing! |
andrea-prearo
force-pushed
the
error-message-sub-expressions
branch
from
047f5ec
to
ba5a3df
Compare
|
@BurntSushi I had issues with the rebase, so I had to do some clean up. But tests are passing now! |
|
@andrea-prearo Thanks! @bors r+ |
|
📌 Commit ba5a3df has been approved by |
…urntSushi Remove sub-expressions from parsing error This PR skips printing sub-expressions in the error message. This should avoid situations where the error message contains an AST with unreadable characters, as shown [here](BurntSushi/ripgrep#395). <img width="1030" alt="unreadable-chars" src="https://user-images.githubusercontent.com/12599697/29924193-02147d0c-8e11-11e7-8b36-a97b63f802b4.png">
|
☀️ Test successful - status-appveyor, status-travis |
This PR skips printing sub-expressions in the error message.
This should avoid situations where the error message contains an AST with unreadable characters, as shown here.