Skip to content
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

Closed
KristofferC opened this issue Feb 12, 2025 · 0 comments · Fixed by #57405
Closed

Export invalidation fails to happen properly (binding partitions) #57377

KristofferC opened this issue Feb 12, 2025 · 0 comments · Fixed by #57405
Labels
regression Regression in behavior compared to a previous version
Milestone

Comments

@KristofferC
Copy link
Member

KristofferC commented Feb 12, 2025

At @Keno's request, broken out from #57328 since that is a bit of a meta issue.

Putting the following in a package:

module GeoParams
    module A
        module B
            using GeoParams
            export S
            struct S end
            module C
                using GeoParams
                h() = S() # commenting this line: ok
                x -> nothing # changing this to a non-anon function: ok
            end
        end
    end

    using .A.B
    export S # moving this export up above module A: ok
end

and calling

julia --project -e 'using GeoParams; @show GeoParams.A.B.C.S'

now fails with an UndefVar error about S not being defined.

@KristofferC KristofferC added the regression Regression in behavior compared to a previous version label Feb 12, 2025
@KristofferC KristofferC added this to the 1.12 milestone Feb 12, 2025
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
Labels
regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant