disallow setindex on immutable values #34176
Conversation
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
vtjnash
changed the title
vtjnash
force-pushed
the
jn/no-immutable-setindex
branch
from
0f63778 to
77624c0
Compare
|
OK, added some extra checks and tests for those. We can never entirely stop you from shooting yourself in the foot, but we can still try. |
src/datatype.c
Outdated
| @@ -923,6 +924,10 @@ static void init_struct_tail(jl_datatype_t *type, jl_value_t *jv, size_t na) | |||
| JL_DLLEXPORT jl_value_t *jl_new_structv(jl_datatype_t *type, jl_value_t **args, uint32_t na) | |||
| { | |||
| jl_ptls_t ptls = jl_get_ptls_states(); | |||
| if (!jl_is_datatype(type) || type->layout == NULL) | |||
| jl_type_error("new", (jl_value_t*)jl_datatype_type, (jl_value_t*)type); | |||
| if ((type->ninitialized > na && !type->mutabl) || na > jl_datatype_nfields(type)) | |||
There was a problem hiding this comment.
Since we weren't doing this check at all before it doesn't really matter, but doesn't the ninitialized check make sense for both mutable and immutable structs?
There was a problem hiding this comment.
Hm, yeah, probably. I was concerned about causing too much breakage of otherwise mostly valid code, but it seems that jl_new_struct_uninit is usually what's needed when someone wants an incomplete mutable type, so I've gone ahead and removed the extra check here.
This was written fairly carefully to be safe, assuming it was not improperly optimized. But others are not as careful when copying this code. And it is just better not to break the object model and attempt to mutate constant values.
vtjnash
force-pushed
the
jn/no-immutable-setindex
branch
from
77624c0 to
f115b14
Compare
|
There should probably have been run a PkgEval here? |
|
Those all look expected to me. This is designed to prevent using ccall to mutate an immutable. |
|
The failures in LoweredCodeUtils and Revise appear to flow directly from JuliaInterpreter, so those three can basically be collapsed down to 1. For JuliaInterpreter, we use this to evaluate Any thoughts about alternatives? If we julia> nt = (a=1,)
(a = 1,)
julia> @interpret pairs(nt)That can dominate real-world test cases, e.g., as mentioned here running Julia's subarray tests under the interpreter is mostly a question of how fast you can evaluate |
|
Ah, nvm, this PR shows how to circumvent it. For the benefit of the other packages, one just has to prepare the args in advance and |
This was written fairly carefully to be safe, assuming it was not improperly optimized. But others are not as careful when copying this code. And it is just better not to break the object model and attempt to mutate constant values.
It's generally not a great idea to break the object model, even with the best of intentions, since you're inherently then relying on the compiler to make specific optimization choices and avoid others. This was written fairly carefully to be safe at the time, assuming it was not improperly optimized. But others are not as careful when copying this code. And it is just better not to break the object model and attempt to mutate constant values.
I started implementing equivalent optimizations of the old code ("SmallArray"), but then surmised that those seemed unlikely to be actually necessary.