lowering: Refactor lowering for const and typed globals #54773
Conversation
|
DNM, because the base PR should be merged first. |
Not entirely true, since loading a module is a function call, but has a world-age change side effect. Similarly, deleting a method is a function call, but has a world-age change side effect. So 2 out of 3 world-change side effects I can think of are function calls, and only adding a method is currently a syntactic-only form. |
I meant world-age affecting syntax forms, which is currently |
Keno
force-pushed
the
kf/constgloballowerrefactor
branch
from
a6b3ce0
to
17320fa
Compare
|
Minor nit: currently
When? Currently functions are not allowed to observe new worlds, while global scope does so, but it also does update the world within a block of code to make it permitted |
|
Fair enough, the world age update at global scope is finer than I had thought (I thought it captured it at the entry to thunk evaluation, but it actually updates it every statement). I'm happy to go the other way and make both of these lower to intrinsics instead. Would you prefer that? |
|
Eventually, I think that will be desirable, though it is unimportant for the PR since that is a new feature it can be added later without breaking the syntactic form |
|
Alright, I'll leave it as is then and tweak the commit message. |
Keno
force-pushed
the
kf/constgloballowerrefactor
branch
from
17320fa
to
8816e4f
Compare
Keno
added
needs docs
Keno
force-pushed
the
kf/constgloballowerrefactor
branch
3 times, most recently
from
df97a48
to
dfaa660
Compare
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
Keno
force-pushed
the
kf/constgloballowerrefactor
branch
from
dfaa660
to
717283e
Compare
|
Fixed the |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
Keno
force-pushed
the
kf/constgloballowerrefactor
branch
from
717283e
to
499eed2
Compare
|
@nanosoldier |
| !!! compat "Julia 1.9" | ||
| This function requires Julia 1.9 or later. | ||
| """ | ||
| Core.set_binding_type! |
There was a problem hiding this comment.
This is definitively a breaking change, as it removes a documented API present since v1.9, no?
There was a problem hiding this comment.
It's neither exported nor public - I think it is rightly considered an implementation detail of the syntax. In addition, I didn't find any public package that actually uses this any other way, so providing a deprecation implementation seems gratuitous.
|
The package evaluation job you requested has completed - possible new issues were detected. |
This is a prepratory commit for #54654 to change the lowering of `const` and typed globals to be compatible with the new semantics. Currently, we lower `const a::T = val` to: ``` const a global a::T a = val ``` (which further expands to typed-globals an implicit converts). This works, because, under the hood, our const declarations are actually assign-once globals. Note however, that this is not syntactically reachable, since we have a parse error for plain `const a`: ``` julia> const a ERROR: ParseError: # Error @ REPL[1]:1:1 const a └─────┘ ── expected assignment after `const` Stacktrace: [1] top-level scope @ none:1 ``` However, this lowering is not atomic with respect to world age. The semantics in #54654 require that the const-ness and the value are established atomically (with respect to world age, potentially on another thread) or undergo invalidation. To resolve this issue, this PR changes the lowering of `const a::T = val` to: ``` let local a::T = val const (global a) = a end ``` where the latter is a special syntax form `Expr(:const, GlobalRef(,:a), :a)`. A similar change is made to const global declarations, which previously lowered via intrinsic, i.e. `global a::T = val` lowered to: ``` global a Core.set_binding_type!(Main, :a, T) _T = Core.get_binding_type(Main, :a) if !isa(val, _T) val = convert(_T, val) end a = val ``` This changes the `set_binding_type!` to instead be a syntax form `Expr(:globaldecl, :a, T)`. This is not technically required, but we currently do not use intrinsics for world-age affecting side-effects anywhere else in the system. In particular, after #54654, it would be illegal to call `set_binding_type!` in anything but top-level context. Now, we have discussed in the past that there should potentially be intrinsic functions for global modifications (method table additions, etc), currently only reachable through `Core.eval`, but such an intrinsic would require semantics that differ from both the current `set_binding_type!` and the new `:globaldecl`. Using an Expr form here is the most consistent with our current practice for these sort of things elsewhere and accordingly, this PR removes the intrinsic. Note that this PR does not yet change any syntax semantics, although there could in principle be a reordering of side-effects within an expression (e.g. things like `global a::(@isdefined(a) ? Int : Float64)` might behave differently after this commit. However, we never defined the order of side effects (which is part of what this is cleaning up, although, I am not formally defining any specific ordering here either - #54654 will do some of that), and that is not a common case, so this PR should be largely considered non-semantic with respect to the syntax change.
Keno
force-pushed
the
kf/constgloballowerrefactor
branch
from
499eed2
to
f00f2e6
Compare
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
Remaining pkgeval failure is |
Keno
removed
the
needs pkgeval
| # `const` does not distribute over assignments | ||
| const aconstassign = bconstassign = 2 | ||
| @test isconst(@__MODULE__, :aconstassign) | ||
| @test !isconst(@__MODULE__, :bconstassign) |
There was a problem hiding this comment.
This might be a bug? (no need to fix in this PR, though)
Following up #54773. Since top-level thunks including `:const` expressions are generally not optimized, they are not validated either, which can cause problems for interpreters like `REPLInterpreter` that perform inference on arbitrary top-level chunks.
This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early `isdefined` that forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid. To get around this, we extend the (internal) `isdefined` form to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need.
This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early `isdefined` that forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid. To get around this, we extend the (internal) `isdefined` form to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need.
This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early `isdefined` that forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid. To get around this, we extend the (internal) `isdefined` form to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need.
This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early `isdefined` that forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid. To get around this, we extend the (internal) `isdefined` form to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need.
This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early `isdefined` that forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid. To get around this, we extend the (internal) `isdefined` form to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need.
This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early `isdefined` that forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid. To get around this, we extend the (internal) `isdefined` form to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need.
This is a follow up to resolve a TODO left in #54773 as part of preparatory work for #54654. Currently, our lowering for type definition contains an early `isdefined` that forces a decision on binding resolution before the assignment of the actual binding. In the current implementation, this doesn't matter much, but with #54654, this would incur a binding invalidation we would like to avoid. To get around this, we extend the (internal) `isdefined` form to take an extra argument specifying whether or not to permit looking at imported bindings. If not, resolving the binding is not required semantically, but for the purposes of type definition (where assigning to an imported binding would error anyway), this is all we need.
|
Regarding the newly added |
|
Just a side note: the fact that the world-age of top-level thunks can be incremented with each statement might mean that new considerations are necessary for the correctness of inference on top-level chunks. |
Following up #54773. Required for external abstract interpreters that may run inference on arbitrary top-level thunks.
Following up #54773. Required for external abstract interpreters that may run inference on arbitrary top-level thunks.
This is a prepratory commit for #54654 to change the lowering of
constand typed globals to be compatible with the new semantics.Currently, we lower
const a::T = valto:(which further expands to typed-globals an implicit converts).
This works, because, under the hood, our const declarations are actually assign-once globals. Note however, that this is not syntactically reachable, since we have a parse error for plain
const a:However, this lowering is not atomic with respect to world age. The semantics in #54654 require that the const-ness and the value are established atomically (with respect to world age, potentially on another thread) or undergo invalidation.
To resolve this issue, this PR changes the lowering of
const a::T = valto:where the latter is a special syntax form
Expr(:const, GlobalRef(,:a), :a).A similar change is made to const global declarations, which previously lowered via intrinsic, i.e.
global a::T = vallowered to:This changes the
set_binding_type!to instead be a syntax formExpr(:globaldecl, :a, T). This is not technically required, but we currently do not use intrinsics for world-age affecting side-effects anywhere else in the system. In particular, after #54654, it would be illegal to callset_binding_type!in anything but top-level context. Now, we have discussed in the past that there should potentially be intrinsic functions for global modifications (method table additions, etc), currently only reachable throughCore.eval, but such an intrinsic would require semantics that differ from both the currentset_binding_type!and the new:globaldecl. Using an Expr form here is the most consistent with our current practice for these sort of things elsewhere and accordingly, this PR removes the intrinsic.Note that this PR does not yet change any syntax semantics, although there could in principle be a reordering of side-effects within an expression (e.g. things like
global a::(@isdefined(a) ? Int : Float64)might behave differently after this commit. However, we never defined the order of side effects (which is part of what this is cleaning up, although, I am not formally defining any specific ordering here either - #54654 will do some of that), and that is not a common case, so this PR should be largely considered non-semantic with respect to the syntax change.Also fixes #54787 while we're at it.