Skip to content

[red-knot] Use iterative approach to collect overloads#17607

Merged
dhruvmanila merged 2 commits intomainfrom
dhruv/overload-iterative
Apr 24, 2025
Merged

[red-knot] Use iterative approach to collect overloads#17607
dhruvmanila merged 2 commits intomainfrom
dhruv/overload-iterative

Conversation

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Apr 24, 2025 •
edited
Loading

Summary

This PR updates the to_overloaded method to use an iterative approach instead of a recursive one.

Refer to #17585 (comment) for context.

The main benefit here is that it avoids calling the to_overloaded function in a recursive manner which is a salsa query. So, this is a bit hand wavy but we should also see less memory used because the cache will only contain a single entry which should be the entire overload chain. Previously, the recursive approach would mean that each of the function involved in an overload chain would have a cache entry. This reduce in memory shouldn't be too much and I haven't looked at the actual data for it.

Test Plan

Existing test cases should pass.

@dhruvmanila dhruvmanila added internal An internal refactor or improvement ty Multi-file analysis & type inference labels Apr 24, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2025 •
edited
Loading

mypy_primer results

No ecosystem changes detected ✅

@dhruvmanila dhruvmanila marked this pull request as ready for review April 24, 2025 15:11
@AlexWaygood AlexWaygood removed their request for review April 24, 2025 15:12
@dcreager
Copy link
Member

dcreager commented Apr 24, 2025

As an aside, can/should FunctionSignature use CallableSignature for its list of overloads, instead of Vec<Signature>?

@dcreager
Copy link
Member

dcreager commented Apr 24, 2025

And if we add CallableSignature::is_overloaded, you could use CallableSignature for the non-overloaded case, too, and then collapse the two variants of FunctionSignature down into a struct.

MichaReiser
MichaReiser reviewed Apr 24, 2025
View reviewed changes
@carljm
Copy link
Contributor

carljm commented Apr 24, 2025

can/should FunctionSignature use CallableSignature for its list of overloads

I thought about this when reviewing a previous PR that introduced FunctionSignature, but decided not to comment on it at the time because CallableSignature has a number of extra fields that are not useful to FunctionSignature, and there didn't seem to be a strong downside to having FunctionSignature not use CallableSignature. I am interested in @dhruvmanila 's thoughts here, though.

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Apr 24, 2025

As an aside, can/should FunctionSignature use CallableSignature for its list of overloads, instead of Vec<Signature>?

Yeah, I think that could be used. Initially, I avoided that because of the additional fields that it had. There are couple of methods, specifically the into_callable_type on FunctionType and BoundMethodType which just requires the Signatures to convert the function / method into a callable type. So, the intermediary CallableSignature is not useful in that case.

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Apr 24, 2025

And if we add CallableSignature::is_overloaded, you could use CallableSignature for the non-overloaded case, too, and then collapse the two variants of FunctionSignature down into a struct.

Yeah, that's a good idea. I can try in a follow-up later to remove the FunctionSignature if possible.

@dhruvmanila dhruvmanila reopened this Apr 24, 2025
@dhruvmanila dhruvmanila merged commit 9937064 into main Apr 24, 2025
34 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/overload-iterative branch April 24, 2025 16:53
dcreager added a commit that referenced this pull request Apr 25, 2025
@dcreager
Merge branch 'main' into dcreager/lazy-generic-class
67e5888 
* main: (24 commits)
  [`airflow`] Extend `AIR301` rule (#17598)
  [`airflow`] update existing `AIR302` rules with better suggestions (#17542)
  red_knot_project: sort diagnostics from checking files
  [red-knot] fix detecting a metaclass on a not-explicitly-specialized generic base (#17621)
  [red-knot] fix inheritance-cycle detection for generic classes (#17620)
  [`pylint`] Detect `global` declarations in module scope (`PLE0118`) (#17411)
  Add Semantic Error Test for LateFutureImport (#17612)
  [red-knot] change TypeVarInstance to be interned, not tracked (#17616)
  [red-knot] Special case `@final`, `@override` (#17608)
  [red-knot] add TODO comment in specialization code (#17615)
  [`semantic-syntax-errors`] test for `LoadBeforeGlobalDeclaration` - ruff linter (#17592)
  [syntax-errors] `nonlocal` declaration at module level (#17559)
  [`airflow`] Apply auto fix to cases where name has been changed in Airflow 3 (`AIR311`) (#17571)
  [syntax-errors] Make `async-comprehension-in-sync-comprehension` more specific (#17460)
  Bump 0.11.7 (#17613)
  [red-knot] Use iterative approach to collect overloads (#17607)
  red_knot_python_semantic: avoid Rust's screaming snake case convention in mdtest
  red_knot_python_semantic: improve diagnostics for unsupported boolean conversions
  red_knot_python_semantic: add "return type span" helper method
  red_knot_python_semantic: move parameter span helper method
  ...
dcreager added a commit that referenced this pull request Apr 26, 2025
@dcreager
Merge branch 'main' into dcreager/legacy-typevar-instance
fb51439 
* main: (27 commits)
  [red-knot] Add new property tests for subtyping with "bottom" callable (#17635)
  [red-knot] Create generic context for generic classes lazily (#17617)
  ruff_db: add tests for annotations with no ranges
  [`airflow`] Extend `AIR301` rule (#17598)
  [`airflow`] update existing `AIR302` rules with better suggestions (#17542)
  red_knot_project: sort diagnostics from checking files
  [red-knot] fix detecting a metaclass on a not-explicitly-specialized generic base (#17621)
  [red-knot] fix inheritance-cycle detection for generic classes (#17620)
  [`pylint`] Detect `global` declarations in module scope (`PLE0118`) (#17411)
  Add Semantic Error Test for LateFutureImport (#17612)
  [red-knot] change TypeVarInstance to be interned, not tracked (#17616)
  [red-knot] Special case `@final`, `@override` (#17608)
  [red-knot] add TODO comment in specialization code (#17615)
  [`semantic-syntax-errors`] test for `LoadBeforeGlobalDeclaration` - ruff linter (#17592)
  [syntax-errors] `nonlocal` declaration at module level (#17559)
  [`airflow`] Apply auto fix to cases where name has been changed in Airflow 3 (`AIR311`) (#17571)
  [syntax-errors] Make `async-comprehension-in-sync-comprehension` more specific (#17460)
  Bump 0.11.7 (#17613)
  [red-knot] Use iterative approach to collect overloads (#17607)
  red_knot_python_semantic: avoid Rust's screaming snake case convention in mdtest
  ...
@dhruvmanila
Copy link
Member Author

dhruvmanila commented Apr 28, 2025

Yeah, that's a good idea. I can try in a follow-up later to remove the FunctionSignature if possible.

Actually, I realized today why I didn't opt for that - it's because CallableSignature requires the signature_type as an input when creating the struct and in FunctionType::signature that would always be FunctionType which isn't correct. For a bound method it should be Type::BoundMethod, for a callable type it should be Type::Callable, and so on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

@MichaReiser MichaReiser MichaReiser approved these changes

@sharkdp sharkdp Awaiting requested review from sharkdp

@dcreager dcreager Awaiting requested review from dcreager

Assignees

No one assigned

Labels

internal An internal refactor or improvement ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dhruvmanila @dcreager @carljm @MichaReiser