-
Notifications
You must be signed in to change notification settings - Fork 79
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
core: Let inferrable constraints solve variables #3698
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3698 +/- ##
==========================================
- Coverage 91.31% 91.30% -0.01%
==========================================
Files 467 467
Lines 58487 58527 +40
Branches 5640 5645 +5
==========================================
+ Hits 53406 53441 +35
- Misses 3632 3634 +2
- Partials 1449 1452 +3 ☔ View full report in Codecov by Sentry. |
class _OperandResultInferenceExtractor(VarExtractor[ParsingState]): | ||
idx: int | ||
is_operand: bool | ||
inner: VarExtractor[Sequence[Attribute]] | ||
constr: RangeConstraint |
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.
class _OperandResultInferenceExtractor(VarExtractor[ParsingState]): | |
idx: int | |
is_operand: bool | |
inner: VarExtractor[Sequence[Attribute]] | |
constr: RangeConstraint | |
class _OperandResultInferenceExtractor(VarExtractor[ParsingState]): | |
idx: int | |
KW_ONLY | |
is_operand: bool | |
inner: VarExtractor[Sequence[Attribute]] | |
constr: RangeConstraint |
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 help read the code below
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.
I've never seen this before
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.
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.
I misremembered the API
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.
I truly hate that API but agree it would make the code better
Overall I think it looks good, and it feels like it would be worth having the support. It feels like it would be worth adding some unit tests with explicit NotImplemented catches with corresponding issues to highlight what's missing. |
I'm going to get the length constraint version into shape and can compare to that. |
Allows an inferrable type to solve other variables, and further allows range variable constraints to be solved if the underlying constraint is inferrable. Includes a test with the motivating example.
Currently this is restricted to when the underlying constraint can be inferred using no variables, and so is likely only useful for the scenario in the test. Anything more involved would be a fair bit more complex.
I also have a branch somewhere with a draft implementation of "length variables", which would probably achieve a similar thing to this PR (and could potentially be extended to be more powerful). Would be good to hear people's thoughts on whether this would be preferable instead of this (slightly hacky) solution.