Skip to content

fix called-argument analysis for calls with splat #58070

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

Merged
merged 2 commits into from
May 10, 2025

Conversation

JeffBezanson
Copy link
Member

No description provided.

@JeffBezanson JeffBezanson added bugfix This change fixes an existing bug backport 1.12 Change should be backported to release-1.12 labels Apr 10, 2025
@KristofferC KristofferC mentioned this pull request Apr 16, 2025
51 tasks
@KristofferC KristofferC mentioned this pull request Apr 29, 2025
53 tasks
@fingolfin fingolfin requested review from vtjnash, c42f and mlechu May 8, 2025 00:23
@mlechu
Copy link
Member

mlechu commented May 8, 2025

julia> function sprint2(f::Function, args...)
           s = IOBuffer()
           f(s, args...)
           String(Base._unsafe_take!(s))
       end
sprint2 (generic function with 1 method)

julia> sprint2(Base.showerror, MethodError("hi", (Int,)))
""

inference bug? nevermind...

julia> function sprint2(f::Function, args...)
           s = IOBuffer()
           f(s, args...); sleep(1)
           String(Base._unsafe_take!(s))
       end
sprint2 (generic function with 1 method)

julia> sprint2(Base.showerror, MethodError("hi", (Int,)))
"MethodError: objects of type String are not callable.\nIn case you did not try calling it explicitly, check if a String has been passed as an argument to a method that expects a callable instead.\nThis error has been manually thrown, explicitly, so the method may exist but be intentionally marked as unimplemented."

@vtjnash
Copy link
Member

vtjnash commented May 8, 2025

It is an llvm bug (or rather, our bug in what we codegen for llvm). I'm looking into why we did what we did

vtjnash added a commit that referenced this pull request May 8, 2025
Fix some misuses of `jl_is_immutable_datatype`, and rename it to
indicate it is not a guarantee of immutable.

Refs: #58070

The cfunction code difference isn't normally emitted by codegen since it
is just a fallback, but can be observed with various code_llvm type
signatures for various combinations of abstract/union/concrete/mutable:

   f(x::T) where {T} = @cfunction identity Ref{T} (Ref{T},)
   code_llvm(f, (Any,), dump_module=true, raw=true)
   code_llvm(f, (Number,), dump_module=true, raw=true)
   code_llvm(f, (Union{Int32,Int64},), dump_module=true, raw=true)
   code_llvm(f, (Complex,), dump_module=true, raw=true)
   code_llvm(f, (Ref{Int},), dump_module=true, raw=true)
vtjnash added a commit that referenced this pull request May 8, 2025
Fix some misuses of `jl_is_immutable_datatype`, and rename it to
indicate it is not a guarantee of immutable.

Refs: #58070

The cfunction code difference isn't normally emitted by codegen since it
is just a fallback, but can be observed with various code_llvm type
signatures for various combinations of abstract/union/concrete/mutable:

   f(x::T) where {T} = @cfunction identity Ref{T} (Ref{T},)
   code_llvm(f, (Any,), dump_module=true, raw=true)
   code_llvm(f, (Number,), dump_module=true, raw=true)
   code_llvm(f, (Union{Int32,Int64},), dump_module=true, raw=true)
   code_llvm(f, (Complex,), dump_module=true, raw=true)
   code_llvm(f, (Ref{Int},), dump_module=true, raw=true)
@vtjnash vtjnash force-pushed the jb/splatiscalled branch from 8b73281 to 9584fc1 Compare May 9, 2025 19:05
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label May 9, 2025
KristofferC pushed a commit that referenced this pull request May 9, 2025
@KristofferC KristofferC mentioned this pull request May 9, 2025
58 tasks
@IanButterworth IanButterworth merged commit bbb0582 into master May 10, 2025
7 checks passed
@IanButterworth IanButterworth deleted the jb/splatiscalled branch May 10, 2025 22:27
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label May 11, 2025
KristofferC pushed a commit that referenced this pull request May 12, 2025
KristofferC pushed a commit that referenced this pull request May 12, 2025
charleskawczynski pushed a commit to charleskawczynski/julia that referenced this pull request May 12, 2025
KristofferC pushed a commit to DilumAluthgeBot/julia that referenced this pull request May 12, 2025
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Jun 4, 2025
KristofferC pushed a commit that referenced this pull request Jun 5, 2025
KristofferC pushed a commit that referenced this pull request Jun 5, 2025
KristofferC pushed a commit that referenced this pull request Jun 5, 2025
KristofferC pushed a commit that referenced this pull request Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants