-
Notifications
You must be signed in to change notification settings - Fork 77
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: (irdl) ParamAttrConstraint can infer recursively #3477
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3477 +/- ##
==========================================
+ Coverage 90.26% 90.29% +0.02%
==========================================
Files 461 462 +1
Lines 57807 57881 +74
Branches 5565 5564 -1
==========================================
+ Hits 52180 52261 +81
+ Misses 4190 4187 -3
+ Partials 1437 1433 -4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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 don't believe this is sound, ParamAttrConstraint
only specifies the base class of the attribute and so could theoretically this could infer the wrong subclass?
Note that I'm not sure if being able to capture subclasses is part of the intended semantics of |
Ah yes, although I'm still not sure that we should officially support the whole class hierarchy thing. The error scenario here is that the user specifies a superclass in the constraint and there's a crash at runtime if the superclass is abstract? To avoid this, we could probably check that the class has a |
Could do an |
A similar question to this is whether a |
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 think I either forgot to do this, or at the time is_runtime_final
didn't exist yet!
So yeah that looks good to me!
Yes it should! What we could do is write an optimization to "convert" a |
As discussed in xdslproject#3477 Fixes xdslproject#3483
This seems to work, is there any reason not to do this?
Note: conflicts with #3456