JuliaLang
Type-assert `opts.delim_flags` to reduce invalidations
In particular the last line of invalidations from PythonCall.jl (CC @cjdoris):
inserting |(x::Number, y::PythonCall.Py) @ PythonCall.Core ~/git/PythonCall.jl/src/Core/Py.jl:432 invalidated:
mt_backedges: 1: signature Tuple{typeof(|), Int64, Any} triggered MethodInstance for Base.JuliaSyntax._register_kinds!(::Dict{Int64, Union{Module, Symbol}}, ::Dict{UInt16, String}, ::Dict{String, UInt16}, ::Module, ::Int64, ::Vector{String}) (0 children)
2: signature Tuple{typeof(|), Int64, Any} triggered MethodInstance for Base.JuliaSyntax._register_kinds!(::Dict{Int64, Union{Module, Symbol}}, ::Dict{UInt16, String}, ::Dict{String, UInt16}, ::Module, ::Int64, ::Vector{String}) (0 children)
3: signature Tuple{typeof(|), Int64, Any} triggered MethodInstance for JuliaSyntax._register_kinds!(::Dict{Int64, Union{Module, Symbol}}, ::Dict{UInt16, String}, ::Dict{String, UInt16}, ::Module, ::Int64, ::Vector{String}) (0 children)
4: signature Tuple{typeof(|), Bool, Any} triggered MethodInstance for <=(::Any, ::Char) (0 children)
5: signature Tuple{typeof(|), Bool, Any} triggered MethodInstance for <=(::Char, ::Any) (9 children)
6: signature Tuple{typeof(|), UInt16, Any} triggered MethodInstance for Base.JuliaSyntax.parse_function_signature(::Base.JuliaSyntax.ParseState, ::Bool) (1155 children)
The issue is that ops.delim_flags was getting inferred to Any:
2220 ambiguous_parens::Bool = opts::NamedTuple.maybe_grouping_parens::Any &&
2221 (peek_behind(ps::JuliaSyntax.ParseState)::@NamedTuple{kind::JuliaSyntax.Kind, flags::UInt16, orig_kind::JuliaSyntax.Kind, is_leaf::Bool}.kind::JuliaSyntax.Kind in KSet"macrocall $")::Bool
2222 emit(ps::JuliaSyntax.ParseState, mark::JuliaSyntax.ParseStreamPosition, K"tuple", (PARENS_FLAG::Core.Const(0x0100)|opts::NamedTuple.delim_flags::Any)::Any)
2223 if ambiguous_parens::Bool
But from reading parse_brackets() I think it's safe to restrict it to a RawFlags
Fixing https://github.com/JuliaLang/JuliaSyntax.jl/issues/600 should fix this too.
Looks like the invalidations are also triggered by InitialValues.jl (in the dependency tree of OhMyThreads.jl):
inserting |(x, ::Union{InitialValues.NonspecificInitialValue, InitialValues.SpecificInitialValue{typeof(|)}}) @ InitialValues ~/.julia/packages/InitialValues/OWP8V/src/InitialValues.jl:161 invalidated:
mt_backedges: 1: signature Tuple{typeof(|), Int64, Any} triggered MethodInstance for Base.JuliaSyntax._register_kinds!(::Dict{Int64, Union{Module, Symbol}}, ::Dict{UInt16, String}, ::Dict{String, UInt16}, ::Module, ::Int64, ::Vector{String}) (0 children)
2: signature Tuple{typeof(|), Int64, Any} triggered MethodInstance for Base.JuliaSyntax._register_kinds!(::Dict{Int64, Union{Module, Symbol}}, ::Dict{UInt16, String}, ::Dict{String, UInt16}, ::Module, ::Int64, ::Vector{String}) (0 children)
3: signature Tuple{typeof(|), Bool, Any} triggered MethodInstance for <=(::Char, ::Any) (0 children)
4: signature Tuple{typeof(|), Bool, Any} triggered MethodInstance for <=(::Any, ::Char) (0 children)
5: signature Tuple{typeof(|), Missing, Any} triggered MethodInstance for <=(::Any, ::Char) (1 children)
6: signature Tuple{typeof(|), Missing, Any} triggered MethodInstance for <=(::Char, ::Any) (10 children)
7: signature Tuple{typeof(|), UInt16, Any} triggered MethodInstance for Base.JuliaSyntax.parse_function_signature(::Base.JuliaSyntax.ParseState, ::Bool) (1161 children)
Would it make sense to type assert the whole opts? Is it only delim_flags that is non-inferred?
I had another look at this and I don't think we can realistically type-assert opts since it's a NamedTuple with varying fields depending on the function that's passed to it. The only reliable thing is to specialize on the function argument as suggested in #600, so I did that in 28fa41d. Checked it with Cthulhu and can confirm that everything's concretely typed now.
I realize it's a bit late, but is there any chance of getting this in for 1.12.2? :face_with_peeking_eye: If so then I can make any necessary backport PRs.
Checked it with Cthulhu and can confirm that everything's concretely typed now.
Can you just confirm that Cthulhu shows the status quo as bad? I have some memory that I tried to look at this with Cthuklhu and it showed that even the current master was ok (which is not true but it still showed that...)
Hmm this is weird, with Revise loaded the second invocation to descend always shows concretely typed variables, even on main. And without it loaded the variables never show up as concretely typed, even on this branch 😕 I think I'm gonna try building Julia with this branch to a proper check.
Yeah this isn't doing it for some reason. For some reason Cthulhu also doesn't show an option to descend into parse_brackets from parse_function_signature.
I tried this commit on the backports-1.12 branch: https://github.com/JamesWrigley/JuliaSyntax.jl/commit/a148565ecbbdf1269955d6ee77975901736380c3
Tested with InitialValues and DataStructures, and it fixes all the JuliaSyntax invalidations for them. This is what Cthulhu reports (without Revise loaded):
2160 opts::NamedTuple = parse_brackets(ps::JuliaSyntax.ParseState, K")") do had_commas, had_splat, num_semis, num_subexprs
2161 _parsed_call = was_eventually_call(ps)
2162 _needs_parse_call = peek(ps, 2) ∈ KSet"( ."
2163 _is_anon_func = (!_needs_parse_call && !_parsed_call) || had_commas
2164 return (needs_parameters = _is_anon_func,
2165 is_anon_func = _is_anon_func,
2166 parsed_call = _parsed_call,
2167 needs_parse_call = _needs_parse_call,
2168 maybe_grouping_parens = !had_commas && !had_splat && num_semis == 0 && num_subexprs == 1)
2169 end::NamedTuple::Type{JuliaSyntax.var"#parse_function_signature##0#parse_function_signature##1"{JuliaSyntax.ParseState}}
2170 is_anon_func::Bool = (opts::NamedTuple.is_anon_func::Any::Bool)::Bool
2171 parsed_call::Bool = (opts::NamedTuple.parsed_call::Any::Bool)::Bool
2172 needs_parse_call::Bool = (opts::NamedTuple.needs_parse_call::Any::Bool)::Bool
2173 maybe_grouping_parens::Bool = (opts::NamedTuple.maybe_grouping_parens::Any::Bool)::Bool
2174 delim_flags::UInt16 = (opts::NamedTuple.delim_flags::Any::RawFlags::Type{UInt16})::UInt16
2175 if is_anon_func::Bool
2176 # function (x) body end ==> (function (tuple-p x) (block body))
2177 # function (x::f()) end ==> (function (tuple-p (::-i x (call f))) (block))
2178 # function (x,y) end ==> (function (tuple-p x y) (block))
2179 # function (x=1) end ==> (function (tuple-p (= x 1)) (block))
2180 # function (;x=1) end ==> (function (tuple-p (parameters (= x 1))) (block))
2181 # function (f(x),) end ==> (function (tuple-p (call f x)) (block))
2182 ambiguous_parens::Bool = maybe_grouping_parens::Bool &&
2183 (peek_behind(ps::JuliaSyntax.ParseState)::@NamedTuple{kind::JuliaSyntax.Kind, flags::UInt16, orig_kind::JuliaSyntax.Kind, is_leaf::Bool}.kind::JuliaSyntax.Kind in KSet"macrocall $")::Bool
2184 emit(ps::JuliaSyntax.ParseState, mark::JuliaSyntax.ParseStreamPosition, K"tuple", (PARENS_FLAG::Core.Const(0x0020)|delim_flags::UInt16)::UInt16)
I fully admit it's a bit ugly... Not sure if it makes sense to merge this PR into main now that JuliaSyntax has been merged into the Julia repo, but if folks think this is acceptable I can make another PR to the upstream version. Is there still time to get it in for Julia 1.12.2?
Heya, we just moved JuliaSyntax into Base in https://github.com/JuliaLang/julia/pull/59870 - if you want to re-propose this PR over there, I've created a branch https://github.com/JuliaLang/JuliaSyntax.jl/tree/pr-for-Base/601 to make this easier.
To make use of the branch, you can use the following steps:
- Clone JuliaLang/julia
- Add JuliaSyntax as a git remote
- Check out pr-for-Base/601
- Rebase on top of master
For example:
# git clone [email protected]:JuliaLang/julia julia_dir
# cd julia_dir
git remote add JuliaSyntax [email protected]:JuliaLang/JuliaSyntax.jl
git fetch JuliaSyntax
git checkout pr-for-Base/601
git rebase origin/master
Sorry this wasn't dealt with prior to the big move!
I think if https://github.com/JamesWrigley/JuliaSyntax.jl/commit/a148565ecbbdf1269955d6ee77975901736380c3 is going to be merged then it should be into this repo and then I can make the other PR to Base. But it looks like we'll need a backports-1.12 branch because main and the commit in 1.12 have diverged quite a bit.
Bump, sorry to be a pain @KristofferC but it'd be really great to get this in for 1.12.3.
I think if https://github.com/JamesWrigley/JuliaSyntax.jl/commit/a148565ecbbdf1269955d6ee77975901736380c3 is going to be merged then it should be into this repo and then I can make the other PR to Base.
The "standard" order for changes is the other way around - Try first to open the this change for master (in JuliaLang/julia), then follow up afterward with the backport PR's (which will be an easy review / merge at that point)
Thanks James sorry for the bother.
Ideally I'd like to never release code out of this repo again, but if it's most expedient to do so for the purposes of backporting patches to 1.12 that's ok.
I'll close this now that the fix has been merged upstream. Backporting to Julia 1.12 will require a backports branch and another PR since main has diverged a bit from the 1.12 release.