[ty] Use ParamSpec without the attr for inferable check#21934
[ty] Use ParamSpec without the attr for inferable check#21934dhruvmanila merged 3 commits intomainfrom
ParamSpec without the attr for inferable check#21934Conversation
dhruvmanila
added
bug
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
dhruvmanila
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
There was a problem hiding this comment.
I find it a bit suspicious that this rule is so much simpler than our existing rules for other typevars, which require six or seven different match clauses to implement. It seems like in principle the rules should be the same, the only difference being that we need to account for stripping the attributes in this case.
Is it possible that this could be instead implemented by just checking the attribute is the same, stripping the attribute, and then delegating to a recursive call to has_relation_to_impl with the base typevars instead of the attributes?
Alternatively, given the restrictions on where ParamSpec attributes can legally appear (only in a callable signature, and only together, applied to *args and **kwargs), I wonder if this wouldn't be easier/simpler if we handled it directly in callable-type has_relation_to_impl?
If none of that seems to work easily, I think it'd be OK to land this version (ecosystem impact looks good as far as it goes!), but maybe with an added TODO that this probably isn't fully correct yet?
There was a problem hiding this comment.
Is it possible that this could be instead implemented by just checking the attribute is the same, stripping the attribute, and then delegating to a recursive call to
has_relation_to_implwith the base typevars instead of the attributes?
Yeah, I tried this but it fails in this test case that I added in this PR:
from typing import Callable
class Container[**P]:
def method(self, f: Callable[P, None]) -> Callable[P, None]:
return f
def try_assign[**Q](self, f: Callable[Q, None]) -> Callable[Q, None]:
# error: [invalid-return-type] "Return type does not match returned value: expected `(**Q@try_assign) -> None`, found `(**P@Container) -> None`"
# error: [invalid-argument-type] "Argument to bound method `method` is incorrect: Expected `(**P@Container) -> None`, found `(**Q@try_assign) -> None`"
return self.method(f)where the invalid-argument-type diagnostic is not emitted.
Alternatively, given the restrictions on where ParamSpec attributes can legally appear (only in a callable signature, and only together, applied to
*argsand**kwargs), I wonder if this wouldn't be easier/simpler if we handled it directly in callable-typehas_relation_to_impl?
Yeah, I think that makes sense. I'll move this check to the Signature::has_relation_to_inner
Some additional context here: https://discord.com/channels/1039017663004942429/1449080092515893316/1449975836055703673
There was a problem hiding this comment.
Interesting. I think perhaps this reveals a bug in our handling of non-ParamSpec typevars?
class Container[T]:
def method(self, x: T) -> T:
return x
def try_assign[U](self, x: U) -> U:
return self.method(x)https://play.ty.dev/e477cd72-491d-4953-b77a-97ad0d47fd1f
It seems like there should be two diagnostics on the last line there as well (I wouldn't expect U to be assignable to T in the call to self.method), but we only get one (invalid return type).
dhruvmanila
deleted the
dhruv/inferrable-paramspec-check-without-attr
branch
* origin/main: Update MSRV to 1.90 (#21987) [ty] Improve check enforcing that an overloaded function must have an implementation (#21978) Update actions/checkout digest to 8e8c483 (#21982) [ty] Use `ParamSpec` without the attr for inferable check (#21934) [ty] Emit diagnostic when a type variable with a default is followed by one without a default (#21787) [ty] Fix callout syntax in configuration mkdocs (#1875) (#21961) Update debug_assert which pointed at missing method (#21969) [ty] Add support for `__qualname__` and other implicit class attributes (#21966)
* origin/main: Fluent formatting of method chains (#21369) [ty] Avoid stack overflow when calculating inferable typevars (#21971) [ty] Add "qualify ..." code fix for undefined references (#21968) [ty] Use jemalloc on linux (#21975) Update MSRV to 1.90 (#21987) [ty] Improve check enforcing that an overloaded function must have an implementation (#21978) Update actions/checkout digest to 8e8c483 (#21982) [ty] Use `ParamSpec` without the attr for inferable check (#21934) [ty] Emit diagnostic when a type variable with a default is followed by one without a default (#21787)
2 participants
Summary
fixes: astral-sh/ty#1820
Test Plan
Add new mdtests.
Ecosystem changes removes all false positives.