Skip to content

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Aug 20, 2025

Summary

Part of: astral-sh/ty#868

This PR adds a heuristic to avoid argument type expansion if it's going to eventually lead to no matching overload.

This is done by checking whether the non-expandable argument types are assignable to the corresponding annotated parameter type. If one of them is not assignable to all of the remaining overloads, then argument type expansion isn't going to help.

Test Plan

Add mdtest that would otherwise take a long time because of the number of arguments that it would need to expand (30).

@dhruvmanila dhruvmanila added performance Potential performance improvement ty Multi-file analysis & type inference labels Aug 20, 2025
Copy link
Contributor

github-actions bot commented Aug 20, 2025

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

Copy link
Contributor

github-actions bot commented Aug 20, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

Comment on lines +665 to +669
reveal_type(
# error: [no-matching-overload]
# revealed: Unknown
f(
C(),
Copy link
Member Author

@dhruvmanila dhruvmanila Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the same example, but pass in A() as the first argument and use a=None instead of a=1 for the parameter above, this would still continue with the expansion. The reason being that A() matches parameter of at least one overload which means we cannot skip argument type expansion.

I'm trying to think through what kind of heuristic to have that would avoid the expansion in this case as well. This would still lead to no-matching-overload because even though Unknown is assignable to int, the spec says that all argument lists resulting from an expansion should evaluate successfully and there's no such expansion as expanding Unknown | None leads to argument lists where at least one would have None which isn't assignable to int.

Regardless, I think it'd be best to set a higher limit on the expansion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think there will be cases where we can't avoid actually trying all the expansions and see if any of them work -- best we can do there is optimize the expansions as best we can (e.g. iterator instead of eagerly materialized?) and then set a reasonable limit.

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Aug 20, 2025

EDIT: Nevermind, it is because ty doesn't support **kwargs and the new logic should avoid checking those arguments to skip argument type expansion for now.

rotki (https://github.com/rotki/rotki)
+ rotkehlchen/tests/db/test_db.py:377:13: error[no-matching-overload] No overload of bound method `set_dynamic_cache` matches arguments
+ rotkehlchen/tests/db/test_db.py:378:20: error[no-matching-overload] No overload of bound method `get_dynamic_cache` matches arguments
+ rotkehlchen/tests/db/test_db.py:418:20: error[no-matching-overload] No overload of bound method `get_dynamic_cache` matches arguments
- Found 1588 diagnostics
+ Found 1591 diagnostics

I wasn't expecting any mypy-primer diff but it seems that we have new diagnostics :)

I think this might be due to the fact that ty doesn't support **kwargs yet but I'm looking into it a bit closely. The main reason I think that is because when I remove the **kwargs in the call, the diagnostic disappear.

Screen.Recording.2025-08-20.at.15.46.35.mov

@dhruvmanila dhruvmanila marked this pull request as ready for review August 20, 2025 10:17
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

Comment on lines +665 to +669
reveal_type(
# error: [no-matching-overload]
# revealed: Unknown
f(
C(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think there will be cases where we can't avoid actually trying all the expansions and see if any of them work -- best we can do there is optimize the expansions as best we can (e.g. iterator instead of eagerly materialized?) and then set a reasonable limit.

// for evaluating the expanded argument lists.
snapshotter.restore(self, pre_evaluation_snapshot);

// At this point, there's at least one argument that can be expanded.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to first explicitly check if there are any expandable arguments (using the new method you added), then do this check, then actually expand the arguments? This way even if the heuristic applies, we've already generated the expansion, just haven't used the expansions yet.

But maybe generating the expansions is very cheap relative to actually trying the call with new argument types, so this doesn't matter?

And I guess we'd slow down the fast path slightly if we first check all arguments for expandability, then separately expand them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think this would really matter in practice mainly because there are only a few differences between expand_type and is_type_expandable which are (1) collecting the enum members from the metadata (both method generates the metadata) (2) performing multi-cartesian product of tuple elements and (3) allocation. I guess it wouldn't hurt to avoid allocating when it's easy to do so. Let me try it, I think I'll make this change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, so the main reason I did it at this location is so that the bindings state is the one before the type checking step, so that we only skip the overloads that have been filtered by the arity check.

So, we'd either have to pay the cost of either

  • Allocation for the first expansion, taking the bindings snapshot. We'd skip the snapshot if there are no expansion in this case.
  • Always take the binding snapshot, check whether expansion needs to happen using the logic in this PR, skip allocating for the first expansion if there's no need

I think going with the latter sounds reasonable i.e., instead of always allocating the first expansion, we'd always allocate the bindings snapshot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but then there's complication on restoring the snapshots at the correct places. I think I'll leave this as is for now.

@dhruvmanila dhruvmanila enabled auto-merge (squash) August 21, 2025 06:10
@dhruvmanila dhruvmanila merged commit d43a3d3 into main Aug 21, 2025
36 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/avoid-argument-type-expansion branch August 21, 2025 06:13
dcreager added a commit that referenced this pull request Aug 21, 2025
* main:
  [ty] Use `dedent` in cursor tests (#20019)
  Fix rust feature activation (#20012)
  [ty] Avoid unnecessary argument type expansion (#19999)
  [ty] Add link for namespaces being partial (#20015)
second-ed pushed a commit to second-ed/ruff that referenced this pull request Sep 9, 2025
## Summary

Part of: astral-sh/ty#868

This PR adds a heuristic to avoid argument type expansion if it's going
to eventually lead to no matching overload.

This is done by checking whether the non-expandable argument types are
assignable to the corresponding annotated parameter type. If one of them
is not assignable to all of the remaining overloads, then argument type
expansion isn't going to help.

## Test Plan

Add mdtest that would otherwise take a long time because of the number
of arguments that it would need to expand (30).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Potential performance improvement ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants