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

ImmutableSetFactory.empty results in StackOverflowError #6457

Closed
scabug opened this issue Oct 1, 2012 · 13 comments
Closed

ImmutableSetFactory.empty results in StackOverflowError #6457

scabug opened this issue Oct 1, 2012 · 13 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Oct 1, 2012

scala> object Factory extends generic.ImmutableSetFactory[immutable.Set] {}
defined module Factory

scala> Factory.empty
java.lang.StackOverflowError
	at scala.collection.generic.ImmutableSetFactory.newBuilder(ImmutableSetFactory.scala:18)
	at scala.collection.generic.GenericCompanion.empty(GenericCompanion.scala:37)
	at scala.collection.generic.ImmutableSetFactory.newBuilder(ImmutableSetFactory.scala:18)
	at scala.collection.generic.GenericCompanion.empty(GenericCompanion.scala:37)
...

Not a regression, I saw the same thing in 2.9.1.

@scabug
Copy link
Author

@scabug scabug commented Oct 1, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6457?orig=1
Reporter: @axel22
Affected Versions: 2.9.1, 2.10.0-M7

@scabug
Copy link
Author

@scabug scabug commented Jan 31, 2013

@JamesIry said:
ImmutableSetFactory says

  def newBuilder[A]: Builder[A, CC[A]] = new SetBuilder[A, CC[A]](empty[A])

GenericCompanion says

  def empty[A]: CC[A] = newBuilder[A].result

So, um, boom.

@scabug
Copy link
Author

@scabug scabug commented Jan 31, 2013

@JamesIry said:
The parallel case with ImmutableMapFactory is instructive

scala> object Factory extends scala.collection.generic.ImmutableMapFactory[scala.collection.immutable.HashMap] {}
<console>:7: error: object creation impossible, since method empty in class MapFactory of type [A, B]=> scala.collection.immutable.HashMap[A,B] is not defined
       object Factory extends scala.collection.generic.ImmutableMapFactory[scala.collection.immutable.HashMap] {}
              ^

The reason is that ImmutableSetFactory mixes in GenericCompanion by way of generic.SetFactory by way of GenericSeqCompanion. ImmutableMapFactory does not mix in GenericCompanion.

@scabug
Copy link
Author

@scabug scabug commented Jan 31, 2013

@JamesIry said:
Given the above analysis I don't think this can be fixed without breaking forward compatibility. So kicking it down the road.

@scabug
Copy link
Author

@scabug scabug commented Mar 15, 2013

@adriaanm said:
Un-assigning to foster work stealing, as announced in https://groups.google.com/forum/?fromgroups=#!topic/scala-internals/o8WG4plpNkw

@scabug
Copy link
Author

@scabug scabug commented Jul 10, 2013

@adriaanm said:
Unassigning and rescheduling to M6 as previous deadline was missed.

@scabug
Copy link
Author

@scabug scabug commented Oct 21, 2013

@retronym said:
Marking as a blocker so we don't forget about this one before 2.11

@scabug
Copy link
Author

@scabug scabug commented Jan 14, 2014

@adriaanm said:
Should we reschedule for RC1? Rex, could you perhaps have a look in time for M8?

@scabug
Copy link
Author

@scabug scabug commented Jan 14, 2014

@Ichoran said:
Looking now.

@scabug
Copy link
Author

@scabug scabug commented Jan 14, 2014

@Ichoran said:
Everyone who implements it successfully actually overrides empty. So I propose we just remove the definition of empty and leave it abstract. Although it is formally possible to define the empty set inside the builder, in practice that's normally awkward. I'll submit a PR if we wish. Ideally you'd deprecate the non-abstractness to give people a chance to change, but I don't think we have that annotation? (It's the lesser-known cousin of deprecatedOverriding.)

@scabug
Copy link
Author

@scabug scabug commented Jan 14, 2014

@Ichoran said:
Oh, I should add by "everyone" I mean "just the Scala library, but nobody in the giant bundle of jars that Adriaan got off of Maven inherited from ImmutableSetFactory, so it's just us that we have to worry about, probably".

@scabug
Copy link
Author

@scabug scabug commented Jan 14, 2014

@adriaanm said:
The .001% of Scala users hit by this source incompatibility are likely to forgive us.

@scabug
Copy link
Author

@scabug scabug commented Jan 15, 2014

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

Successfully merging a pull request may close this issue.

None yet
2 participants