-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Struct redefinition caching, to support "undo" workflow #59127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The concept seems sound to me. However, there is a bit of a semantic complication with respect to the type parameters. The reason that lowering does this whole complicated thing in the first place is that it will try use the old type parameters to facilitate the equivalence query. When I added the partitioning, I added some code to re-substitute the parameters from the old to the new. I think in order to make this work, you'll want to essentially do the opposite of that - scan all the existing partitions, check equiv_typedef, if it matches, substitute in the variables from the old dt and then check field equivalence. I think that should work. Also note that there may be more than one past type that passes equiv_types, so you do need to check all of them. |
Not required, since it's only used in one place. In fact, you'll be able to delete the |
We may need a mechanism to signal this outcome to Revise. The code that checks equivalence is currently in https://github.com/timholy/Revise.jl/blob/437e2d1ce846d6330be731cdc190e39818f1eeb7/src/lowered.jl#L452-L470 (not yet merged, part of timholy/Revise.jl#894). Would an alternative option be to have Revise do this analysis itself? There might be one more complication: suppose the user performs the following sequence:
So now |
Why does Revise care? If the type is not the same as the immediate prior one, it still needs to redefine all the methods. |
Yeah, I agree with Keno - I don't think Revise needs to care. From Revise's perspective, we have still redefined the struct. We've just revised it "back" to one that was defined in the past. But Revise still needs to do exactly the same things it would without this PR: delete all the invalidated methods, consts and structs, and re-evaluate them according to the new type. (It's just that that "new" type happens to have been one that julia has seen before, at some point in the past.) |
To be clearer, imagine the following baseline situation:
Now revise the definition of Then in one go (without issuing a REPL command in between):
Revise does not have a clear definition of the order in which changes should be made. Suppose it initially revises However, it's not 100% clear that the undo functionality makes this more complicated to handle. Revise probably could handle it with a dependency tree that ignores world-age. But circular definitions (which we allow) could make this a little harder. |
Interesting. I'm still not sure that I see the issue. If you revise Conversely, if you revise Indeed, i think that nothing about that process changes as a result of this PR. Does that sound right to you? |
Agreed, except for the possibility of cycles. However, I think these have to be internal to a single type. Example: #42297 |
JuliaCon 2025 Hackathon Project!
This is to help improve the user experience of struct definition with Revise.
The focus of this change is to improve the following scenario, which could happen today on master, after Revise supports invalidation of methods and structs from struct changes:
After this change, step 6 would succeed, since we will find the existing definition of the struct
typeof(X)
, and use that cached definition when evaluating the restored definition for X.TODO:
typebody!
, removing the current "hack" to find the binding from the module by name.julia-internal.h
, with the normal naming conventions.Co-Authored-By: @topolarity