Crash in constructors phase involving macros

[scabug]

When compiling tests subproject of spire we get a compiler crash in constructors phase:

[info] [info] Compiling 43 Scala sources to /Users/grek/scala/community-builds/target-0.8.0/project-builds/spire-a623899e2c648c1777951bb52e29d9f5981bee20/tests/target/scala-2.11.0-dbuildx1ef8223e86d2dca416641a53dcfc648d1a7933e9/test-classes...
[info] [error] macro$18$1 not in List(value $outer, value macro$18$1)
[info] scala.reflect.internal.FatalError: macro$18$1 not in List(value $outer, value macro$18$1)
[info] 	at scala.reflect.internal.SymbolTable.abort(SymbolTable.scala:57)
[info] 	at scala.tools.nsc.Global.abort(Global.scala:268)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer.parameterNamed(Constructors.scala:504)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer.parameter(Constructors.scala:496)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer$$anonfun$15.apply(Constructors.scala:690)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer$$anonfun$15.apply(Constructors.scala:681)
[info] 	at scala.collection.immutable.List.map(List.scala:274)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer.<init>(Constructors.scala:681)
[info] 	at scala.tools.nsc.transform.Constructors$ConstructorTransformer.transform(Constructors.scala:83)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer$$anonfun$13.apply(Constructors.scala:672)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer$$anonfun$13.apply(Constructors.scala:638)
[info] 	at scala.collection.immutable.List.foreach(List.scala:383)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer.<init>(Constructors.scala:638)
[info] 	at scala.tools.nsc.transform.Constructors$ConstructorTransformer.transform(Constructors.scala:83)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer$$anonfun$13.apply(Constructors.scala:672)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer$$anonfun$13.apply(Constructors.scala:638)
[info] 	at scala.collection.immutable.List.foreach(List.scala:383)
[info] 	at scala.tools.nsc.transform.Constructors$TemplateTransformer.<init>(Constructors.scala:638)
[info] 	at scala.tools.nsc.transform.Constructors$ConstructorTransformer.transform(Constructors.scala:83)
[info] 	at scala.tools.nsc.transform.Constructors$ConstructorTransformer.transform(Constructors.scala:29)
[info] 	at scala.reflect.api.Trees$Transformer$$anonfun$transformStats$1.apply(Trees.scala:2589)
[info] 	at scala.reflect.api.Trees$Transformer$$anonfun$transformStats$1.apply(Trees.scala:2587)
[info] 	at scala.collection.immutable.List.loop$1(List.scala:172)
[info] 	at scala.collection.immutable.List.mapConserve(List.scala:188)
[info] 	at scala.reflect.api.Trees$Transformer.transformStats(Trees.scala:2587)
[info] 	at scala.reflect.internal.Trees$$anonfun$itransform$7.apply(Trees.scala:1419)
[info] 	at scala.reflect.internal.Trees$$anonfun$itransform$7.apply(Trees.scala:1419)
[info] 	at scala.reflect.api.Trees$Transformer.atOwner(Trees.scala:2600)
[info] 	at scala.reflect.internal.Trees$class.itransform(Trees.scala:1418)
[info] 	at scala.reflect.internal.SymbolTable.itransform(SymbolTable.scala:16)
[info] 	at scala.reflect.internal.SymbolTable.itransform(SymbolTable.scala:16)
[info] 	at scala.reflect.api.Trees$Transformer.transform(Trees.scala:2555)
[info] 	at scala.tools.nsc.transform.Constructors$ConstructorTransformer.transform(Constructors.scala:87)
[info] 	at scala.tools.nsc.transform.Constructors$ConstructorTransformer.transform(Constructors.scala:29)
[info] 	at scala.tools.nsc.ast.Trees$Transformer.transformUnit(Trees.scala:147)

At the moment reproduction is a bit hard because it requires a dbuild configuration.

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-8425?orig=1
Reporter: @gkossakowski
Affected Versions: 2.11.0-RC1

[scabug]

@gkossakowski said:
CC [~xeno.by]

[scabug]

@xeno-by said:
Uh-oh

[scabug]

@gkossakowski said:
Here's a PR with dbuild configuration for spire: scala/community-build#24

[scabug]

@retronym said:
Does spire build against 2.11.0-RC1? Might be easier to debug this issue than via dbuild.

[scabug]

@retronym said:
Yep: https://github.com/retronym/spire/tree/ticket/8425

[info] Compiling 43 Scala sources to /Users/jason/code/spire/tests/target/scala-2.11.0-RC1/test-classes...
[error] macro$18$1 not in List(value $outer, value macro$18$1)
[trace] Stack trace suppressed: run last tests/test:compile for the full output.
[error] (tests/test:compile) scala.reflect.internal.FatalError: macro$18$1 not in List(value $outer, value macro$18$1)
[error] Total time: 196 s, completed Mar 17, 2014 11:42:58 PM
>

[scabug]

@retronym said (edited on Mar 17, 2014 10:57:25 PM UTC):
At the crash:

(name, constrParams.last, constrParams.last.name, constrParams.last.rawname)
= (macro$18$1,value macro$18$1,index$$macro$18$1,index$$macro$18$1)

unit = CforTest.scala

[scabug]

@retronym said (edited on Mar 17, 2014 11:24:20 PM UTC):
This is related to fresh names. See scala/scala@47d1fb1#diff-892d58c3587ea926d0bce6d7d53f16c6R34

