-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, peek(s::Stateful)
may return nothing
when s
is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But since nothing[2]
is an error, this would also be fixed via #40880?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 #40880
Unfortunately, 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
).
peek
returnsnothing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why are these necessary at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.