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

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

Open
StefanKarpinski opened this issue Nov 30, 2018 · 7 comments
Open

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

StefanKarpinski opened this issue Nov 30, 2018 · 7 comments

Comments

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Nov 30, 2018

Since #22193 and #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
@StefanKarpinski StefanKarpinski changed the title isimmutable("abc") == false !isimmutable("abc") Nov 30, 2018
@StefanKarpinski StefanKarpinski changed the title !isimmutable("abc") !isimmutable("abc") && !isimmutable(:abc) Nov 30, 2018
@fingolfin
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Sponsor Member

vtjnash commented Nov 30, 2018

dup
#25349 for Symbol. Potentially String and SimpleVector would make more sense marked as immutable though.

@fingolfin
Copy link
Contributor

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
Copy link
Sponsor Member

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
Copy link
Contributor

musm commented Feb 22, 2022

related #43229

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

@DavidSagan
Copy link

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.

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

No branches or pull requests

7 participants