% ack 'fresh' --scala
macros/src/main/scala/spire/macros/Syntax.scala
10:  def name(s: String) = newTermName(c.fresh(s + "$"))
ticket/8425 ~/code/spire ack '"index' --scala
core/src/main/scala/spire/util/Pack.scala
175:          c.abort(c.enclosingPosition, "index outside of 0-3")
197:          c.abort(c.enclosingPosition, "index outside of 0-7")

macros/src/main/scala/spire/macros/Syntax.scala
85:    val index = util.name("index")
136:      util.names("range", "index", "end", "limit", "step")
ticket/8425 ~/code/spire ack 'fresh' --scala
macros/src/main/scala/spire/macros/Syntax.scala
10:  def name(s: String) = newTermName(c.fresh(s + "$"))

Now we helpfully add a second '$' in addition to the one added by Spire. The '$$' looks like an expanded name (ie, one that has the FQCN prepended to make it unique when we are forced to make something Scala-private JVM public).

The Constructors phase uses unexpandedName which goes from index$$macro$18$1 to macro$18$1:

    // The constructor parameter corresponding to an accessor
    def parameter(acc: Symbol): Symbol = parameterNamed(acc.unexpandedName.getterName)

That doesn't exist in the list of constructor parameters.

The error message is confusing because the same un-expansion is happening somewhere in Symbol#toString.

I suggest that fresh-name generation should not add a '$' if the user provided prefix already ends in '$'.

[scabug]

@retronym said:
Marking for RC3, /cc-ing the arbiter of such matters, @adriaanm

[scabug]

@xeno-by said (edited on Mar 17, 2014 11:16:23 PM UTC):
Can I has RC3?! edit. Also, of course, that should be my first sentence here, thanks a lot Jason for tracking down the bug!

[scabug]

@xeno-by said:
Just a bit more context for my excitement. There's a number of important quasiquote bugs that we could prevent from being released into the wild (most notably, #8387) by having RC3, so it would be very appreciated if we could have time for that!

[scabug]

@retronym said (edited on Mar 17, 2014 11:21:37 PM UTC):
Actually, RC2 isn't on Sonatype just yet: https://oss.sonatype.org/index.html#nexus-search;gav~~scala-library~2.11.0-RC2~~

Nor being built: https://scala-webapps.epfl.ch/jenkins/view/scala-release-2.11.x/job/scala-release-2.11.x/

Could you submit a PR and email Adriaan to give him the option to include a fix. I'd be happy enough with a unit test to show that fresh("foo$") doesn't contain $$; we don't have to extract a functional test from spire.

[scabug]

@xeno-by said (edited on Mar 17, 2014 11:29:02 PM UTC):
Sure. If we decide to delay the release, can we wait until tomorrow, so that Denys and I can roll out a fix to https://issues.scala-lang.org/browse/SI-8387? (the fix is trivial, but I'd like to consult with Denys to make sure it'll work okay).

upd. I'd like to emphasize that #8387 is very important for macros, as it creates a major flaw in a frequently used quasiquote pattern, which we won't be able to easily fix later on, because people will naturally grow dependent on its current behavior.

[scabug]

@gkossakowski said:
Thanks Jason for tacking it down.

Is there a work-around present for this issue that users can employ? What about striping additional dollar in case it got returned by {{freshName}}?

Let's step back from the excitement and look at the landscape: it's a regression that has been detected in tests of just one project using an experimental feature. If there's a work-around then I'd like to demote this to Major and I don't think we should block the release.

[scabug]

@xeno-by said:
scala/scala#3638

[scabug]

@adriaanm said:
Just saw this ticket. Was updating all the modules to check binary compat etc, which I'd been procrastinating.
I'll look at the PR now.

[scabug]

@retronym said:
Yes, the "obscure action at a distance" manifestation gives me the heebie jeebies.

[scabug]

@retronym said:
Right, I forgot to mention the shapeless_2.11 mispublishing. Well spotted. I think we'll be BC for shapeless in any case, but I wonder what went wrong.

[scabug]

@gkossakowski said:
Ok. Mr. Zaugg, you won the dispute over whether to include the fix or not by throwing a reference to "heebie jeebies"! I've never heard of it befote and I love it! :-)

[scabug]

@adriaanm said:
To be clear, I'm okay including this fix because RC2 had not been cut yet and it's very low risk with apparently high visibility.
We wouldn't do an RC3 for this. Keep in mind that 2.11.1-RC1 is only 2 months away.

[scabug]

@retronym said:
Btw, huzzah for dbuild and the diligent dbuilder.

[scabug]

@gkossakowski said:
Post portem: I did build spire a week ago against 2.11.0-RC1. How come I didn't this issue then? Well, at the time I was interested in getting the minimal build working so I build just core subproject (which amounts to about 70% of LoC of spire). Only once things stabilized a bit I decided to give the entire spire a try and found this problem.

[scabug]

@adriaanm said:
+1 on the huzzahs!

[scabug]

@paulp said (edited on Mar 18, 2014 2:35:06 AM UTC):
See also #2806, especially the hysterical monument to futility enshrined in the apparent object of its attentions, lonesome '$$'. In other news, it turns out that post-trac-to-jira-conversion, one is not the loneliest number. Two is the loneliest number. Six Dog Night regrets the error.