core: introduce param_def API for ParametrizedAttribute declaration #3444
Conversation
…sc/type-var-constr
…c/type-var-constr
…-constraints/param_def
superlopuh
requested review from
compor,
AntonLydike,
alexarice,
math-fehr,
PapyChacal and
webmiche
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## sasha/misc/type-var-constr-merge #3444 +/- ##
=================================================================
Coverage 90.31% 90.31%
=================================================================
Files 464 464
Lines 58215 58254 +39
Branches 5555 5558 +3
=================================================================
+ Hits 52577 52613 +36
Misses 4210 4210
- Partials 1428 1431 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| default: None = None, | ||
| resolver: None = None, | ||
| init: Literal[False] = False, | ||
| ) -> Attribute: ... |
There was a problem hiding this comment.
This doesn't feel ideal? We effectively lose the python typing for the field which was kind of the whole point of having the typevar
There was a problem hiding this comment.
yep and it's currently breaking my code when I migrate xDSL to it. I'll probably change this soon.
| @@ -649,7 +650,7 @@ def return_target_analysis(module: builtin.ModuleOp): | |||
|
|
|||
|
|
|||
| class StencilTypeConversion(TypeConversionPattern): | |||
| @attr_type_rewrite_pattern | |||
| @attr_constr_rewrite_pattern(StencilTypeConstr) | |||
There was a problem hiding this comment.
Is this a different change?
There was a problem hiding this comment.
I guess it's more related to the previous change, let me revert this and see what happens
There was a problem hiding this comment.
Nope it's related to this change, since we use the attr_def instead of the type hints to construct the constraint, we can't do it on the ABC that is StencilTypeConstr since it's not really an attribute. We have to construct the constraint manually instead.
There was a problem hiding this comment.
Similarly in the change in riscv_snitch
…-constraints/param_def
This change introduces a new API, to bring attribute definition in line with operation definition. My proposal is to allow either API for the next release, in order not to break dependencies. I will open another PR on top of this one that will deprecate the old API, and migrate all of the dialects in xDSL to the new API. After we release a version with the deprecation warning, we can remove the old implementation entirely.
Note stacked PR