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

implement setproperty! for modules #44137

Closed
wants to merge 5 commits into from
Closed

Conversation

simeonschaub
Copy link
Member

It doesn't seem like there was any good reason for disallowing this.
Especially now with #43671 merged I found myself wanting this.

@simeonschaub simeonschaub added the needs news A NEWS entry is required for this change label Feb 12, 2022
It doesn't seem like there was any good reason for disallowing this.
Especially now with #43671 merged I found myself wanting this.
src/builtins.c Outdated Show resolved Hide resolved
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

LGTM, though we probably will want codegen support, and to implement swapproperty!/modifyproperty!/replaceproperty! also

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

We might want to improve this text error to say "imported variable" or something similar:

julia> stdout; Core.setfield!(Main, :stdout, stdout)
stdout = Base.TTY(RawFD(13) open, 0 bytes waiting)
ERROR: cannot assign a value to variable Base.stdout from module Main
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1

@simeonschaub
Copy link
Member Author

Ok, I can look into what happens in codegen

@simeonschaub
Copy link
Member Author

I think I want to tackle codegen in another PR. This should ideally be consistent with regular assignment to a global, where we currently just call into jl_checked_assignment, so we should ideally teach LLVM to reason about both ways to assign to globals.

@simeonschaub simeonschaub added merge me PR is reviewed. Merge when all tests are passing and removed needs news A NEWS entry is required for this change labels Feb 15, 2022
@aviatesk
Copy link
Member

What will happen if we call setfield(M, :var, v) where M.var is a constant value?

@simeonschaub
Copy link
Member Author

It ends up calling jl_checked_assignement, so the behavior should be the same as with regular assignments to globals.

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

LGTM.

base/Base.jl Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
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".

@simeonschaub simeonschaub added triage This should be discussed on a triage call and removed merge me PR is reviewed. Merge when all tests are passing labels Feb 15, 2022
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Feb 17, 2022
simeonschaub added a commit that referenced this pull request Feb 17, 2022
This replaces #44137. As discussed on triage, instead of supporting
modules in `setfield!`, this adds two new builtins `getglobal` and
`setglobal!` explicitly for reading and modifying module bindings. We
should probably consider `getfield(::Module, ::Symbol)` to be
soft-deprecated, but I don't think we want to add any warnings since
that will likely just annoy people.
@simeonschaub
Copy link
Member Author

Closing in favor of #44231

simeonschaub added a commit that referenced this pull request Feb 18, 2022
This replaces #44137. As discussed on triage, instead of supporting
modules in `setfield!`, this adds two new builtins `getglobal` and
`setglobal!` explicitly for reading and modifying module bindings. We
should probably consider `getfield(::Module, ::Symbol)` to be
soft-deprecated, but I don't think we want to add any warnings since
that will likely just annoy people.
simeonschaub added a commit that referenced this pull request Feb 19, 2022
This replaces #44137. As discussed on triage, instead of supporting
modules in `setfield!`, this adds two new builtins `getglobal` and
`setglobal!` explicitly for reading and modifying module bindings. We
should probably consider `getfield(::Module, ::Symbol)` to be
soft-deprecated, but I don't think we want to add any warnings since
that will likely just annoy people.
simeonschaub added a commit that referenced this pull request Feb 19, 2022
This replaces #44137. As discussed on triage, instead of supporting
modules in `setfield!`, this adds two new builtins `getglobal` and
`setglobal!` explicitly for reading and modifying module bindings. We
should probably consider `getfield(::Module, ::Symbol)` to be
soft-deprecated, but I don't think we want to add any warnings since
that will likely just annoy people.
@vtjnash vtjnash deleted the sds/module_setfield branch March 2, 2022 15:20
simeonschaub added a commit that referenced this pull request Mar 14, 2022
This replaces #44137. As discussed on triage, instead of supporting
modules in `setfield!`, this adds two new builtins `getglobal` and
`setglobal!` explicitly for reading and modifying module bindings. We
should probably consider `getfield(::Module, ::Symbol)` to be
soft-deprecated, but I don't think we want to add any warnings since
that will likely just annoy people.

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants