misc: use experimental TypeForm feature of Pyright #3450
Conversation
superlopuh
requested review from
AntonLydike,
alexarice,
math-fehr,
PapyChacal,
n-io and
webmiche
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3450 +/- ##
==========================================
- Coverage 90.40% 90.39% -0.01%
==========================================
Files 467 467
Lines 58840 58841 +1
Branches 5603 5605 +2
==========================================
- Hits 53195 53191 -4
- Misses 4204 4206 +2
- Partials 1441 1444 +3 ☔ View full report in Codecov by Sentry. |
|
@superlopuh can you please link the incoming PRs when they land? |
|
I hope so. I'll wait for @math-fehr's review to merge. The main risk with this would be to rely on an experimental feature, so we might get stuck again if they remove support. OTOH, we pin our version of Pyright, so it's not like everything will break, and I haven't seen any negative feedback on the TypeForm PEP so I have a feeling it'll land eventually. |
|
Even if Pyright is pinned, we don't want to get stuck on an old version of pyright again |
|
I'll keep exploring other options but I have a feeling that it might be the best one to have a clean interface for |
| _T = TypeVar("_T") | ||
|
|
||
|
|
||
| def isa(arg: Any, hint: type[_T]) -> TypeGuard[_T]: | ||
| def isa(arg: Any, hint: "TypeForm[_T]") -> TypeGuard[_T]: |
There was a problem hiding this comment.
What error do you get here?
There was a problem hiding this comment.
TypeForm is defined in Pyright, not typing_extensions, so the error is:
E NameError: name 'TypeForm' is not defined
| if not isa(op_val := op.value, IntegerAttr): | ||
| if not isinstance(op_val := op.value, IntegerAttr): |
There was a problem hiding this comment.
What happens in those cases? isa is returning an error, or is it just that isinstance can do it so we should follow that?
There was a problem hiding this comment.
Argument type is partially unknown
Argument corresponds to parameter "hint" in function "isa"
Argument type is "TypeForm[IntegerAttr[Unknown]]"
There was a problem hiding this comment.
IntegerAttr is a valid reference to the object that refers to the class, whereas IntegerAttr[IntegerType | IndexType] is the valid type annotation, it's a place where type[_T] and TypeForm[_T] don't overlap.
There was a problem hiding this comment.
I assume that this isn't the case for newer versions of Python, where generic arguments can have default values, where IntegerAttr would be shorthand for IntegerAttr[IntegerType | IndexType]
|
Actually I'm now realizing that this will mean that any downstream users also need to enable experimental features locally for their xDSL code to type check... I'm not sure that I want to force that on people. Let's see if we find another solution for the current goal of migrating to |
|
I had an offline discussion with @math-fehr , and he convinced me that it would be fine. In the end, type-checking is an optional feature, and is not required for the framework to run correctly for the user, and it's only a flag away. |
|
Will we likely need to turn on some experimental feature flag downstream? |
|
Yep, same one as in this PR. I'll open another PR for the README change |
Adds missing documentation to #3450
This lets us use better typing when dealing with TypeVars, PRs leveraging this incoming.