Skip to content

Fix #2426: Use Scala-2 syntax for annotations of class constructors #2432

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

Merged
merged 8 commits into from
May 23, 2017
Merged
Show file tree
Hide file tree
Changes from all 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
132 changes: 114 additions & 18 deletions compiler/src/dotty/tools/dotc/parsing/Parsers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,41 @@ object Parsers {
} finally inFunReturnType = saved
}

/** A placeholder for dummy arguments that should be re-parsed as parameters */
val ParamNotArg = EmptyTree

/** A flag indicating we are parsing in the annotations of a primary
* class constructor
*/
private var inClassConstrAnnots = false

private def fromWithinClassConstr[T](body: => T): T = {
val saved = inClassConstrAnnots
try {
inClassConstrAnnots = true
body
} finally {
inClassConstrAnnots = saved
if (lookaheadTokens.nonEmpty) {
in.insertTokens(lookaheadTokens.toList)
lookaheadTokens.clear()
}
}
}

/** Lookahead tokens for the case of annotations in class constructors.
* We store tokens in lookahead as long as they can form a valid prefix
* of a class parameter clause.
*/
private var lookaheadTokens = new ListBuffer[TokenData]

/** Copy current token to end of lookahead */
private def saveLookahead() = {
val lookahead = new TokenData{}
lookahead.copyFrom(in)
lookaheadTokens += lookahead
}

def migrationWarningOrError(msg: String, offset: Int = in.offset) =
if (in.isScala2Mode)
ctx.migrationWarning(msg, source atPos Position(offset))
Expand Down Expand Up @@ -1280,20 +1315,79 @@ object Parsers {
if (in.token == RPAREN) Nil else commaSeparated(exprInParens)

/** ParArgumentExprs ::= `(' [ExprsInParens] `)'
* | `(' [ExprsInParens `,'] PostfixExpr `:' `_' `*' ')' \
*/
def parArgumentExprs(): List[Tree] =
inParens(if (in.token == RPAREN) Nil else commaSeparated(argumentExpr))
* | `(' [ExprsInParens `,'] PostfixExpr `:' `_' `*' ')'
*
* Special treatment for arguments of primary class constructor
* annotations. All empty argument lists `(` `)` following the first
* get represented as `List(ParamNotArg)` instead of `Nil`, indicating that
* the token sequence should be interpreted as an empty parameter clause
* instead. `ParamNotArg` can also be produced when parsing the first
* argument (see `classConstrAnnotExpr`).
*
* The method affects `lookaheadTokens` as a side effect.
* If the argument list parses as `List(ParamNotArg)`, `lookaheadTokens`
* contains the tokens that need to be replayed to parse the parameter clause.
* Otherwise, `lookaheadTokens` is empty.
*/
def parArgumentExprs(first: Boolean = false): List[Tree] = {
if (inClassConstrAnnots) {
assert(lookaheadTokens.isEmpty)
saveLookahead()
accept(LPAREN)
val args =
if (in.token == RPAREN)
if (first) Nil // first () counts as annotation argument
else ParamNotArg :: Nil
else {
openParens.change(LPAREN, +1)
try commaSeparated(argumentExpr)
finally openParens.change(LPAREN, -1)
}
if (args == ParamNotArg :: Nil)
in.adjustSepRegions(RPAREN) // simulate `)` without requiring it
else {
lookaheadTokens.clear()
accept(RPAREN)
}
args
}
else
inParens(if (in.token == RPAREN) Nil else commaSeparated(argumentExpr))
}

/** ArgumentExprs ::= ParArgumentExprs
* | [nl] BlockExpr
*/
def argumentExprs(): List[Tree] =
if (in.token == LBRACE) blockExpr() :: Nil else parArgumentExprs()

