[WIP] core: Add a Not Constraint#5167
Conversation
wip to discuss the addition of a Not Constraint, discussion started in xdslproject#5098
Jimmy2027
changed the title
Add a constraint that constrains an integer to not be equal to a given value. Might be generalized to a general "Not" constraint, see xdslproject#5167
…ains an integer to not be equal to a given value. Might be generalized to a general "Not" constraint, see xdslproject#5167
… constrains an integer to not be equal to a given value. Might be generalized to a general "Not" constraint, see xdslproject#5167
|
Can you please make the CI pass? Then would be nice to chat on the Zulip about this to have a bit more of a high-bandwidth conversation. It seems like the general approach should work but would be good to have @math-fehr and @alexarice's takes. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5167 +/- ##
==========================================
- Coverage 86.12% 85.66% -0.47%
==========================================
Files 367 367
Lines 51804 52217 +413
Branches 5900 5925 +25
==========================================
+ Hits 44617 44730 +113
- Misses 5763 6061 +298
- Partials 1424 1426 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
superlopuh
changed the title
math-fehr
left a comment
math-fehr
left a comment
There was a problem hiding this comment.
That looks good to me! I think we need that constraint, even though that can easily become a footgun if we are not careful enough
|
Would be good to get @alexarice's tick before merging. It would be nice to have a way to avoid the backtracking. This seems related to the anyof PR, a bit of functionality we're missing is a check for whether running the constraint will modify the context. |
alexarice
left a comment
alexarice
left a comment
There was a problem hiding this comment.
I'm generally hesitant about checks on constraints that ensure there are no variables, as I feel at least part of the motivation of this system was to eventually be fully compatible/upstream to irdl, where every constraint is a variable. I guess in irdl this could be replaced with some sort of check that the constraint only has a single use, but I still thought I'd voice my unease at this.
|
|
||
| def __post_init__(self) -> None: | ||
| """Validate that the inner constraint doesn't contain variables.""" | ||
| if self.constraint.variables(): |
There was a problem hiding this comment.
As mentioned in the xdsl meeting. I don't believe this ensures that the constraint doesn't mention any variables, just that it doesn't guarantee solving variables.
There was a problem hiding this comment.
As an example Not(Not(VarConstraint("x", AnyAttr())) is allowed by this atm
There was a problem hiding this comment.
the inner constraint Not(VarConstraint("x", AnyAttr()) would throw an error no?
I'm getting this:
xdsl.utils.exceptions.PyRDLError: Not constraint cannot contain variables, but inner constraint VarConstraint(name='x', constraint=AnyAttr()) has variables: {'x'}.
There was a problem hiding this comment.
Sorry, it was a bad example: I think Not(AnyOf(VarConstraint("x", base(IntegerAttr)), VarConstraint("y", base(FloatAttr)))) is a concrete example.
|
I still think we might want to merge this at some point, but let's close it for now, until we have a better conviction of when this is safe to use. |
4 participants
wip to discuss the addition of a Not Constraint,
discussion started in #5098