-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
…onstructors Use Scala-2 compatible syntax for annotations of primary class constructors. In fact, we can drop Scala 2's restriction that such annotations may only have one parameter list.
This change caused a lot of complexity. More than 100 loc to deal with this issue. |
else { | ||
td.copyFrom(following.head) | ||
following = following.tail | ||
} |
There was a problem hiding this comment.
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.
val mods = modifiers(accessModifierTokens, annotsAsMods()) | ||
if (mods.hasAnnotations && !mods.hasFlags) | ||
if (in.token == THIS) in.nextToken() | ||
else syntaxError(AnnotatedPrimaryConstructorRequiresModifierOrThis(owner), mods.annotations.last.pos) |
There was a problem hiding this comment.
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).
|
||
/** 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 `:`, |
There was a problem hiding this comment.
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 :).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This error can no longer be raised.
To maintain compatibility with Scala2 we a `()` argument list as belonging to a primary constructor annotation if it is the first argument list for that annotation.
I tweaked it now so that the first |
When doing the indentation-based syntax, I found a better hammer: We can simply copy the Scanner at the current offset to create a lookahead Scanner. With that, the logic for handling primary constructor arguments could be greatly simplified. Most of scala#2432 could be reverted.
When doing the indentation-based syntax, I found a better hammer: We can simply copy the Scanner at the current offset to create a lookahead Scanner. With that, the logic for handling primary constructor arguments could be greatly simplified. Most of scala#2432 could be reverted.
Use Scala-2 compatible syntax for annotations of primary class constructors.
In fact, we can drop Scala 2's restriction that such annotations may only
have one parameter list.