Skip to content

Proper constraints on CC types in collections #7504

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.

Sign up for GitHub

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

Jump to bottom
Closed
szeiger wants to merge 1 commit into from
Closed

Proper constraints on CC types in collections #7504

szeiger wants to merge 1 commit into from

Conversation

@szeiger
Copy link
Contributor

@szeiger szeiger commented Dec 7, 2018

I started this in the hope of eventually fixing scala/bug#11188 but after 2 days I'm giving up. This part here appears to be useful enough on its own. The type constraints are better than before and we don't need AnyConstr anymore.

I ran into several problems trying to take this further:

  • Due to Views we can't have nice constraints such as CC[X] <: Seq[X] in SeqOps. The worst case is MapOps which requires CC to be not just MapView but even a tupled anonymous type function on View.
  • There's a compiler bug that creates bad forwarders. I ran into it when trying to enforce constraints to SortedMap instead of only Map in SortedMapOps.
  • What we really need for ++: on maps gives wrong type (++: is deprecated but it should still work) bug#11188 is a constraint C <: CC[A] (and similar ones for other kinds of CC) but this is not possible because of variance. We need CC[+_] for +A but CC[_] for A. This becomes a problem when we get to mutable (and therefore invariant) collection types that extend generic (covariant) collection types. We can't tie the knot anymore because the variance changes.
  • MapOps uses a CC type constraint to coax the compiler into producing methods with return types that don't clash after erasure with inherited methods from IterableOps and SeqOps. Adding the "correct" constraints break this.
  • I also tried to add proper self type constraints in the hope of getting rid of coll and some casts but it quickly gets very verbose because the compiler requires some excessive repetition of self types (in some cases but not others; this might be a bug) even when extending this type directly (so it should automatically be part of the self type).

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Dec 7, 2018
@smarter
Copy link
Member

smarter commented Dec 7, 2018

I attempted something a bit like this for Sets in 2623ca8

We can't tie the knot anymore because the variance changes.

I forgot the details but I don't recall having this problem in my PR, I did however introduce an InvariantSetOps for some operations: b98ddb9

@smarter
Copy link
Member

smarter commented Dec 7, 2018 •
edited
Loading

(The context for these commits is #6728)

@szeiger szeiger mentioned this pull request Dec 7, 2018
Closed
@szeiger szeiger force-pushed the wip/iterable-constraints branch from c7c9872 to 022208f Compare December 10, 2018 12:43
@szeiger
Add proper type constraints for CC type parameters
73188f0 
This also provides a nice solution for AnyConstr (see
scala#7077 and
scala#7072): We simply don’t need it
anymore.

We really want to constrain both IterableCC and MapCC to Iterable
(the latter because it’s needed for MapView) but that causes collisions
after erasure, so we have to go back to IterableOnce for IterableCC.
Using something more specific (like MapOps with Iterable) for
MapCC is not possible because in MapView the MapCC is not actually
MapView but only View.
@szeiger szeiger force-pushed the wip/iterable-constraints branch from 022208f to 73188f0 Compare December 11, 2018 10:35
@szeiger szeiger mentioned this pull request Dec 11, 2018
Closed
@szeiger szeiger added the library:collections PRs involving changes to the standard collection library label Dec 11, 2018
@lrytz lrytz requested a review from julienrf December 12, 2018 11:39
@julienrf
Copy link
Contributor

julienrf commented Dec 15, 2018 •
edited
Loading

This part here appears to be useful enough on its own.

It’s not clear to me what the benefits of this PR are, given that it doesn’t fix scala/bug#11188. Can you also summarize them? It seems to me that it essentially adds narrower bounds on type parameters, which I think adds more constraints to the use site.

@julienrf
Copy link
Contributor

julienrf commented Dec 15, 2018

I think it should be possible to get rid of some .coll calls or .toIterable calls maybe?

@szeiger
Copy link
Contributor Author

szeiger commented Jan 3, 2019

The new constraints have no effect on users. For example, IterableOps was always intended to be an implementation trait for Iterable and is not used in any other context. Adding the proper constraints should avoid unnecessary casts but with the limited changes I was able to make I didn't come across any such case. There could be some in user code. The only direct benefit here is getting rid of AnyConstr.

@julienrf
Copy link
Contributor

julienrf commented Jan 4, 2019 •
edited
Loading

The new constraints have no effect on users.

It might make it harder to abstract over IterableOps. For instance, the upper bound constraint is now propagated to the Iterable.WithFilter class definition. I think similar use cases may happen in user code bases. We can see another example in the files/run/partialfun.scala test.

That being said, we could argue that IsIterable is the recommended way to abstract over IterableOps (this is more true with IsSeq, for SeqOps), which are unaffected by this PR. So, there is still a way to write generic code without being bothered by bound constraints.

@Ichoran Ichoran self-assigned this Jan 10, 2019
@SethTisue
Copy link
Member

SethTisue commented Feb 5, 2019

@szeiger needs rebase

after rebase, should we run the 2.13 community build on this before merging?

@Ichoran
Copy link
Contributor

Ichoran commented Feb 8, 2019

@SethTisue @julienrf - I'm leaving this one to you guys (and @szeiger of course) to get over the finish line.

@julienrf
Copy link
Contributor

julienrf commented Feb 8, 2019

If the PR allows us to remove unnecessary calls to .coll or .toIterable then I see the value. But in its current state I don’t think it is worth it because it just makes things more constrained with the only value being that there is no need for AnyConstr (which is needed only for Dotty compatibility, btw).

@szeiger
Copy link
Contributor Author

szeiger commented Feb 14, 2019

More constrained = more correct. But it's getting too late for such a big change and we still can't get the constraints exactly right so there's not a huge benefit.

@szeiger szeiger closed this Feb 14, 2019
@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Feb 14, 2019
@szeiger szeiger mentioned this pull request Aug 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julienrf julienrf Awaiting requested review from julienrf

Assignees

@Ichoran Ichoran

Labels

library:collections PRs involving changes to the standard collection library

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@szeiger @smarter @julienrf @SethTisue @Ichoran @scala-jenkins