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

Tuple constructor change from 0.5 to 0.6 #20969

Closed
sbromberger opened this issue Mar 10, 2017 · 16 comments
Closed

Tuple constructor change from 0.5 to 0.6 #20969

sbromberger opened this issue Mar 10, 2017 · 16 comments
Labels
docs This change adds or pertains to documentation

Comments

@sbromberger
Copy link
Contributor

sbromberger commented Mar 10, 2017

0.5:

julia> immutable Edge
           src::Int
           dst::Int
       end

julia> src(e::Edge) = e.src
src (generic function with 1 method)

julia> dst(e::Edge) = e.dst
dst (generic function with 1 method)

julia> import Base.convert

julia> convert(::Type{Tuple}, e::Edge) = (src(e), dst(e))
convert (generic function with 597 methods)

julia> e = Edge(1,2)
Edge(1,2)

julia> Tuple(e)
(1,2)

0.6:

julia> struct Edge
         src::Int
         dst::Int
       end

julia> src(e::Edge) = e.src
src (generic function with 1 method)

julia> dst(e::Edge) = e.dst
dst (generic function with 1 method)

julia> import Base.convert

julia> convert(::Type{Tuple}, e::Edge) = (src(e), dst(e))
convert (generic function with 700 methods)

julia> e = Edge(1,2)
Edge(1, 2)

julia> Tuple(e)
ERROR: MethodError: no method matching start(::Edge)
Closest candidates are:
  start(::SimpleVector) at essentials.jl:259
  start(::Base.MethodList) at reflection.jl:559
  start(::ExponentialBackOff) at error.jl:107
  ...
Stacktrace:
 [1] Tuple(::Edge) at ./tuple.jl:198

Not sure whether I missed something in NEWS again, but here we are.

@yuyichao yuyichao changed the title convert breakage from 0.5 to 0.6 Tuple constructor change from 0.5 to 0.6 Mar 10, 2017
@yuyichao yuyichao added the docs This change adds or pertains to documentation label Mar 10, 2017
@sbromberger
Copy link
Contributor Author

@yuyichao could you let me know how to fix this?

@yuyichao
Copy link
Contributor

#15516

@yuyichao
Copy link
Contributor

Well, you didn't define the constructor so define it if you want it.

@sbromberger
Copy link
Contributor Author

sbromberger commented Mar 10, 2017

Apparently Tuple requires a constructor instead of convert.

@JeffBezanson
Copy link
Member

I think the new Tuple constructors could probably be methods of convert instead. I'll try it; I don't expect it to break anything.

@JeffBezanson
Copy link
Member

JeffBezanson commented Mar 10, 2017

Hmm, on second thought I guess it didn't seem like a good idea to allow convert from arbitrary iterables to tuples. We generally use constructors for making collections from other iterators, and convert only between types that are fairly similar.

@sbromberger
Copy link
Contributor Author

sbromberger commented Mar 10, 2017

@JeffBezanson thanks. Does that mean that convert(::Type{Pair}, e::Edge) (which works, incidentally, in 0.5 and 0.6) is better off being a constructor? (I confess to being unclear about when to use convert and when to use a constructor.)

@JeffBezanson
Copy link
Member

When to define convert is a judgment call; the key thing is that it's called implicitly on assignment, so the types should be similar enough that that wouldn't cause surprises.

@musm
Copy link
Contributor

musm commented Mar 10, 2017

@sbromberger
Copy link
Contributor Author

sbromberger commented Mar 10, 2017

@musm - thanks. I'm going to close this out, I guess, since it looks like it was deliberate.

I'd really prefer "one way to do it" and not have to worry about judgement calls, but I understand that's not always possible. I'll default to explicit constructors, which means importing/extending Base.Pair and Base.Tuple, which seems ... wrong, somehow.

@JeffBezanson
Copy link
Member

It's interesting to think about merging convert and constructors. It has certainly come up before. The main issue is that some types naturally have idempotent constructors, and others don't. For example Int(Int(x)) === Int(x) always, but tuple(tuple(x)) !== tuple(x). However, obviously tuple is a separate function, not the same as Tuple. So we could get away with this if we had a rule that non-idempotent constructors must be separate functions, and not types themselves. We seem to do this a lot already, e.g. tuple and [ ]. This could potentially fix #15120. It would be interesting to review what non-idempotent constructors exist in packages.

The other, lesser, issue, is the potential for surprises or performance traps when an expensive "conversion" happens. It makes sense to be able to construct a Tuple from a generator, say, but do we want to be able to store a generator into an array of tuples and have it converted? Some of those cases can be addressed by separate functions, like parse and collect, so they don't have to be either constructors or conversions.

@andyferris
Copy link
Member

I think the general problem with idempotent constructors is with containers that nest. You can create a tuple(tuple(...)) and also static array SVector(SVector(1,2,3)). I'd like to be able to do both nested static arrays and be able to convert static arrays, but getting rid of convert so that I have to create a new svector function (and similarly for any other container type) doesn't seem like a net win to me.

Base.Array is immune since we use different syntax to populate the container (such as [[1,2,3]]), and just like many other mutable containers we use a constructor to allocate them and then mutate them with setindex! or push! or whatever. However, immutable containers are just as valid and they must be populated by the constructors.

@JeffBezanson
Copy link
Member

Unfortunately lots of container constructors already are idempotent. For example Set(Set(x)) doesn't create a set of sets, and Tuple(Tuple([1,2])) doesn't create a tuple of tuples. Both of them first convert another iterator to the target type, then do an identity conversion. I'm not sure what to do for SVector, but this seems to be an inconsistency we already have.

@andyferris
Copy link
Member

I guess we could require SVector((1,2,3)). It would certainly make the constructor logic easier to just expect an iterable (or indexable) object as input.

@StefanKarpinski
Copy link
Member

Unfortunately lots of container constructors already are idempotent.

Isn't that fortunate if you want to move towards expecting all constructors to be idempotent?

@JeffBezanson
Copy link
Member

Yes, I meant unfortunate for SVectors, not anybody else :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

No branches or pull requests

6 participants