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

WIP: remove fallback eltype that returns Any for all types #26852

Closed
wants to merge 2 commits into from

Conversation

JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented Apr 19, 2018

For a long time we've allowed eltype to be called on anything and return Any (eltype(:x), eltype(+), you name it). This feels very julia 0.1 to me and doesn't seem like a good thing.

Removing this method already uncovered a couple bugs, where iterators were silently and incorrectly getting Any from eltype because the wrong method was called.

The part of this I'm not sure about is the Random code, where the Sampler types have an eltype parameter, and call eltype on a constructor argument to populate it. Some of the types passed here do not have eltype methods, and indeed the tests pass if I just replace the eltype calls with Any. So maybe this type parameter is unused and can just be removed. cc @rfourquet

Currently I have the fallback eltype give a MethodError, but it could be a deprecation warning instead.

@JeffBezanson JeffBezanson added breaking This change will break code deprecation This change introduces or involves a deprecation triage This should be discussed on a triage call labels Apr 19, 2018
@@ -350,7 +350,7 @@ function precise_container_type(@nospecialize(arg), @nospecialize(typ), vtypes::
else
return Any[ p for p in tti0.parameters ]
end
elseif tti0 <: Array
elseif tti0 isa Type{<:Array{T}} where T
Copy link
Member

Choose a reason for hiding this comment

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

?

aren't these supposed to be equivalent, with the subtyping test likely being more reliable

Copy link
Member Author

Choose a reason for hiding this comment

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

Type{<:Array{T}} where T is a strict subtype of Type{<:Array}, since no value of T turns Array{T} into Array.

@@ -233,10 +233,10 @@ size(v::Pairs) = size(v.itr)
end
@inline done(v::Pairs, state) = done(v.itr, state)

eltype(::Type{Pairs{K, V}}) where {K, V} = Pair{K, V}
eltype(::Type{<:Pairs{K, V}}) where {K, V} = Pair{K, V}
Copy link
Member

Choose a reason for hiding this comment

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

need to now handle the cases where !@isdefined K or !@isdefined V

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and now I notice this can actually be deleted since it's covered by the eltype method for AbstractDict.

@JeffBezanson JeffBezanson added the needs news A NEWS entry is required for this change label Apr 19, 2018
@JeffBezanson JeffBezanson added this to the 1.0 milestone Apr 19, 2018
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Apr 19, 2018
@StefanKarpinski
Copy link
Member

Triage accepts.

@mauro3
Copy link
Contributor

mauro3 commented Apr 20, 2018

Why not give a deprecation instead of a method error?

@JeffBezanson JeffBezanson removed the needs news A NEWS entry is required for this change label Apr 20, 2018
@JeffBezanson
Copy link
Member Author

So, the real issue here seems to be that the default value of IteratorEltype is HasEltype. Without a fallback eltype, it would make more sense to make that EltypeUnknown, but then every type that defines eltype has to add a definition IteratorEltype(::Type{MyType}) = HasEltype(), and things will be pretty broken until they do. So this change might not actually be worth it.

Of course, IteratorSize is similar in that the default is HasLength, but that seems to be much less of an issue in practice than eltype.

@StefanKarpinski
Copy link
Member

Wouldn't it make sense to change the default to not having an eltype? That seems like a strange default to me.

@JeffBezanson
Copy link
Member Author

It's a nuisance; every time you define eltype you'd also have to define IteratorEltype for it to "take effect". But with HasEltype as the default, you get a method error for eltype, and at that point you can decide whether to define eltype or IteratorEltype(::Type{MyType}) = EltypeUnknown(). So it works surprisingly well. I'm not sure we should touch this until we have a better solution e.g. based on applicable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants