Base.isidentifier(::Symbol) should check normalization

[stevengj]

As discussed on discourse, while the Symbol("...") foo constructor allows one to construct invalid Julia identifiers and in particular identifiers lacking normalization (an intentional decision way back in #5462), the Base.isidentifier function doesn't check for normalization of Symbol arguments.

This also has the consequence that show is incorrect for non-normalized Symbols, since it checks isidentifier to see whether they can be printed as :foo rather than as Symbol("foo"):

julia> "e\u0301" # e with acute accent, not NFC normalized
"é"

julia> Symbol("e\u0301") == :é   # correct: :é gets normalized
false

julia> length(String(Symbol("e\u0301"))), length(String(:é))
(2, 1)

julia> Base.isidentifier(Symbol("e\u0301"))   # incorrect: should check normalization
true

julia> Symbol("e\u0301") # incorrect display: not the same as :é
:é

My suggestion is that

julia/base/show.jl

Line 1538 in 66e9410

isidentifier(s::Symbol) = isidentifier(string(s))

should be changed to something like:

function isidentifier(s::Symbol)
    str = string(s)
    ct(codepoint::UInt32) = get(Unicode._julia_charmap, codepoint, codepoint)
    isidentifier(str) && str == Unicode.normalize(str; stable=true, compose=true, chartransform=ct)
end

(Note that isidentifier should continue to not check normalization for ::String arguments, since in that case it has a slightly different meaning — it checks whether the string can be parsed into a valid identifier.)

[stevengj]

(Marking as "good first issue" since someone needs only to make a 5-line patch from the above suggestion, clarify the isidentifier docs, then add a test.)

[KristofferC]

Note that Symbol(::String) can currently be constant propagated so it would be good to make sure that is still the case.

[stevengj]

@KristofferC, the proposed fix won't affect Symbol(::String) at all, only isidentifier(::Symbol).

[re1san]

Hey @stevengj I made the above changes as suggested and added a test, but the build process fails due to segmentation fault. I have attached the log below.
log.txt

[stevengj]

Maybe the chartransform argument needs to be in global scope, so that it is a constant. No, that shouldn't matter. May be some kind of bootstrapping thing?