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

Attempt to revert 14026 #15343

Merged
merged 4 commits into from
Jun 1, 2022
Merged

Attempt to revert 14026 #15343

merged 4 commits into from
Jun 1, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 31, 2022

This is an attempt to partially revert #14026. It has lots of failures. We need to find someone to complete the job or else do a total revert of #14026. #14026 is important to have, but I fear we cannot accept all the regressions it causes for code that should compile.

So I think it is better to back out now, and try to put it back in the 3.2 series.

Fixes #14494
Fixes #15178
Fixes #15184
Fixes #15216

@smarter
Copy link
Member

smarter commented May 31, 2022

A quick look at the test failures:

tests/neg/i12640.scala failed

Apparently that test is flaky in the number of errors it outputs, it was removed once before (#12720) but added back recently (#14873).

tests/neg/i8900.scala failed

That's expected since this is a soundness bug that #14026 fixed.

tests/run/i8861.scala failed

Also expected, that test was added in #14026.

tests/pos/i15174.scala failed

Pickling difference, so probably harmless? (edit: the difference is likely caused by a type avoidance bug, so it makes sense for it to be broken by this reversal)

tests/run-macros/i7519c failed

That one is weird:

-- Error: tests/run-macros/i7519c/Test_2.scala:3:17 --------------------------------------------------------------------
3 |    println(quote[Int])
  |            ^^^^^^^^^^
  |            Exception occurred while executing macro expansion.
  |            java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
  |            	at scala.collection.immutable.ArraySeq$ofRef.apply(ArraySeq.scala:331)
  |            	at dotty.tools.dotc.quoted.PickledQuotes$.$anonfun$1(PickledQuotes.scala:176)
  |            	at scala.collection.Iterator$$anon$9.next(Iterator.scala:577)
  |            	at scala.collection.mutable.Growable.addAll(Growable.scala:62)
  |            	at scala.collection.mutable.Growable.addAll$(Growable.scala:57)
  |            	at scala.collection.immutable.MapBuilderImpl.addAll(Map.scala:692)
  |            	at scala.collection.immutable.Map$.from(Map.scala:643)
  |            	at scala.collection.IterableOnceOps.toMap(IterableOnce.scala:1256)
  |            	at scala.collection.IterableOnceOps.toMap$(IterableOnce.scala:1255)
  |            	at scala.collection.AbstractIterator.toMap(Iterator.scala:1293)
  |            	at dotty.tools.dotc.quoted.PickledQuotes$.spliceTypes(PickledQuotes.scala:178)
  |            	at dotty.tools.dotc.quoted.PickledQuotes$.unpickleTerm(PickledQuotes.scala:86)
  |            	at scala.quoted.runtime.impl.QuotesImpl.unpickleExprV2(QuotesImpl.scala:3044)
  |            	at Macro_1$package$.quoteImpl(Macro_1.scala:12)
  |
  |---------------------------------------------------------------------------------------------------------------------
  |Inline stack trace
  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
  |This location contains code that was inlined from Macro_1.scala:8
8 |inline def quote[T]: String = ${ quoteImpl[T] }
  |                              ^^^^^^^^^^^^^^^^^
   ---------------------------------------------------------------------------------------------------------------------

This test hasn't been touched in a long time, but this might be related to the quote refactoring /cc @nicolasstucki.

odersky added 3 commits June 1, 2022 12:21
More precisely, put it under a Config flag which is off by default.

This reverts commit ae1b00d.
More precisely, put it under a Config flag which is off by default.

This reverts commit 629006b.
Only i7519c is unexpected; need to figure out why it fails here.
@odersky
Copy link
Contributor Author

odersky commented Jun 1, 2022

I also put the two reverted follow-on commits under flag Config.checkLevels and moved the failing tests to pending.

It would be good to clarify why i7519c fails without level checking. /cc @nicolasstucki

@odersky odersky requested a review from smarter June 1, 2022 12:27
@odersky
Copy link
Contributor Author

odersky commented Jun 1, 2022

So, here's the reversal. It should be easy to re-enable this by

  • setting Config.checkLevels = true
  • re-instantiating the tests moved to pending in 44c2a3b
  • Making sure the new tests still pass

@odersky odersky added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jun 1, 2022
@odersky odersky requested a review from nicolasstucki June 1, 2022 12:43
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for the mysterious macro test error.

@odersky
Copy link
Contributor Author

odersky commented Jun 1, 2022

I discussed with @nicolasstucki. Nothing definite comes to mind, and it does not seem urgent to address i7519c. So I think we can merge and that way we have addressed all regressions so far.

@odersky odersky merged commit 3ddb21c into scala:main Jun 1, 2022
@odersky odersky deleted the revert-14026 branch June 1, 2022 14:48
@smarter smarter mentioned this pull request Jun 1, 2022
@Kordyjan Kordyjan added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Jun 6, 2022
Kordyjan added a commit that referenced this pull request Jun 7, 2022
@smarter smarter added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Jul 14, 2022
@Kordyjan Kordyjan added this to the 3.2.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
4 participants