Python: Add type-tracking flow for class (instance) attributes - #16670
Draft
RasmusWL wants to merge 10 commits into
Draft
Python: Add type-tracking flow for class (instance) attributes#16670RasmusWL wants to merge 10 commits into
RasmusWL wants to merge 10 commits into
Conversation
RasmusWL
marked this pull request as ready for review
June 10, 2024 08:31
Member
Author
|
haven't written a change-note yet, I'll do that later 👍 |
Contributor
|
yoff
approved these changes
Dec 10, 2024
yoff
left a comment
Contributor
There was a problem hiding this comment.
This looks good to me; I think the performance concerns are not so severe, as the reads and writes all target the class expression as an intermediate node? We should have a DCA run for sure, but it feels like it should be OK. Regarding your other questions:
- This sounds like a problem given Tom's comment, but I got the impression that you had an off-line conversation sorting this out?
- [skipped by initial comment]
- I agree that this is likely fine in practice and that we should see it being a problem before complicating the code. (If we had an easy "get the last locally assigned value" we could have used that.)
- I wonder if subclass flow will become important; I agree it can wait.
- I think it is cool to have instance flow. I notice that our tracking of instances is only local, but I think a proper implementation will require the full type-tracking-recursive-with-call-graph-construction-machinery that we are not using yet (and which comes with its own performance concerns).
RasmusWL
marked this pull request as draft
March 25, 2025 12:04
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
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.
This PR adds flow from
<value>to the use inprintfor type-tracking. It also handles instances, and class-level attributes. (reviewing commit-by-commit is recommended)Implementation questions
The implementation so far uses
loadStoreStepto add flow to the self/cls parameters of normal/classmethods on a class. As highlighted in the comment in the code, compared with a "simple" jump-step or levelStepNoCall, this allows any potential flow-summaries to still work.(note: Ruby currently uses levelStepNoCall, which is where the inspiration for doing so came from).
However, there's a few things I'm not 100% sure of, so let me call those out:
whether there's an implicit assumption that
loadStoreStepis not allowed to cross function borders (if so, we should add that as a consistency check). If that's the case, we can implement this withjumpStep, but will sacrifice some functionality since we will need to target attribute-reads directly.performance! Since this adds a step from all
nwrites to everymreads of an attribute, worst case is O(n * m) = O(n²) 😞 We should be able to overcome this by adding an intermediary node that represents class-instance thatself.foo = <value>can target, resulting in O(n + m) steps 👍 I haven't done anything about this yet, since I wanted to get the basic functionality in first.This PR does simple assumption that any
self.foo = <value>will end up being available after the function has finished... but we could see code likeself.foo = <value1>; self.foo = <value2>, where only<value2>would be available afterwards. I don't expect this will become a huge issue, so I propose we wait until we see this being a problem in real code.Subclass flow is currently not handled. I think we need proper performance solved first, and also wanted to ensure we could get agreement on basic functionality become making implementation more complex.
Instance flow. This felt like the right thing to do, both so we would be able to handle examples like the one below, but also so instance and
selfreference within a method would behave in the same way.