-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Export invalidation fails to happen properly (binding partitions) #57377
Labels
regression
Regression in behavior compared to a previous version
Milestone
Comments
Keno
added a commit
that referenced
this issue
Feb 13, 2025
The original design for the BindingPartition datastructure had ->restriction and ->kind as separate non-atomic fields. However, to support the old semantics, we created an intermediate state where both the restriciton and the kind were placed into ->restriction as a pointer-int-union (i.e. using the low three bits of the pointer to store an int). In #57341, I removed that last semantic place that needed to update these both atomically. This PR removes all the remaining non-semantic places and changes the datastructure back to its indended design. This is a necessary prerequisitve to be able to use more than three ->kind bits, which will be required for export invalidation (#57377), as well as some nicer error messages in failure cases.
vtjnash
pushed a commit
that referenced
this issue
Feb 13, 2025
The original design for the BindingPartition datastructure had ->restriction and ->kind as separate non-atomic fields. However, to support the old semantics, we created an intermediate state where both the restriciton and the kind were placed into ->restriction as a pointer-int-union (i.e. using the low three bits of the pointer to store an int). In #57341, I removed that last semantic place that needed to update these both atomically. This PR removes all the remaining non-semantic places and changes the datastructure back to its indended design. This is a necessary prerequisitve to be able to use more than three ->kind bits, which will be required for export invalidation (#57377), as well as some nicer error messages in failure cases.
Keno
added a commit
that referenced
this issue
Feb 14, 2025
Whether or not a binding is exported affects the binding resolution of any downstream modules that `using` the module that defines the binding. As such, it needs to fully participate in the invalidation mechanism, so that code which references bindings whose resolution may have changed get properly invalidated. To do this, move the `exportp` flag from Binding into a separate bitflag set that gets or'd into the BindingPartition `->kind` field. Note that we do not move `publicp` in the same way since it does not affect binding resolution. There is currently no mechanism to un-export a binding, although the system is set up to support this in the future (and Revise may want it). That said, at such a time, we may need to revisit the above decision on `publicp`. Lastly, I will note that this adds a fair number of additional invalidations. Most of these are unnecessary, as changing an export only affects the *downstream* users not the binding itself. I am planning to tackle this as part of a larger future PR that avoids invalidation when this is not required. Fixes #57377
KristofferC
pushed a commit
that referenced
this issue
Feb 14, 2025
The original design for the BindingPartition datastructure had ->restriction and ->kind as separate non-atomic fields. However, to support the old semantics, we created an intermediate state where both the restriciton and the kind were placed into ->restriction as a pointer-int-union (i.e. using the low three bits of the pointer to store an int). In #57341, I removed that last semantic place that needed to update these both atomically. This PR removes all the remaining non-semantic places and changes the datastructure back to its indended design. This is a necessary prerequisitve to be able to use more than three ->kind bits, which will be required for export invalidation (#57377), as well as some nicer error messages in failure cases. (cherry picked from commit 40fbc88)
Keno
added a commit
that referenced
this issue
Feb 14, 2025
Whether or not a binding is exported affects the binding resolution of any downstream modules that `using` the module that defines the binding. As such, it needs to fully participate in the invalidation mechanism, so that code which references bindings whose resolution may have changed get properly invalidated. To do this, move the `exportp` flag from Binding into a separate bitflag set that gets or'd into the BindingPartition `->kind` field. Note that we do not move `publicp` in the same way since it does not affect binding resolution. There is currently no mechanism to un-export a binding, although the system is set up to support this in the future (and Revise may want it). That said, at such a time, we may need to revisit the above decision on `publicp`. Lastly, I will note that this adds a fair number of additional invalidations. Most of these are unnecessary, as changing an export only affects the *downstream* users not the binding itself. I am planning to tackle this as part of a larger future PR that avoids invalidation when this is not required. Fixes #57377
Keno
added a commit
that referenced
this issue
Feb 15, 2025
Whether or not a binding is exported affects the binding resolution of any downstream modules that `using` the module that defines the binding. As such, it needs to fully participate in the invalidation mechanism, so that code which references bindings whose resolution may have changed get properly invalidated. To do this, move the `exportp` flag from Binding into a separate bitflag set that gets or'd into the BindingPartition `->kind` field. Note that we do not move `publicp` in the same way since it does not affect binding resolution. There is currently no mechanism to un-export a binding, although the system is set up to support this in the future (and Revise may want it). That said, at such a time, we may need to revisit the above decision on `publicp`. Lastly, I will note that this adds a fair number of additional invalidations. Most of these are unnecessary, as changing an export only affects the *downstream* users not the binding itself. I am planning to tackle this as part of a larger future PR that avoids invalidation when this is not required. Fixes #57377
Keno
added a commit
that referenced
this issue
Feb 15, 2025
Whether or not a binding is exported affects the binding resolution of any downstream modules that `using` the module that defines the binding. As such, it needs to fully participate in the invalidation mechanism, so that code which references bindings whose resolution may have changed get properly invalidated. To do this, move the `exportp` flag from Binding into a separate bitflag set that gets or'd into the BindingPartition `->kind` field. Note that we do not move `publicp` in the same way since it does not affect binding resolution. There is currently no mechanism to un-export a binding, although the system is set up to support this in the future (and Revise may want it). That said, at such a time, we may need to revisit the above decision on `publicp`. Lastly, I will note that this adds a fair number of additional invalidations. Most of these are unnecessary, as changing an export only affects the *downstream* users not the binding itself. I am planning to tackle this as part of a larger future PR that avoids invalidation when this is not required. Fixes #57377
KristofferC
pushed a commit
that referenced
this issue
Feb 15, 2025
Whether or not a binding is exported affects the binding resolution of any downstream modules that `using` the module that defines the binding. As such, it needs to fully participate in the invalidation mechanism, so that code which references bindings whose resolution may have changed get properly invalidated. To do this, move the `exportp` flag from Binding into a separate bitflag set that gets or'd into the BindingPartition `->kind` field. Note that we do not move `publicp` in the same way since it does not affect binding resolution. There is currently no mechanism to un-export a binding, although the system is set up to support this in the future (and Revise may want it). That said, at such a time, we may need to revisit the above decision on `publicp`. Lastly, I will note that this adds a fair number of additional invalidations. Most of these are unnecessary, as changing an export only affects the *downstream* users not the binding itself. I am planning to tackle this as part of a larger future PR that avoids invalidation when this is not required. Fixes #57377 (cherry picked from commit b27a24a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
At @Keno's request, broken out from #57328 since that is a bit of a meta issue.
Putting the following in a package:
and calling
now fails with an
UndefVar
error aboutS
not being defined.The text was updated successfully, but these errors were encountered: