Conversation
|
CodSpeed WallTime Performance ReportMerging #19099 will not alter performanceComparing Summary
|
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
I love the reduction in code complexity (especially relative to #19094!) but I do think this pretty clearly breaks the gradual guarantee for something like this?
from unresolvable_module import Foo, Bar
def f(x: list[Foo]):
y = cast(list[Bar], x)it seems just like somewhat random luck that we don't have anything in the mypy_primer corpus that triggers a false-positive redundant-cast diagnostic on that as a result of this change :-) I'd be curious to see what this idea looks like; it seems viable:
I think the better approach to this would be a strategy parameter to is_equivalent_to that could cause
Unknownto not be considered equivalent toUnknownorAny, rather than a separate recursive type walk.
I also don't think this gets us away in the long term from having to find a way to recursively walk types. For example, we've discussed for a while that a feature many people want is a way to know what percentage of types across a single run of ty are inferred as Any/Unknown. Mypy's version of this feature also tells users what percentage of types are "partially Any/Unknown"; it would be a shame not to be able to match that capability.
But on second thought, I don't think this approach works for protocols that have unknown fields? Equivalence for protocols just normalizes both the l.h.s. and the r.h.s., then compares for identity; this has the advantage that once recursive protocols are "solved" in ruff/crates/ty_python_semantic/src/types/instance.rs Lines 259 to 261 in fc43d3c But this means that these two protocols will normalize to the same type, because from unresolved_module import Foo
from typing import Any, Protocol
class Bar(Protocol):
x: Foo
class Baz(Protocol):
x: Any |
|
Ok, I'm convinced, both that we may have other needs for this in future, and that doing this |
2 participants
Summary
Remove the recursive type walk
any_over_typeand its sole current use in deciding whether to emit a redundant-cast diagnostic; instead, just skip the redundant-cast diagnostic if we are castingUnknownorTodoas a top-level type. This removes another recursive type walk (and its associated recursive-type complications).The ecosystem report suggests the number of false positives this currently adds is quite manageable, and the few that it does add can mostly be removed by targeting a few cases where we currently create
Todotypes. While suppressing false positives fromTodotypes is useful in the short term and worth doing when it's easy, I don't think it should drive significant architecture decisions or extra runtime work.If we do decide (even though it currently seems to have little impact on the ecosystem report) that it's important for gradual-guarantee reasons to do deep suppression of
redundant-castwhen nestedUnknowntypes are involved (I am not sure how strong the case for this is, since the use ofcastin the first place suggests a typed codebase), I think the better approach to this would be a strategy parameter tois_equivalent_tothat could causeUnknownto not be considered equivalent toUnknownorAny, rather than a separate recursive type walk.Test Plan
CI