-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Attempt to revert 14026 #15343
Conversation
Off by default
A quick look at the test failures:
Apparently that test is flaky in the number of errors it outputs, it was removed once before (#12720) but added back recently (#14873).
That's expected since this is a soundness bug that #14026 fixed.
Also expected, that test was added in #14026.
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)
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. |
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 |
So, here's the reversal. It should be easy to re-enable this by
|
There was a problem hiding this 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.
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. |
Backport #15343: Attempt to revert 14026
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