[ty] Refactor relation.rs to store state on a struct rather than passing around 7 arguments every time we recurse#23837
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 85.29%. The percentage of expected errors that received a diagnostic held steady at 78.13%. The number of fully passing files held steady at 64/132. |
Memory usage reportSummary
Significant changesClick to expand detailed breakdownsphinx
flake8
trio
prefect
|
|
|
(I don't love the name |
|
I think this is an improvement. Really just naming thoughts:
Personally I like the idea of naming the struct just In some structural cases we descend into methods on other structs (like protocols or signatures), and it looks like currently we "unpack" the new struct into multiple arguments again when we do that. Seems like those methods could just take the decider/relation struct instead? Maybe better split into follow-up to keep the diff smaller, I guess. |
Yeah, as I said in the PR description:
which is partly why I went with the |
|
If we do the extended refactor that way (moving all the methods to the decider struct), another method name could be |
|
that works, I can make that change! |
dcreager
left a comment
There was a problem hiding this comment.
Ah, I see now that you only meant you're done introducing merge conflicts that are due to splitting up large files! 😂
j/k, I think this is a great refactoring
f336b5b to
317bcc0
Compare
21a7117 to
76306b2
Compare
598c479 to
c3732ad
Compare
|
OK... I think this is roughly where I would want it to be? |
c3732ad to
fcb3c07
Compare
19003c2 to
df76e32
Compare
charliermarsh
left a comment
There was a problem hiding this comment.
I read the summary closely which makes sense to me. I only skimmed the changes but assuming they match the summary, LGTM.
fe6244a to
8b7feb8
Compare
8b7feb8 to
6e0953f
Compare
…ssing around 7 arguments every time we recurse
6e0953f to
79bee72
Compare
* main: (94 commits) Fix shell injection via `shell=True` in subprocess calls (#23894) [ty] Refactor `relation.rs` to store state on a struct rather than passing around 7 arguments every time we recurse (#23837) Don't return code actions for non-Python documents (#23905) [ty] Make the default database truly statically infallible (#23929) [ty] Add `Download` button to ty playground which creates a zip export (#23478) [ty] Respect `kw_only` overwrites in dataclasses (#23930) [ty] Clarify in diagnostics that `from __future__ import annotations` only stringifies type annotations (#23928) [ty] Add a `Copy Markdown` button to playground (#23002) [ty] Fix folding range classification of lines starting with `#` (#23831) [ty] Fix folding ranges for notebooks (#23830) [ty] fix too-many-cycle panics when inferring literal type loop variables (#23875) Add `RegularCallableTypeOf` and `into_regular_callable` in `ty_extensions` (#23909) [ty] treat properties as full structural types (#23925) [ty] Avoid duplicated work during multi-inference (#23923) [ty]: make `possibly-missing-attribute` ignored by default [ty]: split out `possibly-missing-submodule` from `possibly-missing-attribute` Update astral-sh/setup-uv action to v7.5.0 (#23922) [ty] Show truthiness in ConstraintSet display and simplify falsy error message (#23913) Bump 0.15.6 (#23919) [ty] Narrow type context during collection literal inference (#23844) ...
Summary
What the title says. Currently all our recursive calls to
has_relation_to_impllook like this, which is very verbose, very repetitive, and not very readable:This PR moves the central recursive
matchinType::has_relation_to_implto a method on a new struct,TypeRelationCheckerstruct.TypeRelationCheckerstoresconstraints,inferable,relation,relation_visitoranddisjointness_visitoras fields, so these no longer need to be passed as arguments. Instead, the typical recursive call now looks like this:which is much more concise :-)
Instead of having many
::has_relation_to_implmethods scattered across the codebase on various types, we now instead have lots of methods onTypeRelationChecker(still scattered across various modules):FunctionType::has_relation_to_implbecomesTypeRelationChecker::check_function_pair;BoundMethodType::has_relation_to_implbecomesTypeRelationChecker::check_method_pair; etc.I've also done the same for
Type::is_disjoint_from_impl.Test Plan
Existing tests. This should be a pure refactor with no semantic changes.