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.

Sign up for GitHub

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

Jump to bottom

implement setproperty! for modules #44137

Closed
wants to merge 5 commits into from
Closed

implement setproperty! for modules #44137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
898f853
implement `setproperty!` for modules
simeonschaub Feb 12, 2022
e599ae0
require atomic writes
simeonschaub Feb 15, 2022
6e98d68
tweak error message
simeonschaub Feb 15, 2022
edf2512
add NEWS and compat annotations
simeonschaub Feb 15, 2022
b4ecd43
change `:release` to `:monotonic`
simeonschaub Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ New language features
ability of these objects ([#43305]).
* Empty n-dimensional arrays can now be created using multiple semicolons inside square brackets, i.e. `[;;;]` creates a 0×0×0 `Array`. ([#41618])
* Type annotations can now be added to global variables to make accessing the variable type stable. ([#43671])
* It is now possible to assign to bindings in another module using `setproperty!(::Module, ::Symbol, x)`. ([#44137])

Language changes
----------------
Expand Down
7 changes: 5 additions & 2 deletions base/Base.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ macro noinline() Expr(:meta, :noinline) end
# with ambiguities by defining a few common and critical operations
# (and these don't need the extra convert code)
getproperty(x::Module, f::Symbol) = (@inline; getfield(x, f))
setproperty!(x::Module, f::Symbol, v) = setfield!(x, f, v) # to get a decent error
getproperty(x::Type, f::Symbol) = (@inline; getfield(x, f))
setproperty!(x::Type, f::Symbol, v) = error("setfield! fields of Types should not be changed")
getproperty(x::Tuple, f::Int) = (@inline; getfield(x, f))
Expand All @@ -41,7 +40,11 @@ setproperty!(x, f::Symbol, v) = setfield!(x, f, convert(fieldtype(typeof(x), f),
dotgetproperty(x, f) = getproperty(x, f)

getproperty(x::Module, f::Symbol, order::Symbol) = (@inline; getfield(x, f, order))
setproperty!(x::Module, f::Symbol, v, order::Symbol) = setfield!(x, f, v, order) # to get a decent error
function setproperty!(x::Module, f::Symbol, v, order::Symbol=:monotonic)
@inline
val::Core.get_binding_type(x, f) = v
return setfield!(x, f, val, order)
end
getproperty(x::Type, f::Symbol, order::Symbol) = (@inline; getfield(x, f, order))
setproperty!(x::Type, f::Symbol, v, order::Symbol) = error("setfield! fields of Types should not be changed")
getproperty(x::Tuple, f::Int, order::Symbol) = (@inline; getfield(x, f, order))
Expand Down
2 changes: 0 additions & 2 deletions base/compiler/tfuncs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,6 @@ function _getfield_tfunc(@nospecialize(s00), @nospecialize(name), setfield::Bool
if isa(name, Const)
nv = name.val
if isa(sv, Module)
setfield && return Bottom
if isa(nv, Symbol)
return abstract_eval_global(sv, nv)
end
Expand Down Expand Up @@ -865,7 +864,6 @@ function _getfield_tfunc(@nospecialize(s00), @nospecialize(name), setfield::Bool
return Bottom
end
if s <: Module
setfield && return Bottom
hasintersect(widenconst(name), Symbol) || return Bottom
return Any
end
Expand Down
6 changes: 6 additions & 0 deletions base/docs/basedocs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1979,6 +1979,9 @@ declared `@atomic`, this specification is mandatory. Otherwise, if not declared
as `@atomic`, it must be `:not_atomic` if specified.
See also [`setproperty!`](@ref Base.setproperty!).

!!! compat "Julia 1.8"
`setfield!` on modules requires at least Julia 1.8.

# Examples
```jldoctest
julia> mutable struct MyMutableStruct
Expand Down Expand Up @@ -2731,6 +2734,9 @@ The syntax `a.b = c` calls `setproperty!(a, :b, c)`.
The syntax `@atomic order a.b = c` calls `setproperty!(a, :b, c, :order)`
and the syntax `@atomic a.b = c` calls `getproperty(a, :b, :sequentially_consistent)`.

!!! compat "Julia 1.8"
`setproperty!` on modules requires at least Julia 1.8.

See also [`setfield!`](@ref Core.setfield!),
[`propertynames`](@ref Base.propertynames) and
[`getproperty`](@ref Base.getproperty).
Expand Down
6 changes: 0 additions & 6 deletions doc/src/manual/variables-and-scoping.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,6 @@ julia> module D
b = a # errors as D's global scope is separate from A's
end;
ERROR: UndefVarError: a not defined

julia> module E
import ..A # make module A available
A.a = 2 # throws below error
end;
ERROR: cannot assign variables in other modules
```

If a top-level expression contains a variable declaration with keyword `local`,
Expand Down
4 changes: 2 additions & 2 deletions doc/src/manual/variables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,13 @@ julia> pi
π = 3.1415926535897...

julia> pi = 3
ERROR: cannot assign a value to variable MathConstants.pi from module Main
ERROR: cannot assign a value to imported variable MathConstants.pi from module Main

julia> sqrt(100)
10.0

julia> sqrt = 4
ERROR: cannot assign a value to variable Base.sqrt from module Main
ERROR: cannot assign a value to imported variable Base.sqrt from module Main
```

## [Allowed Variable Names](@id man-allowed-variable-names)
Expand Down
35 changes: 23 additions & 12 deletions src/builtins.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,22 +940,33 @@ JL_CALLABLE(jl_f_setfield)
enum jl_memory_order order = jl_memory_order_notatomic;
JL_NARGS(setfield!, 3, 4);
if (nargs == 4) {
JL_TYPECHK(getfield, symbol, args[3]);
JL_TYPECHK(setfield!, symbol, args[3]);
order = jl_get_atomic_order_checked((jl_sym_t*)args[3], 0, 1);
}
jl_value_t *v = args[0];
jl_datatype_t *st = (jl_datatype_t*)jl_typeof(v);
size_t idx = get_checked_fieldindex("setfield!", st, v, args[1], 1);
int isatomic = !!jl_field_isatomic(st, idx);
if (isatomic == (order == jl_memory_order_notatomic))
jl_atomic_error(isatomic ? "setfield!: atomic field cannot be written non-atomically"
: "setfield!: non-atomic field cannot be written atomically");
jl_value_t *ft = jl_field_type_concrete(st, idx);
if (!jl_isa(args[2], ft))
jl_type_error("setfield!", ft, args[2]);
if (order >= jl_memory_order_acq_rel || order == jl_memory_order_release)
jl_fence(); // `st->[idx]` will have at least relaxed ordering
set_nth_field(st, v, idx, args[2], isatomic);
if (st == jl_module_type) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In recent discussions, @JeffBezanson considered getfield on modules to be a mistake and preferred a separate builtin instead for 2.0. Maybe we should avoid repeating this for setfield! and just go straight for a separate builtin?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but I think with the current getfield that would arguably be more of a special case, no? I'd be happy to change that though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffBezanson should say which he prefers. I had suggested implementing the getbinding builtin right away also and just keeping getfield working with the intent to deprecate in 2.0. That way there wouldn't be an asymmetry and getfield would just have a bit of a legacy quirk.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think we should have getglobal and setglobal!, eventually deprecate the use of getfield for this, and hook up getproperty and setproperty!. Triage also brings up that there is no way other than eval to declare a global const, though we do have set_binding_type!. I prefer "global" here, it just feels like jargon-y than "binding".

JL_TYPECHK(setfield!, symbol, args[1]);
if (order == jl_memory_order_notatomic)
jl_atomic_error("setfield!: module binding cannot be written non-atomically");
jl_binding_t *b = jl_get_binding_wr((jl_module_t*)v, (jl_sym_t*)args[1], 1);
if (order >= jl_memory_order_acq_rel || order == jl_memory_order_release)
jl_fence();
jl_checked_assignment(b, args[2]);
}
else {
size_t idx = get_checked_fieldindex("setfield!", st, v, args[1], 1);
int isatomic = !!jl_field_isatomic(st, idx);
if (isatomic == (order == jl_memory_order_notatomic))
jl_atomic_error(isatomic ? "setfield!: atomic field cannot be written non-atomically"
: "setfield!: non-atomic field cannot be written atomically");
jl_value_t *ft = jl_field_type_concrete(st, idx);
if (!jl_isa(args[2], ft))
jl_type_error("setfield!", ft, args[2]);
if (order >= jl_memory_order_acq_rel || order == jl_memory_order_release)
jl_fence(); // `st->[idx]` will have at least relaxed ordering
set_nth_field(st, v, idx, args[2], isatomic);
}
return args[2];
}

Expand Down
2 changes: 1 addition & 1 deletion src/codegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3873,7 +3873,7 @@ static Value *global_binding_pointer(jl_codectx_t &ctx, jl_module_t *m, jl_sym_t
assert(b != NULL);
if (b->owner != m) {
char *msg;
(void)asprintf(&msg, "cannot assign a value to variable %s.%s from module %s",
(void)asprintf(&msg, "cannot assign a value to imported variable %s.%s from module %s",
jl_symbol_name(b->owner->name), jl_symbol_name(s), jl_symbol_name(m->name));
emit_error(ctx, msg);
free(msg);
Expand Down
2 changes: 1 addition & 1 deletion src/module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m JL_PROPAGATES_ROOT,
}
else if (error) {
JL_UNLOCK(&m->lock);
jl_errorf("cannot assign a value to variable %s.%s from module %s",
jl_errorf("cannot assign a value to imported variable %s.%s from module %s",
jl_symbol_name(b->owner->name), jl_symbol_name(var), jl_symbol_name(m->name));
}
}
Expand Down
6 changes: 3 additions & 3 deletions test/compiler/inference.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1725,10 +1725,10 @@ end
@test setfield!_tfunc(Some, Symbol, Int) === Union{}
@test setfield!_tfunc(Some{Any}, Int, Int) === Union{}
@test setfield!_tfunc(Some, Int, Int) === Union{}
@test setfield!_tfunc(Const(@__MODULE__), Const(:v), Int) === Union{}
@test setfield!_tfunc(Const(@__MODULE__), Const(:v), Int) === Int
@test setfield!_tfunc(Const(@__MODULE__), Int, Int) === Union{}
@test setfield!_tfunc(Module, Const(:v), Int) === Union{}
@test setfield!_tfunc(Union{Module,Base.RefValue{Any}}, Const(:v), Int) === Union{}
@test setfield!_tfunc(Module, Const(:v), Int) === Int
@test setfield!_tfunc(Union{Module,Base.RefValue{Any}}, Const(:v), Int) === Int
@test setfield!_tfunc(ABCDconst, Const(:a), Any) === Union{}
@test setfield!_tfunc(ABCDconst, Const(:b), Any) === Union{}
@test setfield!_tfunc(ABCDconst, Const(:d), Any) === Union{}
Expand Down
19 changes: 19 additions & 0 deletions test/core.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7698,3 +7698,22 @@ end
@test a == 1
@test b == Core.svec(2, 3)
end

@testset "setproperty! on modules" begin
m = Module()
@eval m global x::Int

@test_throws ConcurrencyViolationError setfield!(m, :x, 1)
setfield!(m, :x, 2, :release)
@test m.x === 2
@test_throws ConcurrencyViolationError setfield!(m, :x, 3, :not_atomic)
@test_throws ErrorException setfield!(m, :x, 4., :release)

m.x = 1
@test m.x === 1
setproperty!(m, :x, 2, :release)
@test m.x === 2
@test_throws ConcurrencyViolationError setproperty!(m, :x, 3, :not_atomic)
m.x = 4.
@test m.x === 4
end
10 changes: 0 additions & 10 deletions test/misc.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,16 +393,6 @@ let s = Set(1:100)
@test summarysize([s]) > summarysize(s)
end

# issue #13021
let ex = try
Main.x13021 = 0
nothing
catch ex
ex
end
@test isa(ex, ErrorException) && ex.msg == "cannot assign variables in other modules"
end

## test conversion from UTF-8 to UTF-16 (for Windows APIs)

# empty arrays
Expand Down