Community build: multiple post-2.13.16 regressions

[SethTisue]

well, this one is a bit complicated, with three different failures in play... (UPDATE: four! see comments)

Confounding failure (already resolved)

first of all, scala-commons wasn't compiling for a while because it contains a custom subclass of Future, and we temporarily added an abstract method to Future for a while before changing our minds.

so to use git bisect to see what's going on here, we have to eliminate the Future issue by patching scala-commons a bit so that it compiles regardless of whether the Scala version we're testing against has the Future change or not. I've pushed that version of scala-commons here: https://github.com/scalacommunitybuild/scala-commons/tree/community-build-2.13

but even with that out of the way, the build still fails, and it fails in two different ways depending on where we look in the scala/scala version history since 2.13.16

both regressions are easily observable outside of dbuild

First regression (back end)

when @sjrd's scala/scala#10993 landed, compilation begins failing in the back end, with:

[error] Error while emitting CodecTestData.scala
[error] key not found: value $outer
[error] one error found
[error] (commons-core / Test / compileIncremental) Compilation failed

Second regression (macro)

but then since @lrytz's scala/scala#10976 landed, we don't even get to commons-core/Test/compile (test sources), because it now fails sooner during commons-core/compile (main sources), with:

[error] java.lang.NullPointerException: Cannot invoke "scala.reflect.api.Symbols$SymbolApi.isSynthetic()" because the return value of "scala.reflect.api.Trees$TreeApi.symbol()" is null
[error] 	at com.avsystem.commons.macros.MacroCommons$Annot$$anonfun$findArg$5.applyOrElse(MacroCommons.scala:286)
[error] 	at com.avsystem.commons.macros.MacroCommons$Annot$$anonfun$findArg$5.applyOrElse(MacroCommons.scala:283)
[error] 	at scala.collection.IterableOnceOps.collectFirst(IterableOnce.scala:1256)
[error] 	at scala.collection.IterableOnceOps.collectFirst$(IterableOnce.scala:1248)
[error] 	at scala.collection.AbstractIterable.collectFirst(Iterable.scala:935)
[error] 	at com.avsystem.commons.macros.MacroCommons$Annot.findArg(MacroCommons.scala:283)
[error] 	at com.avsystem.commons.macros.meta.MacroSymbols$TagMatchingSymbol.$anonfun$requiredTags$6(MacroSymbols.scala:319)

I've shortened the stack trace; a full stack trace is at https://gist.github.com/SethTisue/5c9df05d473dbf477f48c5cd450c15d3

Summary

so it appears we have two different regressions here

a back end regression caused by 10993, and something about macros caused by 10976

since macros are in play, we can't rule out that a buggy macro is the true cause and our changes in scala/scala just uncovered that bugginess

@sjrd @lrytz I'm kicking this to you guys first, before attempting to isolate or minimize either problem. if you can't tell what's going on, you could ask for me help trying to minimize this/these

[sjrd]

Oh, I'm afraid I have zero time to allocate for investigations for at least a week. Too much to do on the teaching side. 😖

Feel free to revert wholesale my PR if it helps.

[lrytz]

Thanks for isolating these Seth. I'll take a look, will ping you @sjrd if anything.

[lrytz]

For the change to annotations (scala/scala#10976)

• The macros in the project identify default annotation arguments by identifying calls to default getters. @ann() used to expand to @ann(ann$init$default$1), now it's @ann(defaultExprAST).
  • The macro works with the reflect.api.Annotation API, so we should probably add something like the argIsDefault method there, we added it only to the compiler internal AnnotationInfo. But it's possible to work around it.
  • But the library shows that the change to annotation defaults is a breaking change. Maybe we should put it behind a flag?

After working around that, I ran into scala/bug#13087, which needs a fix on the compiler side.

And after that the backend crash related to scala/scala#10993.

• minimized: [REVERTED] Do not expand lambdas used in super constructor calls. scala#10993 (comment)

[SethTisue]

But the library shows that the change to annotation defaults is a breaking change. Maybe we should put it behind a flag?

Maybe we could discuss that on a call — all the implications, the pros and cons, aren't obvious to me.

Also wondering if the flag would be opt-in or opt-out.

[SethTisue]

now that I have full community build results, I see that that both regressions are turning up in other projects as well:

back end bug:

scalasql

macro bug:

specs2
singleton-ops

[SethTisue]

for now let's revert scala/scala#10993: scala/scala#10997

[lrytz]

scala/bug#13087, which needs a fix on the compiler side

Fix in scala/scala#10998.

for now let's revert scala/scala#10993: scala/scala#10997

I think that's good. Maybe it's not worth trying again, at least I won't. I added a reproducer to scala/scala#10993 (comment), but it's possible that it also has other failure modes.

But the library shows that the change to annotation defaults is a breaking change. Maybe we should put it behind a flag?

Maybe we could discuss that on a call — all the implications, the pros and cons, aren't obvious to me.

Also wondering if the flag would be opt-in or opt-out.

Here's the fixes needed in scala-commons: https://github.com/scalacommunitybuild/scala-commons/compare/community-build-2.13...scalacommunitybuild:scala-commons:cb/new-annot-defaults?expand=1

Summary of the compiler change:

scala> class ann(x: Int = 1) extends annotation.Annotation
scala> @ann class K
scala> :power

scala> val arg = typeOf[K].typeSymbol.annotations.head.args.head
val arg: $r.intp.global.Tree = 1

scala> arg.tpe
val res1: $r.intp.global.Type = Int(1) @scala.annotation.meta.defaultArg

The arg.tpe is tagged with the defaultArg annotation to tell that it's a default.

In 2.13.16, it used to be

scala> val arg = typeOf[K].typeSymbol.annotations.head.args.head
val arg: $r.intp.global.Tree = ann.<init>$default$1

Macros / compiler plugins that are detecting default arguments need to be adjusted.

We could have an opt-in flag. When compiling an annotation that uses defaults, the AST inserted by the compiler would depend on that flag (1 or ann.<init>$default$1). But then macros / compiler plugins would basically have to support both styles forever, which is in a way worse.