val argumentExpr = () => exprInParens() match {
case a @ Assign(Ident(id), rhs) => cpy.NamedArg(a)(id, rhs)
case e => e
val argumentExpr = () => {
val arg =
if (inClassConstrAnnots && lookaheadTokens.nonEmpty) classConstrAnnotExpr()
else exprInParens()
arg match {
case arg @ Assign(Ident(id), rhs) => cpy.NamedArg(arg)(id, rhs)
case arg => arg
}
}

/** Handle first argument of an argument list to an annotation of
* a primary class constructor. If the current token either cannot
* start an expression or is an identifier and is followed by `:`,
Copy link
Member

Choose a reason for hiding this comment

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

But a : can legitimately appear in an argument list, e.g:

class ann(x: Any) extends scala.annotation.Annotation

object Test {
  val elem: Int = 1
  class Foo @ann(elem: Float) (x: String)
}

Banning multiple param lists in constructor annotations still seems better to me than having complex rules to guess whether something is or isn't an argument list :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how banning multi param solves that problem?

Copy link
Member

@smarter smarter May 15, 2017

Choose a reason for hiding this comment

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

Indeed, it seems that scalac always expects a primary constructor annotation to have exactly one parameter list (having zero parameter lists won't be parsed correctly), which is a more annoying restriction. I'm still not super enthusiastic about adding so much special-casing in the parser (this will need to be replicated in scala.meta for example, /cc @xeno-by), but I'm not opposed to it. The main annoyance is that if seemingly valid code is misinterpreted, you'll get a very confusing error message, but that could be left as a future improvement.

* stop parsing the rest of the expression and return `EmptyTree`,
* indicating that we should re-parse the expression as a parameter clause.
* Otherwise parse as normal.
*/
def classConstrAnnotExpr() = {
if (in.token == IDENTIFIER) {
saveLookahead()
postfixExpr() match {
case Ident(_) if in.token == COLON => ParamNotArg
case t => expr1Rest(t, Location.InParens)
}
}
else if (isExprIntro) exprInParens()
else ParamNotArg
}

/** ArgumentExprss ::= {ArgumentExprs}
Expand All @@ -1305,9 +1399,17 @@ object Parsers {
}

/** ParArgumentExprss ::= {ParArgumentExprs}
*
* Special treatment for arguments of primary class constructor
* annotations. If an argument list returns `List(ParamNotArg)`
* ignore it, and return prefix parsed before that list instead.
*/
def parArgumentExprss(fn: Tree): Tree =
if (in.token == LPAREN) parArgumentExprss(Apply(fn, parArgumentExprs()))
if (in.token == LPAREN) {
val args = parArgumentExprs(first = !fn.isInstanceOf[Trees.Apply[_]])
if (inClassConstrAnnots && args == ParamNotArg :: Nil) fn
else parArgumentExprss(Apply(fn, args))
}
else fn

/** BlockExpr ::= `{' (CaseClauses | Block) `}'
Expand Down Expand Up @@ -2094,21 +2196,15 @@ object Parsers {
*/
def classConstr(owner: Name, isCaseClass: Boolean = false): DefDef = atPos(in.lastOffset) {
val tparams = typeParamClauseOpt(ParamOwner.Class)
val cmods = constrModsOpt(owner)
val cmods = fromWithinClassConstr(constrModsOpt(owner))
val vparamss = paramClauses(owner, isCaseClass)
makeConstructor(tparams, vparamss).withMods(cmods)
}

/** ConstrMods ::= AccessModifier
* | Annotation {Annotation} (AccessModifier | `this')
/** ConstrMods ::= {Annotation} [AccessModifier]
*/
def constrModsOpt(owner: Name): Modifiers = {
val mods = modifiers(accessModifierTokens, annotsAsMods())
if (mods.hasAnnotations && !mods.hasFlags)
if (in.token == THIS) in.nextToken()
else syntaxError(AnnotatedPrimaryConstructorRequiresModifierOrThis(owner), mods.annotations.last.pos)
Copy link
Member

@smarter smarter May 15, 2017

Choose a reason for hiding this comment

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

This means we can drop this Message subclass (but we must be careful to leave a dummy id in its place in the ErrorMessageID enum to avoid changing the error number of every subsequent message).

mods
}
def constrModsOpt(owner: Name): Modifiers =
modifiers(accessModifierTokens, annotsAsMods())

/** ObjectDef ::= id TemplateOpt
*/
Expand Down
36 changes: 33 additions & 3 deletions compiler/src/dotty/tools/dotc/parsing/Scanners.scala
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ object Scanners {
*/
var errOffset: Offset = NoOffset


/** Generate an error at the given offset */
def error(msg: String, off: Offset = offset) = {
ctx.error(msg, source atPos Position(off))
Expand Down Expand Up @@ -217,11 +216,42 @@ object Scanners {

private class TokenData0 extends TokenData

/** we need one token lookahead and one token history
/** The scanner itself needs one token lookahead and one token history
*/
val next : TokenData = new TokenData0
private val prev : TokenData = new TokenData0

/** The parser can also add more lookahead tokens via `insertTokens`.
* Tokens beyond `next` are stored in `following`.
*/
private var following: List[TokenData] = Nil

/** Push a copy of token data `td` to `following` */
private def pushCopy(td: TokenData) = {
val copy = new TokenData0
copy.copyFrom(td)
following = copy :: following
}

/** If following is empty, invalidate token data `td` by setting
* `td.token` to `EMPTY`. Otherwise pop head of `following` into `td`.
*/
private def popCopy(td: TokenData) =
if (following.isEmpty) td.token = EMPTY
else {
td.copyFrom(following.head)
following = following.tail
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a missing closing brace here.


/** Insert tokens `tds` in front of current token */
def insertTokens(tds: List[TokenData]) = {
if (next.token != EMPTY) pushCopy(next)
pushCopy(this)
following = tds ++ following
popCopy(this)
if (following.nonEmpty) popCopy(next)
}

/** a stack of tokens which indicates whether line-ends can be statement separators
* also used for keeping track of nesting levels.
* We keep track of the closing symbol of a region. This can be
Expand Down Expand Up @@ -310,7 +340,7 @@ object Scanners {
if (token == ERROR) adjustSepRegions(STRINGLIT)
} else {
this copyFrom next
next.token = EMPTY
popCopy(next)
}

/** Insert NEWLINE or NEWLINES if
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public enum ErrorMessageID {
ExpectedTokenButFoundID,
MixedLeftAndRightAssociativeOpsID,
CantInstantiateAbstractClassOrTraitID,
AnnotatedPrimaryConstructorRequiresModifierOrThisID,
DUMMY_AVAILABLE_1,
OverloadedOrRecursiveMethodNeedsResultTypeID,
RecursiveValueNeedsResultTypeID,
CyclicReferenceInvolvingID,
Expand Down
19 changes: 2 additions & 17 deletions compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1135,21 +1135,6 @@ object messages {
|""".stripMargin
}

case class AnnotatedPrimaryConstructorRequiresModifierOrThis(cls: Name)(implicit ctx: Context)
extends Message(AnnotatedPrimaryConstructorRequiresModifierOrThisID) {
val kind = "Syntax"
val msg = hl"""${"private"}, ${"protected"}, or ${"this"} expected for annotated primary constructor"""
val explanation =
hl"""|When using annotations with a primary constructor of a class,
|the annotation must be followed by an access modifier
|(${"private"} or ${"protected"}) or ${"this"}.
|
|For example:
| ${"class Sample @deprecated this(param: Parameter) { ..."}
| ^^^^
|""".stripMargin
}

case class OverloadedOrRecursiveMethodNeedsResultType(tree: Names.TermName)(implicit ctx: Context)
extends Message(OverloadedOrRecursiveMethodNeedsResultTypeID) {
val kind = "Syntax"
Expand Down Expand Up @@ -1277,7 +1262,7 @@ object messages {
|$noParameters""".stripMargin

}

case class AmbiguousOverload(tree: tpd.Tree, alts: List[SingleDenotation], pt: Type)(
err: typer.ErrorReporting.Errors)(
implicit ctx: Context)
Expand All @@ -1296,7 +1281,7 @@ object messages {
|- adding a type ascription as in `${"instance.myMethod: String => Int"}`
|"""
}

case class ReassignmentToVal(name: Names.Name)(implicit ctx: Context)
extends Message(ReassignmentToValID) {
val kind = "Reference"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,21 +198,6 @@ class ErrorMessagesTests extends ErrorMessagesTest {
assertTrue("expected trait", isTrait)
}

@Test def constructorModifier =
checkMessagesAfter("frontend") {
"""
|class AnotherClass @deprecated ()
""".stripMargin
}
.expect { (ictx, messages) =>
implicit val ctx: Context = ictx
val defn = ictx.definitions

assertMessageCount(1, messages)
val AnnotatedPrimaryConstructorRequiresModifierOrThis(cls) :: Nil = messages
assertEquals("AnotherClass", cls.show)
}

@Test def overloadedMethodNeedsReturnType =
checkMessagesAfter("frontend") {
"""
Expand Down Expand Up @@ -399,7 +384,7 @@ class ErrorMessagesTests extends ErrorMessagesTest {
assertEquals("Scope.foo(1)", tree.show)
assertEquals("((a: Int)Unit)(Scope.foo)", methodPart.show)
}

@Test def ambiugousOverloadWithWildcard =
checkMessagesAfter("frontend") {
"""object Context {
Expand Down
3 changes: 1 addition & 2 deletions docs/docs/internals/syntax.md
Original file line number Diff line number Diff line change
Expand Up @@ -335,8 +335,7 @@ TmplDef ::= ([‘case’ | `enum'] ‘class’ | trait’) ClassDef
| `enum' EnumDef
ClassDef ::= id ClassConstr TemplateOpt ClassDef(mods, name, tparams, templ)
ClassConstr ::= [ClsTypeParamClause] [ConstrMods] ClsParamClauses with DefDef(_, <init>, Nil, vparamss, EmptyTree, EmptyTree) as first stat
ConstrMods ::= AccessModifier
| Annotation {Annotation} (AccessModifier | ‘this’)
ConstrMods ::= {Annotation} [AccessModifier]
ObjectDef ::= id TemplateOpt ModuleDef(mods, name, template) // no constructor
EnumDef ::= id ClassConstr [`extends' [ConstrApps]] EnumDef(mods, name, tparams, template)
[nl] ‘{’ EnumCaseStat {semi EnumCaseStat} ‘}’
Expand Down
24 changes: 24 additions & 0 deletions tests/pos/i2426.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
class Foo @deprecated("foo", "2.11.0") (x: Int)

class Bar @deprecated(x: Int)

class Baz1 @deprecated(implicit c: C)
class Baz2 @deprecated()(implicit c: C)
class Baz3 @deprecated()()(implicit c: C)

object Test {
implicit val c: C = obj
new Baz1
new Baz2
new Baz3()
}

class D(implicit x: C)

class C
object obj extends C

class ann(x: C)(y: C, s: String) extends scala.annotation.Annotation

class Bam @ann(obj)(obj, "h")(n: String)