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.

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

Fix type-instability of ∘ and Some when wrapping types #35980

Merged
merged 4 commits into from
May 24, 2020

Conversation

tkf
Copy link
Member

@tkf tkf commented May 21, 2020

It looks fixing type instability of closure is hard (#35970). So, can we add a workaround for cases like Float64 ∘ first?

fix #34849


I also added a similar fix for Some

Before

julia> typeof(Float64 ∘ first)
Base.var"#62#63"{DataType,typeof(first)}

julia> typeof(Some(Int))
Some{DataType}

After

julia> typeof(Float64 ∘ first)
Base.ComposedFunction{Type{Float64},typeof(first)}

julia> typeof(Some(Int))
Some{Type{Int64}}

@tkf tkf changed the title Fix type-instability of f ∘ g when f or g is a type Fix type-instability of ∘ and Some when wrapping types May 21, 2020
show(io, c.g)
end

show(io::IO, ::MIME"text/plain", c::ComposedFunction) = show(io, c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a fallback for this.

Copy link
Member Author

@tkf tkf May 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, text/plain output is rather ugly:

julia> uppercase∘first
(::Base.ComposedFunction{typeof(uppercase),typeof(first)}) (generic function with 1 method)

I think this is due to show(io::IO, ::MIME"text/plain", f::Function).

But defining show here was incorrect because MIME is not defined here. I moved it to show.jl where show(io::IO, ::MIME"text/plain", f::Function) is defined.

With this definition, I get

julia> uppercase∘first
uppercase ∘ first

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, I was missing that it's a subtype of Function.

@JeffBezanson
Copy link
Member

Yes I think this is a reasonable approach for now. For example we already do this for Generators.

@nalimilan
Copy link
Member

This is clearly and improvement for composed functions, but is it so clear for Some? I can't decide whether it's better to have Some(Int) be a Some{Type{Int}} or a Some{DataType}. For example [Int] isa Vector{DataType}... Do we have concrete examples that could justify one choice or the other?

@tkf
Copy link
Member Author

tkf commented Jun 1, 2020

It is somewhat unfortunate that typeof([Int]) === Vector{DataType} because, IIUC, it is highly discouraged to dispatch on the representation of the type. People still can use [Int] isa Vector{<:Type} but a lot of users may not notice if they see Vector{DataType} in the REPL.

Do we have concrete examples that could justify one choice or the other?

Is type-stability not enough to justify the change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specialize ∘ on type arguments?
3 participants