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.

Sign up for GitHub

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

Jump to bottom

uncheckedStable backport to 2.12.x caused regression in annotations #12216

Closed
retronym opened this issue Nov 3, 2020 · 4 comments
Closed

uncheckedStable backport to 2.12.x caused regression in annotations #12216

retronym opened this issue Nov 3, 2020 · 4 comments
Assignees
@retronym
Labels
blocker compiler crash regression
Milestone
2.12.13

Comments

@retronym
Copy link
Member

retronym commented Nov 3, 2020

reproduction steps

using Scala 2.12.13-bin-b08ccde

import scala.annotation.StaticAnnotation

class anno(attr: Boolean) extends StaticAnnotation

object Test {
  @anno(attr = true) def attr: Int = 1
}

problem

Exception in thread "main" java.lang.StackOverflowError
	at scala.tools.nsc.typechecker.Contexts$Context.innerDepth$1(Contexts.scala:487)
	at scala.tools.nsc.typechecker.Contexts$Context.make(Contexts.scala:496)
	at scala.tools.nsc.typechecker.Contexts$Context.makeNonSilent(Contexts.scala:550)
	at scala.tools.nsc.typechecker.Namers$Namer.$anonfun$annotSig$4(Namers.scala:1888)
	at scala.tools.nsc.typechecker.Namers$Namer.$anonfun$annotSig$3(Namers.scala:1887)
	at scala.reflect.internal.AnnotationInfos$LazyAnnotationInfo.forcedInfo$lzycompute(AnnotationInfos.scala:224)
	at scala.reflect.internal.AnnotationInfos$LazyAnnotationInfo.forcedInfo(AnnotationInfos.scala:224)
	at scala.reflect.internal.AnnotationInfos$LazyAnnotationInfo.atp(AnnotationInfos.scala:226)
	at scala.reflect.internal.AnnotationInfos$AnnotationInfo.symbol(AnnotationInfos.scala:298)
	at scala.reflect.internal.AnnotationInfos$AnnotationInfo.matches(AnnotationInfos.scala:319)
	at scala.reflect.internal.AnnotationInfos$Annotatable.dropOtherAnnotations(AnnotationInfos.scala:67)
	at scala.reflect.internal.AnnotationInfos$Annotatable.hasAnnotation(AnnotationInfos.scala:52)
	at scala.reflect.internal.AnnotationInfos$Annotatable.hasAnnotation$(AnnotationInfos.scala:50)
	at scala.reflect.internal.Symbols$Symbol.hasAnnotation(Symbols.scala:220)
	at scala.reflect.internal.TreeInfo.isUncheckedStable(TreeInfo.scala:125)
	at scala.reflect.internal.TreeInfo.isStableMemberOf(TreeInfo.scala:133)
	at scala.reflect.internal.TreeInfo.isStableIdentifier(TreeInfo.scala:114)
	at scala.reflect.internal.TreeInfo.isPath(TreeInfo.scala:102)
	at scala.reflect.internal.TreeInfo.admitsTypeSelection(TreeInfo.scala:158)
	at scala.tools.nsc.typechecker.Typers$Typer.isModuleTypedExpr$1(Typers.scala:615)
	at scala.tools.nsc.typechecker.Typers$Typer.stabilize(Typers.scala:644)
	at scala.tools.nsc.typechecker.Typers$Typer.typedIdent$2(Typers.scala:5286)
	at scala.tools.nsc.typechecker.Typers$Typer.typedIdentOrWildcard$1(Typers.scala:5298)
	at scala.tools.nsc.typechecker.Typers$Typer.typed1(Typers.scala:5732)
	at scala.tools.nsc.typechecker.Typers$Typer.typed(Typers.scala:5780)
	at scala.tools.nsc.typechecker.Typers$Typer.typedLhs$1(Typers.scala:5858)
	at scala.tools.nsc.typechecker.Typers$Typer.typedAssign$1(Typers.scala:4516)
	at scala.tools.nsc.typechecker.Typers$Typer.typedOutsidePatternMode$1(Typers.scala:5716)
	at scala.tools.nsc.typechecker.Typers$Typer.typed1(Typers.scala:5744)
	at scala.tools.nsc.typechecker.Typers$Typer.typed(Typers.scala:5780)
	at scala.tools.nsc.typechecker.NamesDefaults.$anonfun$isAmbiguousAssignment$2(NamesDefaults.scala:533)
	at scala.tools.nsc.typechecker.Typers$Typer.silent(Typers.scala:713)
	at scala.tools.nsc.typechecker.NamesDefaults.$anonfun$isAmbiguousAssignment$1(NamesDefaults.scala:532)
	at scala.runtime.java8.JFunction0$mcZ$sp.apply(JFunction0$mcZ$sp.java:23)
	at scala.tools.nsc.typechecker.Contexts$Context.$anonfun$savingUndeterminedTypeParams$1(Contexts.scala:337)
	at scala.tools.nsc.typechecker.Contexts$Context.savingUndeterminedTypeParams(Contexts.scala:401)
	at scala.tools.nsc.typechecker.NamesDefaults.isAmbiguousAssignment(NamesDefaults.scala:516)

The problem doesn't exist in 2.13.x (where @uncheckedStable support was authored. Maybe some other change on that branch avoids the problem?

I'm trying fixes in retronym/scala#104
@retronym retronym added this to the 2.12.13 milestone Nov 3, 2020
@retronym retronym self-assigned this Nov 3, 2020
@retronym
Copy link
Member Author

