Fix interactions between macros and by-name implicits #7199

milessabin · 2018-09-11T11:14:49Z

While prototyping a version of shapeless which replaces Lazy with by-name implicits it emerged that implicit resolutions which involve both by-name implicits and implicit macros can produce dictionaries
which type check but which have broken owner chains.

This had been anticipated (see 5dd9909) but the ownership change performed there wasn't thoroughgoing enough.

In addition, an unnecessary check in Implicits.scala, excluding implicit expansion sites with empty trees when attempting to determine if an implicit resolution was recursive, meant that implicit macros, which
always have empty trees in this case, were never detected as recursive.

Finally, the implicit dictionary is now encoded as,

final class LazyDefns$n extends java.io.Serializable {
  final val rec$m ...
}
val lazyDefns$n = new LazyDefns$
lazyDefns$n.rec$m

rather than as an object, to eliminate the unecessary overhead of object lazy initialization and to allow serialzation in rare cases where the enclosing value is captured.

With this commit a Lazy-free shapeless build is possible.

adriaanm

Let's see if we can refine this a bit. Could you also split out the different fixes into separate commits?

adriaanm · 2018-09-27T13:48:00Z

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

      else {
        val typer = newTyper(this)

+        def changeNonLocalOwners(tree: Tree, newowner: Symbol): Unit = {


Could you move this closer to class ChangeOwnerTraverser, so that it's more discoverable for potential future usage?

Maybe, after some refactoring, this could be a subclass of ChangeOwnerTraverser?

I've moved this next to ChangeOwnerTraverser and lifted the LocalOwnerTraverser out. It doesn't really work as a subclass of the former.

adriaanm · 2018-09-27T13:50:50Z

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

-          }
+          val vname = newTermName(typer.fresh.newName("lazyDefns$"))
+          val vdef0 =
+            ValDef(Modifiers(FINAL | SYNTHETIC), vname, Ident(cname), New(Ident(cname), Nil))


Could include a signature for the valdef. It's simply cdef.symbol.tpe_*, right?

What do you mean by that? The ValDef has a tpt: Ident(cname).

Ahh ... OK, I think I get it :-)

adriaanm · 2018-09-27T14:00:03Z

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

