search

JuliaLang / Compat.jl

Compatibility across Julia versions
Other
142 stars 117 forks source link

Deprecate `sort` for `NTuple` and other iterables #811

Closed martinholters closed 7 months ago

martinholters commented 7 months ago

As this functionality was removed from Julia in https://github.com/JuliaLang/julia/pull/52010.

CC @ericphanson

martinholters commented 7 months ago

This is on top of #810 to see whether tests now pass on nightly. EDIT: Rebased after merging #810.

codecov[bot] commented 7 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (db2077e) 92.83% compared to head (e900807) 93.23%.

Files Patch % Lines
src/deprecated.jl 96.15% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #811 +/- ## ========================================== + Coverage 92.83% 93.23% +0.39% ========================================== Files 2 3 +1 Lines 335 340 +5 ========================================== + Hits 311 317 +6 + Misses 24 23 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

martinholters commented 7 months ago

The sort tests still pass (except for the @allocated ones), but of course are quite noise with many depwarns now. Should we remove them?

KristofferC commented 7 months ago

No, this has to be reverted. We cannot have this level of type piracy in Compat just because it existed on Julia master for a while.

I mean, Compat should not even overload things in Base. You are supposed to have to call Compat.sort if you want to opt in to the "future" behavior of sort.

martinholters commented 7 months ago

No, this has to be reverted. We cannot have this level of type piracy in Compat just because it existed on Julia master for a while.

Yeah, that is super unfortunate, but if we just revert this, we have to do a new major version if we take SemVer seriously (which I like to do). And new major versions of Compat are not welcome by everyone, ref. https://github.com/JuliaLang/Compat.jl/pull/767#issuecomment-1127423282. A middle ground might be to keep this (deprecated) in Compat but only for Julia < v1.10. That would mean that for sort on general iterables, you'd need Julia < 1.10. Not sure that's much better, but at least, Compat doesn't pollute Base forever then.

I mean, Compat should not even overload things in Base. You are supposed to have to call Compat.sort if you want to opt in to the "future" behavior of sort.

Hm, we've traditionally added methods to functions in Base whenever possible, reasoning that we're only doing it for past Julia versions where we know it's safe. And it's convenient from a user perspective. But I agree that always requiring the Compat. prefix would be much safer. We might want to adopt that in the future. But it doesn't help us with the mess we're in here now.

KristofferC commented 7 months ago

Can we at least run PkgEval on this change and see if it breaks something in practice and if not we just remove it? Having this deprecated (but still there) in all of < 1.10 is awful. It shouldn't even be there in the first place since this is a feature that was introduced in 1.10 and should not automatically be available on pre-1.10. We don't backport features for a reason.

The type of development where things that are tested on Julia master quickly get put into Compat in a type piracy way and then "cannot be removed due to semver" is completely unworkable. This package is depended on by a lot of other packages and any type piracy in it is very bad.

FWIW, I also think this change broke a package (https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/46617e5_vs_0ba6ec2/ITensorVisualizationBase.primary.log) where, ironically, their own type piracy (https://github.com/ITensor/NDTensors.jl/blob/19c079c554b546c57b3f5bd72db15c39112ff5d0/src/tupletools.jl#L162) stopped working.

So at least we have some evidence that the added type piracy here broke a package so based on semver (which we take seriously) it should be reverted.

ericphanson commented 7 months ago

In the past, “not matching Julia” has been called a bugfix btw: #745

martinholters commented 7 months ago

Yeah, these are good arguments for reverting.

Can we at least run PkgEval on this change

Can nanosoldier to this for us or would someone need to run it on their own box?

Meanwhile, searching for Compat = "4.9" only found NDTensors, so that's a likely candidate for being broken -- again.