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
Changes from 1 commit
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
12 changes: 9 additions & 3 deletions compiler/src/dotty/tools/dotc/core/Annotations.scala
Original file line number Diff line number Diff line change
@@ -7,14 +7,16 @@ import StdNames._
import dotty.tools.dotc.ast.tpd
import scala.util.Try
import util.Spans.Span
import printing.{Showable, Printer}
import printing.Texts.Text

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)
@@ -44,15 +46,18 @@ object Annotations {
/** The tree evaluation has finished. */
def isEvaluated: Boolean = true

/** 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)
@@ -98,6 +103,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 {
7 changes: 5 additions & 2 deletions compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala
Original file line number Diff line number Diff line change
@@ -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)
@@ -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>"
3 changes: 3 additions & 0 deletions compiler/src/dotty/tools/dotc/printing/Printer.scala
Original file line number Diff line number Diff line change
@@ -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

32 changes: 20 additions & 12 deletions compiler/src/dotty/tools/dotc/printing/RefinedPrinter.scala
Original file line number Diff line number Diff line change
@@ -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._
@@ -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) =>
@@ -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)
@@ -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))

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