+
+          val cname = newTypeName(typer.fresh.newName("LazyDefns$"))
+          val cdef0 =
+            ClassDef(Modifiers(FINAL | SYNTHETIC), cname, Nil,


If possible, it would be preferable to use an approach closer to the one in synthesizePartialFunction (

scala/src/compiler/scala/tools/nsc/typechecker/Typers.scala

Lines 2805 to 2814 in 036143b

anonClass setInfo ClassInfoType(parents(matchResTp), newScope, anonClass)

List(applyMeth, isDefinedAtMethod)

}

members foreach (m => anonClass.info.decls enter m.symbol)

val typedBlock = typedPos(tree.pos, mode, pt) {

Block(ClassDef(anonClass, NoMods, ListOfNil, members, tree.pos.focus), atPos(tree.pos.focus)(

Apply(Select(New(Ident(anonClass.name).setSymbol(anonClass)), nme.CONSTRUCTOR), List())

))

), and call the ClassDef factory method at

scala/src/compiler/scala/tools/nsc/ast/Trees.scala

Line 61 in 80f165a

def ClassDef(sym: Symbol, constrMods: Modifiers, vparamss: List[List[ValDef]], body: List[Tree], superPos: Position): ClassDef = {

Addressed review comments

milessabin · 2018-10-28T18:44:18Z

Also rebased.

milessabin · 2018-10-28T19:11:45Z

The failure on Travis appears to be another instance of scala/scala-dev#570.

SethTisue · 2018-11-06T16:38:13Z

@milessabin rebasing again should fix Travis

milessabin · 2018-11-07T13:14:47Z

Rebased.

milessabin · 2018-11-08T09:56:01Z

Travis failure this time is a gateway timeout ... I've restarted the job.

milessabin · 2018-11-08T09:56:25Z

@adriaanm this is ready to go now ...

SethTisue · 2018-11-08T12:24:02Z

new Travis failure is

[warn] 8 warnings found
[error] ## Exception when compiling 161 sources to /home/travis/build/scala/scala/build/quick/classes/reflect
[error] null
[error] scala.tools.nsc.typechecker.Typers$Typer.makeAccessible(Typers.scala:548)
[error] scala.tools.nsc.typechecker.Typers$Typer.$anonfun$typed1$60(Typers.scala:5074)

doesn't look familiar to me

lrytz · 2018-11-08T12:41:15Z

This is the same SOE as initially reported in scala/scala-dev#570, if you look a little bit further down:

[error] java.lang.StackOverflowError
[error] 	at scala.tools.nsc.typechecker.Typers$Typer.makeAccessible(Typers.scala:548)
[error] 	at scala.tools.nsc.typechecker.Typers$Typer.$anonfun$typed1$60(Typers.scala:5074)
[error] 	at scala.tools.nsc.typechecker.Typers$Typer.$anonfun$silent$2(Typers.scala:717)
[error] 	at scala.tools.nsc.typechecker.Typers$Typer.silent(Typers.scala:717)
[error] 	at scala.tools.nsc.typechecker.Typers$Typer.typedSelect$1(Typers.scala:5074)
[error] 	at scala.tools.nsc.typechecker.Typers$Typer.typedSelectOrSuperCall$1(Typers.scala:5131)
[error] 	at scala.tools.nsc.typechecker.Typers$Typer.typed1(Typers.scala:5676)
[error] 	at scala.tools.nsc.transform.Erasure$Eraser.typed1(Erasure.scala:797)

I guess someone catches the SOE.

milessabin · 2018-11-08T14:32:02Z

@lrytz am I right that the SOE isn't an issue for this PR, it's still scala/scala-dev#570?

lrytz · 2018-11-08T14:32:31Z

yes!

milessabin · 2018-11-09T14:20:26Z

Rebased following #7375.

milessabin · 2018-11-09T15:05:49Z

@SethTisue The failure I'm seeing here is a gateway timeout: https://travis-ci.org/scala/scala/jobs/452899122#L5233.

An unnecessary check in Implicits.scala excludes implicit expansion sites with empty trees when attempting to determine if an implicit resolution was recursive meant that implicit macros, which always have empty trees in this case, were never detected as recursive.

While prototyping a version of shapeless which replaces Lazy with by-name implicits it emerged that implicit resolutions which involve both by-name implicits and implicit macros can produce dictionaries which type checked but which had broken owner chains. This had been anticipated (see 5dd9909) but the ownership change performed there wasn't thoroughgoing enough. The implicit dictionary is now encoded as, final class LazyDefns$n extends java.io.Serializable { final val rec$m ... } val lazyDefns$n = new LazyDefns$ lazyDefns$n.rec$m rather than as an object, to eliminate the unecessary overhead of object lazy initialization and to allow serialzation in rare cases where the enclosing value is captured. With this commit a Lazy-free shapeless build is possible.

milessabin · 2018-11-21T11:58:13Z

Rebased.

adriaanm · 2018-11-21T14:30:49Z

Sorry about the delay.

milessabin · 2018-11-21T15:08:19Z

Thanks!

milessabin added this to the 2.13.0-RC1 milestone Sep 11, 2018

milessabin requested a review from adriaanm September 11, 2018 11:14

adriaanm previously requested changes Sep 27, 2018

View reviewed changes

milessabin force-pushed the topic/byname-implicits-and-macros branch from a9dd3c0 to 45ef876 Compare October 28, 2018 18:38

milessabin mentioned this pull request Oct 28, 2018

multiple PRs are green on Jenkins, red on Travis-CI scala/scala-dev#570

Closed

milessabin mentioned this pull request Nov 4, 2018

Byname implicits parameters with recursive dictionaries scala/scala-dev#419

Closed

5 tasks

milessabin force-pushed the topic/byname-implicits-and-macros branch from 45ef876 to 3c70a51 Compare November 7, 2018 13:14

milessabin force-pushed the topic/byname-implicits-and-macros branch from 3c70a51 to c803691 Compare November 9, 2018 14:19

milessabin added 2 commits November 21, 2018 11:32

milessabin force-pushed the topic/byname-implicits-and-macros branch from c803691 to 371d87c Compare November 21, 2018 11:57

adriaanm merged commit 046d96d into scala:2.13.x Nov 21, 2018

milessabin mentioned this pull request Jan 21, 2019

Mixing by-name implicits and inlining results in broken owner chains scala/scala3#5766

Closed

	anonClass setInfo ClassInfoType(parents(matchResTp), newScope, anonClass)
	List(applyMeth, isDefinedAtMethod)
	}

	members foreach (m => anonClass.info.decls enter m.symbol)

	val typedBlock = typedPos(tree.pos, mode, pt) {
	Block(ClassDef(anonClass, NoMods, ListOfNil, members, tree.pos.focus), atPos(tree.pos.focus)(
	Apply(Select(New(Ident(anonClass.name).setSymbol(anonClass)), nme.CONSTRUCTOR), List())
	))

Fix interactions between macros and by-name implicits #7199

Fix interactions between macros and by-name implicits #7199

Uh oh!

Conversation

milessabin commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm left a comment

Choose a reason for hiding this comment

Uh oh!

adriaanm Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

adriaanm Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

milessabin Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

adriaanm Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

milessabin Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

milessabin Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

adriaanm Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

milessabin commented Oct 28, 2018

Uh oh!

milessabin commented Oct 28, 2018

Uh oh!

SethTisue commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milessabin commented Nov 7, 2018

Uh oh!

milessabin commented Nov 8, 2018

Uh oh!

milessabin commented Nov 8, 2018

Uh oh!

SethTisue commented Nov 8, 2018

Uh oh!

lrytz commented Nov 8, 2018

Uh oh!

milessabin commented Nov 8, 2018

Uh oh!

lrytz commented Nov 8, 2018

Uh oh!

milessabin commented Nov 9, 2018

Uh oh!

milessabin commented Nov 9, 2018

Uh oh!

milessabin commented Nov 21, 2018

Uh oh!

adriaanm commented Nov 21, 2018

Uh oh!

milessabin commented Nov 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

milessabin commented Sep 11, 2018 •

edited

Loading

SethTisue commented Nov 6, 2018 •

edited

Loading