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

implement setproperty! for modules #44137

Closed
wants to merge 5 commits into from
Closed

implement setproperty! for modules #44137

Changes from 1 commit
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
12 changes: 10 additions & 2 deletions base/Base.jl
View file
Original file line number Diff line number Diff line change
@@ -29,7 +29,11 @@ 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
function setproperty!(x::Module, f::Symbol, v)
@inline
val::Core.get_binding_type(x, f) = v
return setfield!(x, f, val)
end
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))
@@ -41,7 +45,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)
@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))
2 changes: 0 additions & 2 deletions base/compiler/tfuncs.jl
View file
Original file line number Diff line number Diff line change
@@ -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
@@ -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
6 changes: 0 additions & 6 deletions doc/src/manual/variables-and-scoping.md
View file
Original file line number Diff line number Diff line change
@@ -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`,
33 changes: 21 additions & 12 deletions src/builtins.c
View file
Original file line number Diff line number Diff line change
@@ -940,22 +940,31 @@ 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 atomically");
jl_binding_t *b = jl_get_binding_wr((jl_module_t*)v, (jl_sym_t*)args[1], 1);
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];
}

6 changes: 3 additions & 3 deletions test/compiler/inference.jl
View file
Original file line number Diff line number Diff line change
@@ -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{}
20 changes: 20 additions & 0 deletions test/core.jl
View file
Original file line number Diff line number Diff line change
@@ -7698,3 +7698,23 @@ end
@test a == 1
@test b == Core.svec(2, 3)
end

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

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

m.x = 1
@test m.x === 1
setproperty!(m, :x, 2, :not_atomic)
@test m.x === 2
@test_throws ConcurrencyViolationError setproperty!(m, :x, 3, :release)
m.x = 4.
@test m.x === 4
end
10 changes: 0 additions & 10 deletions test/misc.jl
View file
Original file line number Diff line number Diff line change
@@ -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