-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Remove nonsentical no-match CFG edges #18820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dca4d21
to
0f34c22
Compare
0f34c22
to
4a01a4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for doing this. Some small suggestions.
* If this pattern is immediately nested within another pattern, then get the | ||
* parent pattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* If this pattern is immediately nested within another pattern, then get the | |
* parent pattern. | |
* Gets the pattern under which this pattern is immediately nested, if any. |
// Identifier patterns that are in fact path patterns can cause failures. For | ||
// instance `None`. Only if a `@ ...` part is present can we be sure that it's | ||
// an actual identifier pattern. | ||
pat = any(IdentPat p | not p.hasPat()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we reintroduce the and not p = any(Variable v).getPat()
constraint? I realize that the variable logic makes its own assumptions about what might be a variable binding or path pattern though...
pat = parent.(TuplePat).getAField() | ||
// NOTE: a `TupleStructPat` can cause a failure if it resolves to a an enum | ||
// variant but not when it resolves to a tuple struct. | ||
pat instanceof TupleStructPat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RecordPat
also needs to be added; enum variants are allowed to use record fields.
* Holds if `pat` can not _itself_ be the cause of a pattern match failure. This | ||
* does not mean that `pat` is irrefutable, as its children might be the cause | ||
* of a failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This QL doc seems to suggest the dual of what the predicate actually computes? I wonder if it is actually better to implement what the QL doc says, e.g.
private predicate cannotCauseMatchFailure(Pat pat) {
pat = any(IdentPat p | p.hasPat())
or
pat instanceof BoxPat
or
pat instanceof RestPat
or
pat instanceof MacroPat
or
pat instanceof WildcardPat
or
pat instanceof RangePat
or
pat instanceof RefPat
or
pat instanceof SlicePat
or
pat instanceof TuplePat
or
pat instanceof ConstBlockPat
}
and then replace not canCauseMatchFailure(pat)
with cannotCauseMatchFailure(pat)
further down.
Cleans up the handling of patterns that can't fail and makes it more precise such that no-match edges that will never be taken are removed.
The added tests also exposed a bug for macro patterns. I haven't fixed that as it's orthogonal to this PR.
Here's an example. Previously we had a
no-match
edge out ofMyStruct {...}
but that is now gone as the struct pattern itself will never cause a match failure: