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

Combines modifiers not allowed with type, and combinations.This also simplifies logic. Adds more detail to explanation #2807

Merged
merged 7 commits into from
Jun 29, 2017
17 changes: 6 additions & 11 deletions compiler/src/dotty/tools/dotc/parsing/Parsers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1680,21 +1680,16 @@ object Parsers {
)

def addFlag(mods: Modifiers, flag: FlagSet): Modifiers = {
def incompatible(kind: String) = {
syntaxError(ModifiersNotAllowed(mods.flags, kind))
Modifiers(flag)
}
if (compatible(mods.flags, flag)) mods | flag
else flag match {
case Trait => incompatible("trait")
case Method => incompatible("method")
case Mutable => incompatible("variable")
case _ =>
syntaxError(s"illegal modifier combination: ${mods.flags} and $flag")
mods
else {
syntaxError(ModifiersNotAllowed(mods.flags, getPrintableTypeFromFlagSet(flag)))
Modifiers(flag)
}
}

private def getPrintableTypeFromFlagSet(flag: FlagSet) =
Map(Trait -> "trait", Method -> "method", Mutable -> "variable").get(flag)

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this method is only used by addFlag, I would nest it in the method like:

def addFlag(mods: Modifiers, flag: FlagSet): Modifiers = {
  def getPrintableTypeFromFlagSet(flag: FlagSet) =
    Map(Trait -> "trait", Method -> "method", Mutable -> "variable").get(flag)

  if (compatible(mods.flags, flag)) mods | flag
  else {
    syntaxError(ModifiersNotAllowed(mods.flags, getPrintableTypeFromFlagSet(flag)))
    Modifiers(flag)
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely

/** Always add the syntactic `mod`, but check and conditionally add semantic `mod.flags`
*/
def addMod(mods: Modifiers, mod: Mod): Modifiers =
Expand Down
25 changes: 21 additions & 4 deletions compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1648,18 +1648,35 @@ object messages {
val explanation = "Method inlining prohibits calling superclass methods, as it may lead to confusion about which super is being called."
}

case class ModifiersNotAllowed(flags: FlagSet, sort: String)(implicit ctx: Context)
case class ModifiersNotAllowed(flags: FlagSet, printableType: Option[String])(implicit ctx: Context)
extends Message(ModifiersNotAllowedID) {
val kind = "Syntax"
val msg = s"modifier(s) `$flags' not allowed for $sort"
val msg = s"modifier(s) `$flags' not allowed for ${printableType.getOrElse("combination")}"
val explanation = {
val code = "sealed def y: Int = 1"
val first = "sealed def y: Int = 1"
val second = "sealed lazy class z"
hl"""You tried to use a modifier that is inapplicable for the type of item under modification
|
| Here is a list of modifiers and applicable item types
|
|
| Modifier ::= LocalModifier
| AccessModifier
| `override' traits, methods
| LocalModifier ::= `abstract' classes, traits
| `final' classes, traits, methods
| `sealed' classes, traits
| `implicit' variables, methods, classes
| `lazy' values
| AccessModifier ::= (`private' | `protected') [AccessQualifier] methods, classes
| AccessQualifier ::= `[' (id | `this') `]'
|
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a copy of the syntax here might be nice, but it'd be better if we could link to the spec (we could open an issue for this instead).

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense rather than duplicating maintenance effort

|
|Consider the following example:
|$code
|$first
|In this instance, the modifier 'sealed' is not applicable to the item type 'def' (method)
|$second
|In this instance, the modifier combination is not supported
"""
}
}
Expand Down
16 changes: 13 additions & 3 deletions compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ package reporting

import core.Contexts.Context
import diagnostic.messages._
import dotty.tools.dotc.core.Flags
import dotty.tools.dotc.core.Flags.FlagSet
import dotty.tools.dotc.core.Types.WildcardType
import dotty.tools.dotc.parsing.Tokens
import org.junit.Assert._
Expand Down Expand Up @@ -841,14 +843,22 @@ class ErrorMessagesTests extends ErrorMessagesTest {
}

@Test def modifiersNotAllowed =
checkMessagesAfter("refchecks")("""lazy trait T""")
verifyModifiersNotAllowed("lazy trait T", "lazy", Some("trait"))

@Test def modifiersOtherThanTraitMethodVariable =
verifyModifiersNotAllowed("sealed lazy class x", "sealed")

private def verifyModifiersNotAllowed(code: String, modifierAssertion: String,
typeAssertion: Option[String] = None) = {
checkMessagesAfter("refchecks")(code)
.expect { (ictx, messages) =>
implicit val ctx: Context = ictx
assertMessageCount(1, messages)
val ModifiersNotAllowed(flags, sort) :: Nil = messages
assertEquals("lazy", flags.toString)
assertEquals("trait", sort)
assertEquals(modifierAssertion, flags.toString)
assertEquals(typeAssertion, sort)
}
}

@Test def wildcardOnTypeArgumentNotAllowedOnNew =
checkMessagesAfter("refchecks") {
Expand Down