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.

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

Remove anomalies and gaps in handling annotations #13348

Merged
merged 7 commits into from
Sep 22, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions compiler/src/dotty/tools/dotc/ast/TreeInfo.scala
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,11 @@ trait TreeInfo[T >: Untyped <: Type] { self: Trees.Instance[T] =>
case _ => 0
}

/** The (last) list of arguments of an application */
def arguments(tree: Tree): List[Tree] = unsplice(tree) match {
case Apply(_, args) => args
case TypeApply(fn, _) => arguments(fn)
case Block(_, expr) => arguments(expr)
/** All term arguments of an application in a single flattened list */
def allArguments(tree: Tree): List[Tree] = unsplice(tree) match {
case Apply(fn, args) => allArguments(fn) ::: args
case TypeApply(fn, _) => allArguments(fn)
case Block(_, expr) => allArguments(expr)
case _ => Nil
}

Expand Down
48 changes: 44 additions & 4 deletions compiler/src/dotty/tools/dotc/core/Annotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,17 @@ import StdNames._
import dotty.tools.dotc.ast.tpd
import scala.util.Try
import util.Spans.Span
import printing.{Showable, Printer}
import printing.Texts.Text
import annotation.internal.sharable

object Annotations {

def annotClass(tree: Tree)(using Context) =
if (tree.symbol.isConstructor) tree.symbol.owner
else tree.tpe.typeSymbol

abstract class Annotation {
abstract class Annotation extends Showable {
def tree(using Context): Tree

def symbol(using Context): Symbol = annotClass(tree)
Expand All @@ -26,7 +29,8 @@ object Annotations {
def derivedAnnotation(tree: Tree)(using Context): Annotation =
if (tree eq this.tree) this else Annotation(tree)

def arguments(using Context): List[Tree] = ast.tpd.arguments(tree)
/** All arguments to this annotation in a single flat list */
def arguments(using Context): List[Tree] = ast.tpd.allArguments(tree)

def argument(i: Int)(using Context): Option[Tree] = {
val args = arguments
Expand All @@ -44,15 +48,48 @@ object Annotations {
/** The tree evaluation has finished. */
def isEvaluated: Boolean = true

/** Normally, type map over all tree nodes of this annotation, but can
* be overridden. Returns EmptyAnnotation if type type map produces a range
* type, since ranges cannot be types of trees.
*/
def mapWith(tm: TypeMap)(using Context) =
val args = arguments
if args.isEmpty then this
else
val findDiff = new TreeAccumulator[Type]:
def apply(x: Type, tree: Tree)(using Context): Type =
if tm.isRange(x) then x
else
val tp1 = tm(tree.tpe)
foldOver(if tp1 =:= tree.tpe then x else tp1, tree)
val diff = findDiff(NoType, args)
if tm.isRange(diff) then EmptyAnnotation
else if diff.exists then derivedAnnotation(tm.mapOver(tree))
else this

/** Does this annotation refer to a parameter of `tl`? */
def refersToParamOf(tl: TermLambda)(using Context): Boolean =
val args = arguments
if args.isEmpty then false
else tree.existsSubTree {
case id: Ident => id.tpe match
case TermParamRef(tl1, _) => tl eq tl1
case _ => false
case _ => false
}

/** A string representation of the annotation. Overridden in BodyAnnotation.
*/
def toText(printer: Printer): Text = printer.annotText(this)

def ensureCompleted(using Context): Unit = tree

def sameAnnotation(that: Annotation)(using Context): Boolean =
symbol == that.symbol && tree.sameTree(that.tree)
}

case class ConcreteAnnotation(t: Tree) extends Annotation {
case class ConcreteAnnotation(t: Tree) extends Annotation:
def tree(using Context): Tree = t
}

abstract class LazyAnnotation extends Annotation {
protected var mySym: Symbol | (Context ?=> Symbol)
Expand Down Expand Up @@ -98,6 +135,7 @@ object Annotations {
if (tree eq this.tree) this else ConcreteBodyAnnotation(tree)
override def arguments(using Context): List[Tree] = Nil
override def ensureCompleted(using Context): Unit = ()
override def toText(printer: Printer): Text = "@Body"
}

class ConcreteBodyAnnotation(body: Tree) extends BodyAnnotation {
Expand Down Expand Up @@ -194,6 +232,8 @@ object Annotations {
apply(defn.SourceFileAnnot, Literal(Constant(path)))
}

@sharable val EmptyAnnotation = Annotation(EmptyTree)

def ThrowsAnnotation(cls: ClassSymbol)(using Context): Annotation = {
val tref = cls.typeRef
Annotation(defn.ThrowsAnnot.typeRef.appliedTo(tref), Ident(tref))
Expand Down
10 changes: 7 additions & 3 deletions compiler/src/dotty/tools/dotc/core/TypeOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,13 @@ object TypeOps:
// with Nulls (which have no base classes). Under -Yexplicit-nulls, we take
// corrective steps, so no widening is wanted.
simplify(l, theMap) | simplify(r, theMap)
case AnnotatedType(parent, annot)
if annot.symbol == defn.UncheckedVarianceAnnot && !ctx.mode.is(Mode.Type) && !theMap.isInstanceOf[SimplifyKeepUnchecked] =>
simplify(parent, theMap)
case tp @ AnnotatedType(parent, annot) =>
val parent1 = simplify(parent, theMap)
if annot.symbol == defn.UncheckedVarianceAnnot
&& !ctx.mode.is(Mode.Type)
&& !theMap.isInstanceOf[SimplifyKeepUnchecked]
then parent1
else tp.derivedAnnotatedType(parent1, annot)
case _: MatchType =>
val normed = tp.tryNormalize
if (normed.exists) normed else mapOver
Expand Down
19 changes: 11 additions & 8 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3604,6 +3604,9 @@ object Types {
case tp: AppliedType => tp.fold(status, compute(_, _, theAcc))
case tp: TypeVar if !tp.isInstantiated => combine(status, Provisional)
case tp: TermParamRef if tp.binder eq thisLambdaType => TrueDeps
case AnnotatedType(parent, ann) =>
if ann.refersToParamOf(thisLambdaType) then TrueDeps
else compute(status, parent, theAcc)
case _: ThisType | _: BoundType | NoPrefix => status
case _ =>
(if theAcc != null then theAcc else DepAcc()).foldOver(status, tp)
Expand Down Expand Up @@ -3656,8 +3659,10 @@ object Types {
if (isResultDependent) {
val dropDependencies = new ApproximatingTypeMap {
def apply(tp: Type) = tp match {
case tp @ TermParamRef(thisLambdaType, _) =>
case tp @ TermParamRef(`thisLambdaType`, _) =>
range(defn.NothingType, atVariance(1)(apply(tp.underlying)))
case AnnotatedType(parent, ann) if ann.refersToParamOf(thisLambdaType) =>
mapOver(parent)
case _ => mapOver(tp)
}
}
Expand Down Expand Up @@ -5380,6 +5385,8 @@ object Types {
variance = saved
derivedLambdaType(tp)(ptypes1, this(restpe))

def isRange(tp: Type): Boolean = tp.isInstanceOf[Range]

/** Map this function over given type */
def mapOver(tp: Type): Type = {
record(s"TypeMap mapOver ${getClass}")
Expand Down Expand Up @@ -5423,8 +5430,9 @@ object Types {

case tp @ AnnotatedType(underlying, annot) =>
val underlying1 = this(underlying)
if (underlying1 eq underlying) tp
else derivedAnnotatedType(tp, underlying1, mapOver(annot))
val annot1 = annot.mapWith(this)
if annot1 eq EmptyAnnotation then underlying1
else derivedAnnotatedType(tp, underlying1, annot1)

case _: ThisType
| _: BoundType
Expand Down Expand Up @@ -5496,9 +5504,6 @@ object Types {
else newScopeWith(elems1: _*)
}

def mapOver(annot: Annotation): Annotation =
annot.derivedAnnotation(mapOver(annot.tree))

def mapOver(tree: Tree): Tree = treeTypeMap(tree)

/** Can be overridden. By default, only the prefix is mapped. */
Expand Down Expand Up @@ -5545,8 +5550,6 @@ object Types {

protected def emptyRange = range(defn.NothingType, defn.AnyType)

protected def isRange(tp: Type): Boolean = tp.isInstanceOf[Range]

protected def lower(tp: Type): Type = tp match {
case tp: Range => tp.lo
case _ => tp
Expand Down
7 changes: 5 additions & 2 deletions compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,10 @@ class PlainPrinter(_ctx: Context) extends Printer {
case _ => literalText(String.valueOf(const.value))
}

def toText(annot: Annotation): Text = s"@${annot.symbol.name}" // for now
/** Usual target for `Annotation#toText`, overridden in RefinedPrinter */
def annotText(annot: Annotation): Text = s"@${annot.symbol.name}"

def toText(annot: Annotation): Text = annot.toText(this)

def toText(param: LambdaParam): Text =
varianceSign(param.paramVariance)
Expand Down Expand Up @@ -570,7 +573,7 @@ class PlainPrinter(_ctx: Context) extends Printer {
Text()

nodeName ~ "(" ~ elems ~ tpSuffix ~ ")" ~ (Str(tree.sourcePos.toString) provided printDebug)
}.close // todo: override in refined printer
}.close

def toText(pos: SourcePosition): Text =
if (!pos.exists) "<no position>"
Expand Down
3 changes: 3 additions & 0 deletions compiler/src/dotty/tools/dotc/printing/Printer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ abstract class Printer {
/** A description of sym's location */
def extendedLocationText(sym: Symbol): Text

/** Textual description of regular annotation in terms of its tree */
def annotText(annot: Annotation): Text

/** Textual representation of denotation */
def toText(denot: Denotation): Text

Expand Down
32 changes: 20 additions & 12 deletions compiler/src/dotty/tools/dotc/printing/RefinedPrinter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import typer.ProtoTypes._
import Trees._
import TypeApplications._
import Decorators._
import NameKinds.WildcardParamName
import NameKinds.{WildcardParamName, DefaultGetterName}
import util.Chars.isOperatorPart
import transform.TypeUtils._
import transform.SymUtils._
Expand Down Expand Up @@ -607,7 +607,7 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
case tree: Template =>
toTextTemplate(tree)
case Annotated(arg, annot) =>
toTextLocal(arg) ~~ annotText(annot)
toTextLocal(arg) ~~ toText(annot)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we are now printing annot as a tree, and not a tree interpreted as an annotation

case EmptyTree =>
"<empty>"
case TypedSplice(t) =>
Expand Down Expand Up @@ -964,14 +964,18 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
keywordStr("package ") ~ toTextPackageId(tree.pid) ~ bodyText
}

/** Textual representation of an instance creation expression without the leading `new` */
protected def constrText(tree: untpd.Tree): Text = toTextLocal(tree).stripPrefix(keywordStr("new ")) // DD

protected def annotText(tree: untpd.Tree): Text = "@" ~ constrText(tree) // DD

override def annotsText(sym: Symbol): Text =
Text(sym.annotations.map(ann =>
if ann.symbol == defn.BodyAnnot then Str(simpleNameString(ann.symbol))
else annotText(ann.tree)))
protected def annotText(sym: Symbol, tree: untpd.Tree): Text =
def recur(t: untpd.Tree): Text = t match
case Apply(fn, Nil) => recur(fn)
case Apply(fn, args) =>
val explicitArgs = args.filterNot(_.symbol.name.is(DefaultGetterName))
recur(fn) ~ "(" ~ toTextGlobal(explicitArgs, ", ") ~ ")"
case TypeApply(fn, args) => recur(fn) ~ "[" ~ toTextGlobal(args, ", ") ~ "]"
case _ => s"@${sym.orElse(tree.symbol).name}"
recur(tree)

protected def modText(mods: untpd.Modifiers, sym: Symbol, kw: String, isType: Boolean): Text = { // DD
val suppressKw = if (enclDefIsClass) mods.isAllOf(LocalParam) else mods.is(Param)
Expand All @@ -984,12 +988,16 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
if (rawFlags.is(Param)) flagMask = flagMask &~ Given &~ Erased
val flags = rawFlags & flagMask
var flagsText = toTextFlags(sym, flags)
val annotations =
if (sym.exists) sym.annotations.filterNot(ann => dropAnnotForModText(ann.symbol)).map(_.tree)
else mods.annotations.filterNot(tree => dropAnnotForModText(tree.symbol))
Text(annotations.map(annotText), " ") ~~ flagsText ~~ (Str(kw) provided !suppressKw)
val annotTexts =
if sym.exists then
sym.annotations.filterNot(ann => dropAnnotForModText(ann.symbol)).map(toText)
else
mods.annotations.filterNot(tree => dropAnnotForModText(tree.symbol)).map(annotText(NoSymbol, _))
Text(annotTexts, " ") ~~ flagsText ~~ (Str(kw) provided !suppressKw)
}

override def annotText(annot: Annotation): Text = annotText(annot.symbol, annot.tree)

def optText(name: Name)(encl: Text => Text): Text =
if (name.isEmpty) "" else encl(toText(name))

Expand Down
7 changes: 7 additions & 0 deletions tests/neg/annot-printing.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-- [E007] Type Mismatch Error: tests/neg/annot-printing.scala:5:46 -----------------------------------------------------
5 |def x: Int @nowarn @main @Foo @Bar("hello") = "abc" // error
| ^^^^^
| Found: ("abc" : String)
| Required: Int @nowarn() @main @Foo @Bar("hello")

longer explanation available when compiling with `-explain`
6 changes: 6 additions & 0 deletions tests/neg/annot-printing.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import scala.annotation.*
class Foo() extends Annotation
class Bar(s: String) extends Annotation

def x: Int @nowarn @main @Foo @Bar("hello") = "abc" // error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with these changes -Xprint:parser will only show <none> where the class name should be

def x: Int @<none> @<none> @<none> @<none>("hello") = "abc"

Copy link
Member

@bishabosha bishabosha Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in 3.1.0-RC1 we still show the correct class names with -Xprint:parser

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Is there an easy way to write a printing test to guard against regressions for this?

Copy link
Member

@bishabosha bishabosha Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the PrintingTest can be modified to add cases that print at parser instead:

compiler/test/dotty/tools/dotc/printing/PrintingTest.scala

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odersky I have added tests to PrintingTest in latest commit


7 changes: 7 additions & 0 deletions tests/pos/dependent-annot.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class C
class ann(x: Any*) extends annotation.Annotation

def f(y: C, z: C) =
def g(): C @ann(y, z) = ???
bishabosha marked this conversation as resolved.
Show resolved Hide resolved
val ac: ((x: C) => Array[String @ann(x)]) = ???
val dc = ac(g())