[ty] Create Definition for all place expressions#18890
Closed
dhruvmanila wants to merge 1 commit intomainfrom
Closed
[ty] Create Definition for all place expressions#18890dhruvmanila wants to merge 1 commit intomainfrom
Definition for all place expressions#18890dhruvmanila wants to merge 1 commit intomainfrom
Conversation
CodSpeed Instrumentation Performance ReportMerging #18890 will degrade performances by 7.8%Comparing Summary
Benchmarks breakdown
|
CodSpeed WallTime Performance ReportMerging #18890 will improve performances by 5.69%Comparing Summary
Benchmarks breakdown
|
dhruvmanila
added a commit
that referenced
this pull request
Jun 24, 2025
## Summary This PR fixes astral-sh/ty#185 by avoiding to infer the value expression for an unpacking. This is done simply by only inferring the value expression in a non-unpacking branch for assignment statement, for statement, with statement and comprehensions. This is a simpler alternative to #18890 which I only realized in hindsight! Ideally, the solution would to consider the "unpack" as it's own region and do all of the inference of every expressions involved in an unpacking inside the unpack query and then merge the results in the outer query. This would require access to the `Unpack` ingredient which is stored on the `Definition`. And, this would require create the said `Definition`s for all attributes and subscript expressions. It does simplify the target inference logic by streamlining it into a single `infer_target` method instead of the `infer_target`/`infer_target_impl` split. Additionally, #18890 also solves a couple of TODOs around raising errors around attribute / subscript assignment. ## Test Plan Update the existing test, go through a couple of ecosystem diagnostic.
dhruvmanila
force-pushed
the
dhruv/unpack-diagnostics-bug
branch
2 times, most recently
from
c14bf1e to
6e5113a
Compare
dhruvmanila
force-pushed
the
dhruv/unpack-diagnostics-bug
branch
from
6e5113a to
ae4f960
Compare
dhruvmanila
changed the title
Definition for all place expressions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR was the first iteration on solving astral-sh/ty#185 but later realized there's a simpler solution instead.
Regardless, this PR does seem useful but it's not a priority so I'm just noting down the things that I've changed and learned:
PlaceExprso that it is constructed for every attribute / subscript expression and not only a subset of it. This allows us to createDefinitionfor every attribute / subscript expression that's involved in an assignment.infer_targetandinfer_target_impland remove the complexity of the closure parameter and streamline the logic so that it all boils down to callinginfer_definitionfor theDefinitionthat belongs to name / attribute / subscript expression.add_bindingsince that's where the assignability check for name expressions are.This also simplifies handling the target inference so that it's only a single
infer_targetcall although it does has it's own complexity specifically around the fact that the value expression of the first comprehension needs to be inferred from the outer scope and not the comprehension scope.Test Plan