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.

Sign up for GitHub

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

Jump to bottom

improve minor inferrabilities #42141

Merged
merged 1 commit into from
Sep 8, 2021
Merged

improve minor inferrabilities #42141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions base/shell.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,22 +49,24 @@ function shell_parse(str::AbstractString, interpolate::Bool=true;
empty!(innerlist)
end

C = eltype(str)
P = Pair{Int,C}
for (j, c) in st
j, c = j::Int, c::eltype(str)
j, c = j::Int, c::C
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@martinholters martinholters Sep 8, 2021 •
edited
Loading

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.

Copy link
Member Author

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})

if !in_single_quotes && !in_double_quotes && isspace(c)
i = consume_upto!(arg, s, i, j)
append_2to1!(args, arg)
while !isempty(st)
# We've made sure above that we don't end in whitespace,
# so updating `i` here is ok
(i, c) = peek(st)::Pair{Int,eltype(str)}
(i, c) = peek(st)::P
isspace(c) || break
popfirst!(st)
end
elseif interpolate && !in_single_quotes && c == '$'
i = consume_upto!(arg, s, i, j)
isempty(st) && error("\$ right before end of command")
stpos, c = popfirst!(st)::Pair{Int,eltype(str)}
stpos, c = popfirst!(st)::P
isspace(c) && error("space not allowed right after \$")
if startswith(SubString(s, stpos), "var\"")
# Disallow var"#" syntax in cmd interpolations.
Expand All @@ -88,19 +90,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)::P)[2] in ('\n', '\r')
i = consume_upto!(arg, s, i, j) + 1
if popfirst!(st)[2] == '\r' && peek(st)[2] == '\n'
if popfirst!(st)[2] == '\r' && (peek(st)::P)[2] == '\n'
i += 1
popfirst!(st)
end
while !isempty(st) && peek(st)[2] in (' ', '\t')
while !isempty(st) && (peek(st)::P)[2] in (' ', '\t')
i = nextind(str, i)
_ = popfirst!(st)
end
elseif in_double_quotes
isempty(st) && error("unterminated double quote")
k, c′ = peek(st)
k, c′ = peek(st)::P
if c′ == '"' || c′ == '$' || c′ == '\\'
i = consume_upto!(arg, s, i, j)
_ = popfirst!(st)
Expand Down
2 changes: 1 addition & 1 deletion base/strings/unicode.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ function iterate(g::GraphemeIterator, i_=(Int32(0),firstindex(g.s)))
y === nothing && return nothing
c0, k = y
while k <= ncodeunits(s) # loop until next grapheme is s[i:j]
c, ℓ = iterate(s, k)
c, ℓ = iterate(s, k)::NTuple{2,Any}
isgraphemebreak!(state, c0, c) && break
j = k
k = ℓ
Expand Down