retronym commented Nov 3, 2020

The relevant difference between 2.12.x and 2.13.x appears to be scala/scala#6092, the change to unconditionally parse a(b = c) as a named parameter rather than first checking if it is valid as an assignment.

It is possible however that other code patterns could trigger this new cyclic error in 2.13.x.

@retronym
Copy link
Member Author

retronym commented Nov 4, 2020

Here is a program that incurs a cyclic error in 2.13.x since the scala/scala#8338.

import scala.annotation.StaticAnnotation

class anno(a: Any) extends StaticAnnotation

object Test {
  def foo = attr
  @anno(foo) def attr: Int = foo
}

|-- object Test BYVALmode-EXPRmode (site: package <empty>)
|    |-- super APPSELmode-EXPRmode-POLYmode-QUALmode (silent: <init> in Test)
|    |    |-- this EXPRmode (silent: <init> in Test)
|    |    |    \-> Test.type
|    |    \-> Test.type
|    |-- def foo BYVALmode-EXPRmode (site: object Test)
|    |    |-- attr EXPRmode (site: method foo in Test)
|    |    |    |-- Int TYPEmode (site: method attr in Test)
|    |    |    |    \-> Int
|    |    |    |-- new anno APPSELmode-BYVALmode-EXPRmode-FUNmode-POLYmode (site: object Test)
|    |    |    |    |-- new anno APPSELmode-EXPRmode-POLYmode-QUALmode (site: object Test)
|    |    |    |    |    |-- anno FUNmode-TYPEmode (site: object Test)
|    |    |    |    |    |    \-> anno
|    |    |    |    |    \-> anno
|    |    |    |    \-> (a: Any): anno
|    |    |    |-- (a: Any): anno : pt=anno EXPRmode (site: value <local Test> in Test)
|    |    |    |    |-- foo : pt=Any BYVALmode-EXPRmode (site: value <local Test> in Test)
|    |    |    |    |    caught scala.reflect.internal.Symbols$CyclicReference: illegal cyclic reference involving method foo: while typing foo
/Users/jz/code/scala/sandbox/test.scala:7: error: recursive method foo needs result type
  @anno(foo) def attr: Int = foo
        ^
|    |    |    |    \-> anno

@retronym
Copy link
Member Author

retronym commented Nov 4, 2020

Here's another take at avoiding these cycles. We could introduce an extra layer of lazy typechecking for annotations. In the first phase, we typecheck the fun of the Apply. In the second phase, we typecheck the Apply itself. AnnotationInfo.matches (called by isUncheckedStable etc), would only require forcing the first phase.

retronym/scala#105

@retronym
Copy link
Member Author

retronym commented Nov 4, 2020

An alternative (or complementary) remedy would be to change the order of evaluation of:

      def isModuleTypedExpr = (
           treeInfo.admitsTypeSelection(tree)
        && (isStableContext(tree, mode, pt) || sym.isModuleNotMethod)
      )

The call to admitsTypeSelection ends up triggering the cyclic error forcing annotations of the referenced tree, even when the second part of the && expression would have evaluated false.

retronym added a commit to retronym/scala that referenced this issue Nov 5, 2020
@retronym
Make annotation typechecking lazier
77a7eba 
Now that `Typer.stabilize` ends up checking for `@uncheckedStable`
annotations to support defs's as stable paths, new opportunities
arise for cyclic errors.

The enclosed test case started to fail since scala#8338 with:

```
|-- object Test BYVALmode-EXPRmode (site: package <empty>)
|    |-- super APPSELmode-EXPRmode-POLYmode-QUALmode (silent: <init> in Test)
|    |    |-- this EXPRmode (silent: <init> in Test)
|    |    |    \-> Test.type
|    |    \-> Test.type
|    |-- def foo BYVALmode-EXPRmode (site: object Test)
|    |    |-- attr EXPRmode (site: method foo in Test)
|    |    |    |-- Int TYPEmode (site: method attr in Test)
|    |    |    |    \-> Int
|    |    |    |-- new anno APPSELmode-BYVALmode-EXPRmode-FUNmode-POLYmode (site: object Test)
|    |    |    |    |-- new anno APPSELmode-EXPRmode-POLYmode-QUALmode (site: object Test)
|    |    |    |    |    |-- anno FUNmode-TYPEmode (site: object Test)
|    |    |    |    |    |    \-> anno
|    |    |    |    |    \-> anno
|    |    |    |    \-> (a: Any): anno
|    |    |    |-- (a: Any): anno : pt=anno EXPRmode (site: value <local Test> in Test)
|    |    |    |    |-- foo : pt=Any BYVALmode-EXPRmode (site: value <local Test> in Test)
|    |    |    |    |    caught scala.reflect.internal.Symbols$CyclicReference: illegal cyclic reference involving method foo: while typing foo
/Users/jz/code/scala/sandbox/test.scala:7: error: recursive method foo needs result type
  @anno(foo) def attr: Int = foo
        ^
|    |    |    |    \-> anno
```

Another variant detailed in scala/bug#12216 fails only on 2.12.x
(after the backport of scala#8338). Backporting this fix will fix
that problem, too.

This reduces the likelihood of cycles by typechecking the only core of
an annotation (_not_ the full application to the annotation argumnets)
to determine its type symbol.
This was referenced Nov 5, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@retronym retronym

Labels
blocker compiler crash regression
Projects
None yet
Milestone
2.12.13
Development

No branches or pull requests
2 participants
@retronym @dwijnand