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

Rework recent, buggy change to immutable.TreeMap #9078

Merged
merged 1 commit into from Jun 23, 2020

Conversation

retronym
Copy link
Member

@retronym retronym commented Jun 23, 2020

We need to favour keys from the left that equivalent (but ne),
while favouring values from the right.

Revert then rework aa8bf99

Fixes scala/bug#12042 (at the second attempt)

@scala-jenkins scala-jenkins added this to the 2.13.4 milestone Jun 23, 2020
@retronym retronym requested a review from mkeskells June 23, 2020 08:59
@retronym retronym modified the milestones: 2.13.4, 2.13.3 Jun 23, 2020
@retronym retronym added library:collections PRs involving changes to the standard collection library prio:blocker release blocker (used only by core team, only near release time) backport candidate labels Jun 23, 2020
@retronym retronym self-assigned this Jun 23, 2020
@retronym retronym marked this pull request as draft June 23, 2020 10:43
@retronym
Copy link
Member Author

retronym commented Jun 23, 2020

Thread safety bug in scala-reflect?

https://scala-ci.typesafe.com/job/scala-2.13.x-validate-main/12271/testReport/junit/test.files/run_/reflection_valueclasses_derived_scala/

!    1 - run/reflection-valueclasses-derived.scala  [output differs]
% diff /home/jenkins/workspace/scala-2.13.x-validate-main/test/files/run/reflection-valueclasses-derived.check /home/jenkins/workspace/scala-2.13.x-validate-main/test/files/run/reflection-valueclasses-derived-run.log
@@ -1,3 +1,27 @@
+Uncaught exception on thread Thread[Reflector-61,5,main]: java.lang.IndexOutOfBoundsException: 2 is out of bounds (min 0, max 0)
+java.lang.IndexOutOfBoundsException: 2 is out of bounds (min 0, max 0)
+	at scala.collection.mutable.ArrayBuffer.apply(ArrayBuffer.scala:97)
+	at scala.reflect.internal.tpe.TypeConstraints.containsSymbol(TypeConstraints.scala:205)
+	at scala.reflect.internal.tpe.TypeConstraints.solveOne$1(TypeConstraints.scala:256)
+	at scala.reflect.internal.tpe.TypeConstraints.$anonfun$solve$8(TypeConstraints.scala:290)
+	at scala.reflect.internal.tpe.TypeConstraints.solve(TypeConstraints.scala:290)
+	at scala.reflect.internal.tpe.TypeConstraints.solve$(TypeConstraints.scala:218)
+	at scala.reflect.internal.SymbolTable.solve(SymbolTable.scala:28)
+	at scala.reflect.internal.Types$ExistentialType.withTypeVars(Types.scala:3209)
+	at scala.reflect.internal.tpe.TypeComparers.thirdTry$1(TypeComparers.scala:553)
+	at scala.reflect.internal.tpe.TypeComparers.secondTry$1(TypeComparers.scala:520)
+	at scala.reflect.internal.tpe.TypeComparers.firstTry$1(TypeComparers.scala:496)
+	at scala.reflect.internal.tpe.TypeComparers.isSubType2(TypeComparers.scala:616)
+	at scala.reflect.internal.tpe.TypeComparers.isSubType1(TypeComparers.scala:348)
+	at scala.reflect.internal.tpe.TypeComparers.isSubType(TypeComparers.scala:306)
+	at scala.reflect.internal.tpe.TypeComparers.isSubType$(TypeComparers.scala:268)
+	at scala.reflect.internal.SymbolTable.isSubType(SymbolTable.scala:28)
+	at scala.reflect.internal.Types$Type.$less$colon$less(Types.scala:814)
+	at scala.reflect.internal.Types$Type.$less$colon$less(Types.scala:270)
+	at Test$.$anonfun$tasks$1(reflection-sync-subtypes.scala:7)
+	at Test$$anon$1.$anonfun$run$1(reflection-sync-subtypes.scala:15)
+	at Test$$anon$1.$anonfun$run$1$adapted(reflection-sync-subtypes.scala:15)
+	at Test$$anon$1.run(reflection-sync-subtypes.scala:15)

@retronym retronym marked this pull request as ready for review June 23, 2020 11:56
We need to favour keys from the left that equivalent (but ne),
while favouring values from the right.
@lrytz
Copy link
Member

lrytz commented Jun 23, 2020

I removed the change to reusable instances; that issue seems unrelated to this PR, and it was causing NPEs (which would probably be fixed with lazy vals).

@SethTisue
Copy link
Member

SethTisue commented Jun 23, 2020

TODO:

  • Add a generic test case to SetMapRulesTest that would have caught the bug before we found it in scala-collections-laws.

improving the tests can happen post-release; I've made a separate ticket on it at scala/scala-dev#713

@SethTisue SethTisue merged commit 98b9a1c into scala:2.13.x Jun 23, 2020
@SethTisue SethTisue removed the prio:blocker release blocker (used only by core team, only near release time) label Jun 23, 2020
@retronym
Copy link
Member Author

I removed the change to reusable instances; that issue seems unrelated to this PR, and it was causing NPEs (which would probably be fixed with lazy vals).

#9080

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidate library:collections PRs involving changes to the standard collection library
Projects
None yet
4 participants