core: Make SSAValue generic #3987
Conversation
Adds a generic parameter to SSAValue that constrains the type associated with the SSAValue. This is useful whenever a function only accepts SSAValue of a given type. stack-info: PR: #3987, branch: math-fehr/stack/2
math-fehr
force-pushed
the
math-fehr/stack/1
branch
from
dedbbc4 to
85e62b9
Compare
math-fehr
force-pushed
the
math-fehr/stack/2
branch
from
43a5756 to
1c59e49
Compare
math-fehr
requested review from
superlopuh,
compor and
alexarice
and removed request for
superlopuh,
compor and
alexarice
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## math-fehr/stack/1 #3987 +/- ##
=====================================================
- Coverage 90.16% 89.99% -0.18%
=====================================================
Files 458 466 +8
Lines 58291 59260 +969
Branches 5691 5800 +109
=====================================================
+ Hits 52559 53330 +771
- Misses 4340 4500 +160
- Partials 1392 1430 +38 ☔ View full report in Codecov by Sentry. |
| @@ -119,6 +119,7 @@ def print(self, *argv: Any) -> None: | |||
| self.print_string(arg) | |||
| continue | |||
| if isinstance(arg, SSAValue): | |||
| arg = cast(SSAValue[Attribute], arg) | |||
There was a problem hiding this comment.
This is yucky but tbf this whole function is yucky
There was a problem hiding this comment.
Yeah, the problem is that isinstance(arg, SSAValue) returns an SSAValue[Unknown], and we cannot use isa in this file if I recall
There was a problem hiding this comment.
I'm not sure I fully grasped why pyright can't realise the type is SSAValue[Attribute] here using the lower bound on the type variable.
There was a problem hiding this comment.
It could work if the field is frozen, but would require too many changes in pyright AFAIU
| def foo(arg: SSAValue[IntegerType]): | ||
| pass | ||
|
|
||
| foo(value) |
There was a problem hiding this comment.
is there any way we can make a test that shows a type error? I guess not as it would have to rely on pyright
There was a problem hiding this comment.
Yeah I'm discussing it with Sasha, we are trying to find a solution
| @@ -478,13 +478,13 @@ def remove_use(self, use: Use): | |||
|
|
|||
|
|
|||
| @dataclass(eq=False) | |||
| class SSAValue(IRWithUses, ABC): | |||
| class SSAValue(Generic[AttributeCovT], IRWithUses, ABC): | |||
There was a problem hiding this comment.
My feeling is that this is unsound unless we make SSAValue immutable first, which blocks most of this PR stack.
There was a problem hiding this comment.
Are there places where we modify the SSAValue type in place?
There was a problem hiding this comment.
I am currently trying to see how easy it is to make this immutable, I'll probably add 1-2 commits in the stack
math-fehr
force-pushed
the
math-fehr/stack/2
branch
from
1c59e49 to
1f5d2be
Compare
Adds a generic parameter to SSAValue that constrains the type associated with the SSAValue. This is useful whenever a function only accepts SSAValue of a given type. stack-info: PR: #3987, branch: math-fehr/stack/2
Stacked PRs:
core: Make SSAValue generic
Adds a generic parameter to SSAValue that constrains the type associated
with the SSAValue.
This is useful whenever a function only accepts SSAValue of a given type.