Improve handling of trailing optional inputs in pattern matching #1948
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@justinchuby re. our discussion |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
❌ 14 Tests Failed:
View the full list of 3 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
| f"Input nums mismatch. {len(node.inputs)} vs {len(pattern_node.inputs)}" | ||
| ) | ||
| # Inputs are padded with Nones to match against pattern | ||
| to_match = itertools.zip_longest(node.inputs, pattern_node.inputs, fillvalue=None) |
Check failure
Code scanning / lintrunner
MYPY/assignment Error
|
I think most of the time a user would want Non-Nones as default. I agree having to check for None for every input can be tedious. Given this I think specifically declaring something can be None is more intentional and less error prone. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pattern-variables are allowed in places where an optional input is expected. If the input is present, the pattern-variable will be bound to the corresponding ir.Value. Otherwise, the pattern-variable will be bound to None.
This PR extends this convention to handle trailing optional inputs better: specifically,
SomeOp(x)andSomeOp(x, None)are treated equivalently during the pattern-match. If the pattern isSomeOp(pattern_x, pattern_y), thenpattern_ywill be bound to None in both cases, and the pattern-match will succeed.Side note: A consequence of allowing pattern-variables to be bound to None is that the corresponding guard/rewrite function will have to handle the case where the pattern-variable is None correctly. This is not an issue for correctly-typed models (where we can assume that a non-optional input will never be None). However, to protect against incorrectly-typed models, we may end up having to add tedious
x is not Nonechecks before accessing x's fields/attributes. It may be useful to perhaps allow the user to flip the convention for a given rule (as to whether pattern-variables are allowed to be bound to None). Or, ensure a model is type-valid before applying optimizations. (Or, a quick hack might be to catch these exceptions in the rewrite-engine and skip that particular rewrite, which is probably a good idea anyway.)