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

Type piracy? #25

Open
dpo opened this issue Oct 4, 2019 · 13 comments · Fixed by #26
Open

Type piracy? #25

dpo opened this issue Oct 4, 2019 · 13 comments · Fixed by #26
Milestone

Comments

@dpo
Copy link
Contributor

dpo commented Oct 4, 2019

We're having an issue using Julia from the Juno REPL (which imports OrderedCollections), in which sort() of a Dict returns an OrderedDict even though the user didn't import OrderedCollections. The unexpected behavior is described here:

https://discourse.julialang.org/t/juno-repl-behaves-differently-than-terminal-repl/29469/2

The culprit appears to be :

julia> d = Dict(1 => "a", 3 => "b", 0 => "c")
Dict{Int64,String} with 3 entries:
  0 => "c"
  3 => "b"
  1 => "a"

julia> sort(d)
OrderedCollections.OrderedDict{Int64,String} with 3 entries:
  0 => "c"
  1 => "a"
  3 => "b"

julia> @which sort(d)
sort(d::Dict) in OrderedCollections at /Users/dpo/.julia/packages/OrderedCollections/E21Rb/src/dict_sorting.jl:21

Thanks.

@oxinabox
Copy link
Member

oxinabox commented Oct 4, 2019

I was sure @ararslan removed that ages ago.

We should just delete it.
Only question is if to tag a major or patch release.

@ararslan ararslan changed the title Type pyracy? Type piracy? Oct 4, 2019
@ararslan
Copy link
Member

ararslan commented Oct 4, 2019

If I did remove it, it was probably before OrderedCollections was split out from DataStructures, or something like that. I don't really remember but I agree this is bad. Since this has the potential to break downstream code and OrderedCollections has declared its version to be > 1.0, I think it should be a major release.

@oxinabox
Copy link
Member

oxinabox commented Oct 4, 2019

It might have happened during the period where the code existed in both places,
and it got removed from DataStructures but not removed from OrderedCollections,
and so it came back when DataStructures switched to just reexporting

@timholy
Copy link
Member

timholy commented Oct 7, 2019

Seems like a good idea to remove it. It was there when I split this out. Maybe there was an unmerged PR or something?

dpo added a commit to dpo/OrderedCollections.jl that referenced this issue Oct 7, 2019
@oxinabox
Copy link
Member

oxinabox commented Oct 8, 2019

Should not have been closed.
As only deprecated so far

@timholy
Copy link
Member

timholy commented Feb 19, 2021

This came up again. The deprecation has been in place for about 16 months, although it only made it into a release 9 months ago. That seems more than long enough. Shall we delete it and release OrderedCollections v2? Or is the plan to reintegrate into DataStructures?

@oxinabox
Copy link
Member

While there is a full and concrete plan for exactly how to get DataStructures up to 1.0 I don't have time to work on it.
So it is going to be a longtime before it is done, unless someone else finds time. JuliaCollections/DataStructures.jl#479 (comment)
And we can't merge OrderedCollections into DataStructures until that is done.
So I would not let that block anything.

But also I am in no rush to remove this type-piracy.
It is unlikely to be causing a problem for anyone; since it turns a method error into a non-method error.
and when we remove it, and trigger a breaking change we are going to cause about 15 human-hours of work to be done for little gain, even with CompatHelper, PR still need to be reviewed, merged, tagged. So say 10 minutes.
We have 84 public direct dependencies, and I am pretty sure at least 26 private ones. So call it 1000minutes is 16hr40min.
probably more because some people will incorrectly tag a breaking change to their package after this, triggering another round of cascading changes.
Doesn't seem worth it, unless we had more things to fix at the same time.
Things that are actually going to break real code.

@mschauer
Copy link

It is arguably also introducing a bug:

julia> eltype(dict)
Pair{Int64, Int64}
julia> collect(dict)
80200-element Vector{Pair{Int64, Int64}}:
julia> sort(dict, by=x->x.second)
ERROR: type Int64 has no field second

@oxinabox
Copy link
Member

oxinabox commented Dec 14, 2023

Following up from #110

Conclusion is we remove it either:

Which ever comes first.

@krynju
Copy link

krynju commented Feb 19, 2024

This is getting worse with 1.10 now out and is actually a critical error in some advanced sysimage use cases

What would it take to push for a 2.0 release? Are there any other planned 2.0 changes?

@mschauer
Copy link

Data point: even though I commented above, I forgot about this issue and now I notice that I have code which relies on this observable behaviour.

@fingolfin fingolfin added this to the v2.0.0 milestone Nov 25, 2024
@fingolfin
Copy link
Contributor

We now have over 350 direct dependencies according to https://juliahub.com/ui/Packages/General/OrderedCollections so I think waiting just made this worse, and will keep making it worse.

I have now created a milestone for https://github.com/JuliaCollections/OrderedCollections.jl/milestone/1 and will open a PR working towards 2.0.0.

I would restrict that release to things like removing this particular type piracy and perhaps removing a few other deprecated things. So that packages ideally can just list both v1 & v2 as compatible in their Project.toml, in order to minimize the pain of the transition.

But first there should be at least one more 1.x release with recent improvements.

@ericphanson
Copy link
Member

IMO a breaking change makes sense and I agree waiting more just makes it worse. I think some of the pain can be reduced by having very clear release notes with suggested upgrade path (e.g. forward compatibility + use both versions in compat as you suggested) and maybe also a discourse post announcing the change. I think uncertainty and time spent seeking information is a big chunk of the upgrade cost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment