JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.72k stars 5.48k forks source link

!isimmutable("abc") && !isimmutable(:abc) #30210

Open StefanKarpinski opened 5 years ago

StefanKarpinski commented 5 years ago

Since https://github.com/JuliaLang/julia/issues/22193 and https://github.com/JuliaLang/julia/pull/22954 Strings are supposed to be immutable, yet:

julia> isimmutable("abc")
false

Similarly, Symbols have been immutable and ===-comparable for ever and yet:

julia> isimmutable(:abc)
false

Pointed out by @fingofin on discourse. Also potentially of interest:

julia> isstructtype(String)
true
fingolfin commented 5 years ago

So I tried this naive patch, but this results in a segfault during make:

diff --git a/src/jltypes.c b/src/jltypes.c
index e89f552afa..8ca60d6658 100644
--- a/src/jltypes.c
+++ b/src/jltypes.c
@@ -2162,7 +2162,7 @@ void jl_init_types(void) JL_GC_DISABLED

     jl_abstractstring_type = jl_new_abstracttype((jl_value_t*)jl_symbol("AbstractString"), core, jl_any_type, jl_emptysvec);
     jl_string_type = jl_new_datatype(jl_symbol("String"), core, jl_abstractstring_type, jl_emptysvec,
-                                     jl_emptysvec, jl_emptysvec, 0, 1, 0);
+                                     jl_emptysvec, jl_emptysvec, 0, 0, 0);
     jl_string_type->instance = NULL;
     jl_compute_field_offsets(jl_string_type);

This is the segfault; it reliably goes away if I undo the patch, and comes back if I re-apply it:

...
    CC src/jltypes.o
    LINK usr/lib/libjulia.1.1.dylib
    JULIA usr/lib/julia/sys.ji
coreio.jl
exports.jl
essentials.jl
ctypes.jl
gcutils.jl
generator.jl
reflection.jl
options.jl
promotion.jl
tuple.jl
pair.jl
traits.jl
range.jl
expr.jl
error.jl
bool.jl
number.jl
int.jl
operators.jl
pointer.jl
refvalue.jl
refpointer.jl
checked.jl
indices.jl
array.jl
abstractarray.jl
subarray.jl
views.jl
abstractdict.jl
iterators.jl
namedtuple.jl
hashing.jl
rounding.jl
float.jl
julia(37356,0x7fff73aef000) malloc: *** error for object 0x1133a1320: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug

signal (6): Abort trap: 6
in expression starting at float.jl:57
__pthread_kill at /usr/lib/system/libsystem_kernel.dylib (unknown line)
Allocations: 1596640 (Pool: 1596344; Big: 296); GC: 2
/bin/sh: line 1: 37356 Abort trap: 6           /Users/mhorn/Projekte/foreign/julia/usr/bin/julia -g1 -O0 -C "native" --output-ji /Users/mhorn/Projekte/foreign/julia/usr/lib/julia/sys.ji.tmp --startup-file=no --warn-overwrite=yes --sysimage /Users/mhorn/Projekte/foreign/julia/usr/lib/julia/corecompiler.ji sysimg.jl
*** This error might be fixed by running `make clean`. If the error persists, try `make cleanall`. ***
make[1]: *** [Makefile:198: /Users/mhorn/Projekte/foreign/julia/usr/lib/julia/sys.ji] Error 1
make: *** [Makefile:78: julia-sysimg] Error 2
chethega commented 5 years ago

The most obvious modification would be a subset of

julia> @eval Base isstructtype(::Type{Symbol}) = false
julia> @eval Base isstructtype(::Type{String}) = false
julia> @eval Base isimmutable(::Symbol) = true
julia> @eval Base isimmutable(::String) = true

That is, make them immutable non-structs in Reflection.jl, and not for codegen / deep internals.

But then, one would need to check whether we want this: Do we e.g. want to permit finalizers on String and Symbol? Currently they are permitted.

I'd guess that object identity should respect finalizers, so that is a point in favor of Reflection.jl-immutability (gcutils.jl). Next, we have isdefined and getfield tfuncs. There, the immutability is potentially dangerous but naively looks OK (don't trust me on that). Next, abstract_call_method_with_const_args in abstractInterpretation.jl already special-cases String as if it were immutable (that check looks like dead code to me but would become live if we change this). Then, lift_leaves in passes.jl. I certainly don't understand the context, but it looks like Strings will fail out anyway, so behavior wouldn't change. All these people would need to chime in and either special case to preserve old behavior or say that the new behavior is fine.

vtjnash commented 5 years ago

dup https://github.com/JuliaLang/julia/pull/25349 for Symbol. Potentially String and SimpleVector would make more sense marked as immutable though.

fingolfin commented 5 years ago

That issue on symbols makes some sense to me -- though it would be great if this was part of the documentation, as I'd never have guessed this reasoning (which is to say, it would be helpful if one could learn about it from the manual, and thus learn quite a lot beyond it, too).

JeffBezanson commented 5 years ago

This is because our struct model is not yet able to express the layout of String, so fieldtypes(String) === (). If it were marked immutable, it would look like a singleton. Same with Symbol.

musm commented 2 years ago

related https://github.com/JuliaLang/julia/issues/43229

Can we just clarify in documentation the technical reason behind the output and close out this issue?

DavidSagan commented 10 months ago

If the ismutable and isimmutable functions are not to be modified, It would be very nice if there were functions (not sure of the best names) that actually returns whether the argument is mutable or not.