[Tracking branch] core: (Constraints) introduce constraint simplification#6034
[Tracking branch] core: (Constraints) introduce constraint simplification#6034alexarice wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6034 +/- ##
=======================================
Coverage 86.78% 86.78%
=======================================
Files 423 423
Lines 62638 62653 +15
Branches 7185 7186 +1
=======================================
+ Hits 54361 54375 +14
- Misses 6703 6705 +2
+ Partials 1574 1573 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
superlopuh
left a comment
There was a problem hiding this comment.
It's interesting that you went with that, I was actually thinking of doing the opposite... In operations we have inits be the nice sugar that does magic, and create basically just populate the fields. In Python it's quite easy to have init return a different type to the one being inited. It's a little dirty but it kind of also makes sense if you want the optimisation to kick in automatically, and to have clients explicitly opt out of the magic if they want to. How do you feel about that?
|
Ah, I didn't realise that an init could return a different type. I'm not sure how I feel about that |
|
If not init, I would expect |
|
I've thought about it a little and having an |
|
Another option is an explicit |
|
Although I would prefer to have a normal form that is automatically simplified, instead of having to re-simplify already simplified subtrees. |
|
The issue with "resimplifying" is I can imagine cases where the "non-normal" form is invalid but the simplified version is |
|
Would you be ok with renaming to |
|
My gut tells me that the hacky init is the best way forward. It's really hard to enforce that things are normalised otherwise, it's quite easy for people implementing |
|
We are in Python after all, and if I understand correctly if the new method is typed Pyright actually understands it. from typing import reveal_type
class A:
def __init__(self, param: str):
self.param = param
def __new__(cls, param: str) -> "A|B":
if param == "b":
return B(param)
else:
return cls(param)
class B(A): ...
a = A("a")
reveal_type(a)
b = A("b")
reveal_type(b) |
|
Cool, I'll try to make it a |
|
I've had a little play around and am now more convinced that this is cursed. All the comments on the stackexchange post you linked say not to do this, and I can't seem to avoid calling |
|
OK how about this: we change the constraints to not do any smart stuff in the init (like no constraint coersion or whatever) and add |
|
I like that |
Going to try the @superlopuh method of make a big PR and slowly split things off
Proposes to implement basic constraint simplification by adding
createmethods to various constraints that are drop ins for their__init__methods, but possibly perform some simplification. These can't just be the__init__methods as the types don't line up (e.g. an AnyOf.create of one constraint doesn't return an AnyOf). Further uses thesecreatemethods inmapping_type_varsand inirdl_to_attr_constraintto force simplification when converting python types.Simplifications added in this PR:
AnyOf1 constraint returns just that constraintAnyOfofAnyOfis flattened to a singleAnyOf(just does one "layer" of flattening at the moment which I think should be sufficient)ParamAttrConstraintofEqAttrConstraintsbecomes anEqAttrConstraintIntAttrConstraintof aEqIntConstraint(X)becomes aEqAttrConstraint(IntAttr(X))The last two suffice to make
irdl_to_attr_constraint(I32) == EqAttrConstraint(i32)I have checked that this PR allows @hhkit 's type:
I didn't implement @superlopuh 's suggested
AnyOfofParamAttrConstraintgoes toParamAttrConstaintofAnyOfas it didn't seem immediately necessary but it could be added.