improve minor inferrabilities #42141
Conversation
base/shell.jl
Outdated
| @@ -88,19 +88,19 @@ function shell_parse(str::AbstractString, interpolate::Bool=true; | |||
| in_double_quotes = !in_double_quotes | |||
| i = consume_upto!(arg, s, i, j) | |||
| elseif !in_single_quotes && c == '\\' | |||
| if !isempty(st) && peek(st)[2] in ('\n', '\r') | |||
| if !isempty(st) && (peek(st)::Pair{Int,Char})[2] in ('\n', '\r') | |||
There was a problem hiding this comment.
Hmm, I am a bit surprised these type annotations are needed. st should have a concrete type, right? So is peek itself not type stable?
There was a problem hiding this comment.
yeah, peek(s::Stateful) may return nothing when s is empty.
There was a problem hiding this comment.
But since nothing[2] is an error, this would also be fixed via #40880?
There was a problem hiding this comment.
peek returns nothing if the iterator has been exhausted. So one could replace the !isempty check by branching on the return type instead and that should allow to get away without the type-assertion, but I'm not sure that's better.
There was a problem hiding this comment.
But since nothing[2] is an error, this would also be fixed via #40880?
No, #40880 only works after a target local variable is used:
if !isempty(st) && (p = peek(st); p[2] in ('\n', '\r'))
p # p::Pair{Int,Char} after #40880Unfortunately, it's invalid if the compiler automatically optimizes peek(st)[2] in ('\n', '\r') into something like peek(st)::Pair{Int,Char}[2] in ('\n', '\r'), since it changes the semantics (the possible exception is changed to TypeError from MethodError).
peekreturnsnothingif the iterator has been exhausted. So one could replace the!isemptycheck by branching on the return type instead and that should allow to get away without the type-assertion, but I'm not sure that's better.
Yeah, I'd like to go with type assertion for now. I think maybe we can refine our iteration protocol so that our type lattice can recognize the remaining length of an iterator ? It'd look very similar to array shape analysis.
| for (j, c) in st | ||
| j, c = j::Int, c::eltype(str) | ||
| j, c = j::Int, c::C |
There was a problem hiding this comment.
But why are these necessary at all?
There was a problem hiding this comment.
I made this PR just because these (very minor) type instabilities appeared when running JET on program that uses shell_parse.
I don't think there is any effective improvement in terms of performance though.
There was a problem hiding this comment.
Sure, fine with me, they certainly won't hurt. I was just wondering whether there might be a deeper issue that could be solved to make the assertions unnecessary.
There was a problem hiding this comment.
From inference point of view, it might be appreciated if peek (and similar functions) just throws for empty input, but I believe it's intentionally implemented to return nothing for such cases.
There was a problem hiding this comment.
Yes, iterate should be inferred as Union{Nothing, Tuple{Pair{Int, C}, State}} here, but the for loop lowering should make it obvious to inference that we are in the !==nothing branch when destructuring the value to (j, c), so it should be possible to infer j::Int and c::C. But I guess if str doesn't have a concrete type, inference of iterate (due to less precise argument types) will be worse? And my point was: Can we fix this earlier, so that iterate can be inferred better? But probably it's not worth worrying too much about it, so please feel free to carry on with your PR as is.
There was a problem hiding this comment.
Ah, I get your point. And yeah, it seems like this annotation will help when str is not concrete:
julia> @eval Base code_typed((AbstractString,); optimize=false) do str
s = SubString(str, firstindex(str))
s = rstrip_shell(lstrip(s))
st = Iterators.Stateful(pairs(s))
for (j, c) in st
return j, c
end
return eltype(str)
end |> first
CodeInfo(
1 ─ %1 = Base.firstindex(str)::Core.Const(1)
│ (s = Base.SubString(str, %1))::SubString
│ %3 = Base.lstrip(s)::SubString
│ (s = Base.rstrip_shell(%3))::SubString
│ %5 = Base.Iterators.Stateful::Core.Const(Base.Iterators.Stateful)
│ %6 = Base.pairs(s)::Base.Generator{_A, Base.var"#6#7"{Pair}} where _A
│ (st = (%5)(%6))::Base.Iterators.Stateful
│ %8 = st::Base.Iterators.Stateful
│ (@_3 = Base.iterate(%8))::Union{Nothing, Tuple{Any, Nothing}}
│ %10 = (@_3 === nothing)::Bool
│ %11 = Base.not_int(%10)::Bool
└── goto #4 if not %11
2 ─ %13 = @_3::Tuple{Any, Nothing}
│ %14 = Core.getfield(%13, 1)::Any
│ %15 = Base.indexed_iterate(%14, 1)::Any
│ (j = Core.getfield(%15, 1))::Any
│ (@_6 = Core.getfield(%15, 2))::Any
│ %18 = Base.indexed_iterate(%14, 2, @_6)::Any
│ (c = Core.getfield(%18, 1))::Any
│ Core.getfield(%13, 2)::Core.Const(nothing)
│ %21 = Core.tuple(j, c)::Tuple{Any, Any}
└── return %21
3 ─ Core.Const(:(@_3 = Base.iterate(%8, %20)))::Union{}
│ Core.Const(:(@_3 === nothing))::Union{}
│ Core.Const(:(Base.not_int(%24)))::Union{}
│ Core.Const(:(goto %28 if not %25))::Union{}
└── Core.Const(:(goto %13))::Union{}
4 ┄ %28 = Base.eltype(str)::Core.Const(Char)
└── return %28
) => Union{Tuple{Any, Any}, Type{Char}}(eltype(str still can be inferred as Type{Char})
No description provided.