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
#44231
Conversation
df33941
to
ae55bf6
Compare
I wonder whether we should also export |
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.
ae55bf6
to
bbb5458
Compare
Is this done? What remains? We export the others (fieldtype, getfield, setfield!, swapfield!, modifyfield!, replacefield!, etc) so I think it would be most consistent to export this too |
Yes, this just needs someone to review, other than that it's done on my part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems nearly complete to me
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Looks like this PR breaks the effect inference on g(bc) = bc.re
@descend g(1+1im)
[(+c,+e,+n,+t)]g(bc) in Main at REPL[2]:1
│ ─ %-1 = invoke g(::Complex{Int64})::Int64
1 1 ─ %1 = Base.getfield(bc, :re)::Int64 │╻ getproperty
└── return %1 │ After this PR: [(+c,!e,+n,+t)]g(bc) in Main at REPL[2]:1
│ ─ %-1 = invoke g(::Complex{Int64})::Int64
1 1 ─ %1 = Base.getfield(bc, :re)::Int64 │╻ getproperty
└── return %1 |
This replaces #44137. As discussed on triage, instead of supporting
modules in
setfield!
, this adds two new builtinsgetglobal
andsetglobal!
explicitly for reading and modifying module bindings. Weshould probably consider
getfield(::Module, ::Symbol)
to besoft-deprecated, but I don't think we want to add any warnings since
that will likely just annoy people.