From a037261b741485519b69fe988dad04d41fe0d141 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 25 Dec 2019 18:46:54 +0100 Subject: [PATCH 1/5] Fix #5495: Disallow lazy enums Use the same scheme already used in Parser for avoiding type/term flags mixups also when desugaring enums. --- .../dotty/tools/dotc/ast/DesugarEnums.scala | 4 ++-- compiler/src/dotty/tools/dotc/ast/untpd.scala | 22 +++++++++++++++++++ .../src/dotty/tools/dotc/core/Flags.scala | 2 +- .../dotty/tools/dotc/parsing/Parsers.scala | 19 ++-------------- tests/neg/i5495.scala | 4 ++++ tests/neg/i6795-b.check | 6 ++--- 6 files changed, 33 insertions(+), 24 deletions(-) create mode 100644 tests/neg/i5495.scala diff --git a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala index 822dce008b45..50d1885e1561 100644 --- a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala +++ b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala @@ -70,8 +70,8 @@ object DesugarEnums { /** Add implied flags to an enum class or an enum case */ def addEnumFlags(cdef: TypeDef)(implicit ctx: Context): TypeDef = - if (cdef.mods.isEnumClass) cdef.withMods(cdef.mods.withFlags(cdef.mods.flags | Abstract | Sealed)) - else if (isEnumCase(cdef)) cdef.withMods(cdef.mods.withFlags(cdef.mods.flags | Final)) + if (cdef.mods.isEnumClass) cdef.withMods(cdef.mods.withAddedFlags(Abstract | Sealed, cdef.span)) + else if (isEnumCase(cdef)) cdef.withMods(cdef.mods.withAddedFlags(Final, cdef.span)) else cdef private def valuesDot(name: PreName)(implicit src: SourceFile) = diff --git a/compiler/src/dotty/tools/dotc/ast/untpd.scala b/compiler/src/dotty/tools/dotc/ast/untpd.scala index 9353a1aa4c29..90e646d37e17 100644 --- a/compiler/src/dotty/tools/dotc/ast/untpd.scala +++ b/compiler/src/dotty/tools/dotc/ast/untpd.scala @@ -6,9 +6,11 @@ import core._ import Types._, Contexts._, Constants._, Names._, Flags._ import Symbols._, StdNames._, Trees._ import util.{Property, SourceFile, NoSource} +import util.Spans.Span import language.higherKinds import annotation.constructorOnly import annotation.internal.sharable +import Decorators._ object untpd extends Trees.Instance[Untyped] with UntypedTreeInfo { @@ -233,6 +235,26 @@ object untpd extends Trees.Instance[Untyped] with UntypedTreeInfo { if (mods.exists(_ eq mod)) this else withMods(mods :+ mod) + private def compatible(flags1: FlagSet, flags2: FlagSet): Boolean = + flags1.isEmpty || flags2.isEmpty + || flags1.isTermFlags && flags2.isTermFlags + || flags1.isTypeFlags && flags2.isTypeFlags + + /** Add `flags` to thos modifier set, checking that there are no type/term conflicts. + * If there are conflicts, issue an error and return the modifiers consisting of + * the added flags only. The reason to do it this way is that the added flags usually + * describe the core of a construct whereas the existing set are the modifiers + * given in the source. + */ + def withAddedFlags(flags: FlagSet, span: Span)(given ctx: Context): Modifiers = + if this.flags.isAllOf(flags) then this + else if compatible(this.flags, flags) then this | flags + else + println(i"BAD") + val what = if flags.isTermFlags then "values" else "types" + ctx.error(em"${(flags & ModifierFlags).flagsString} $what cannot be ${this.flags.flagsString}", ctx.source.atSpan(span)) + Modifiers(flags) + /** Modifiers with given list of Mods. It is checked that * all modifiers are already accounted for in `flags` and `privateWithin`. */ diff --git a/compiler/src/dotty/tools/dotc/core/Flags.scala b/compiler/src/dotty/tools/dotc/core/Flags.scala index dc6cea67be51..b1ec95a0e7c3 100644 --- a/compiler/src/dotty/tools/dotc/core/Flags.scala +++ b/compiler/src/dotty/tools/dotc/core/Flags.scala @@ -339,7 +339,7 @@ object Flags { val (_, DefaultMethod @ _, _) = newFlags(38, "") /** Symbol is an enum class or enum case (if used with case) */ - val (Enum @ _, _, _) = newFlags(40, "") + val (Enum @ _, _, _) = newFlags(40, "enum") /** An export forwarder */ val (Exported @ _, _, _) = newFlags(41, "exported") diff --git a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala index 500df14fdb1f..425ad5a7060f 100644 --- a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala +++ b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala @@ -2631,23 +2631,8 @@ object Parsers { addMod(mods, mod) } - private def compatible(flags1: FlagSet, flags2: FlagSet): Boolean = ( - flags1.isEmpty - || flags2.isEmpty - || flags1.isTermFlags && flags2.isTermFlags - || flags1.isTypeFlags && flags2.isTypeFlags - ) - - def addFlag(mods: Modifiers, flag: FlagSet): Modifiers = { - def getPrintableTypeFromFlagSet = - Map(Trait -> "trait", Method -> "method", Mutable -> "variable").get(flag) - - if (compatible(mods.flags, flag)) mods | flag - else { - syntaxError(ModifiersNotAllowed(mods.flags, getPrintableTypeFromFlagSet)) - Modifiers(flag) - } - } + def addFlag(mods: Modifiers, flag: FlagSet): Modifiers = + mods.withAddedFlags(flag, Span(in.offset)) /** Always add the syntactic `mod`, but check and conditionally add semantic `mod.flags` */ diff --git a/tests/neg/i5495.scala b/tests/neg/i5495.scala new file mode 100644 index 000000000000..89d020669ece --- /dev/null +++ b/tests/neg/i5495.scala @@ -0,0 +1,4 @@ +lazy enum LazyList[+A] { // error + case :: (head: A, tail: LazyList[A]) + case Nil +} \ No newline at end of file diff --git a/tests/neg/i6795-b.check b/tests/neg/i6795-b.check index 69fa72fe840e..a38fd9efb2d8 100644 --- a/tests/neg/i6795-b.check +++ b/tests/neg/i6795-b.check @@ -1,6 +1,4 @@ --- [E083] Syntax Error: tests/neg/i6795-b.scala:1:11 ------------------------------------------------------------------- +-- Error: tests/neg/i6795-b.scala:1:11 --------------------------------------------------------------------------------- 1 |sealed def y: Int = 1 // error | ^ - | Modifier(s) `sealed` not allowed for method - -longer explanation available when compiling with `-explain` + | values cannot be sealed From 03965dbf1266e1873fff845ec13170a8332c30d6 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 25 Dec 2019 18:51:44 +0100 Subject: [PATCH 2/5] Drop redundant error message + tests --- .../reporting/diagnostic/ErrorMessageID.scala | 1 - .../dotc/reporting/diagnostic/messages.scala | 21 ------------------- .../dotc/reporting/ErrorMessagesTests.scala | 18 ---------------- tests/neg/i5495.scala | 2 +- 4 files changed, 1 insertion(+), 41 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.scala index 946a1f6884c6..eac53eff220a 100644 --- a/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.scala +++ b/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.scala @@ -91,7 +91,6 @@ enum ErrorMessageID extends java.lang.Enum[ErrorMessageID] { ExpectedTopLevelDefID, AnonymousFunctionMissingParamTypeID, SuperCallsNotAllowedInlineableID, - ModifiersNotAllowedID, WildcardOnTypeArgumentNotAllowedOnNewID, FunctionTypeNeedsNonEmptyParameterListID, WrongNumberOfParametersID, diff --git a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala index 9a64593c614f..ae1120419000 100644 --- a/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala @@ -1695,27 +1695,6 @@ object messages { val explanation: String = "Method inlining prohibits calling superclass methods, as it may lead to confusion about which super is being called." } - case class ModifiersNotAllowed(flags: FlagSet, printableType: Option[String])(implicit ctx: Context) - extends Message(ModifiersNotAllowedID) { - val kind: String = "Syntax" - val msg: String = em"Modifier(s) `${flags.flagsString}` not allowed for ${printableType.getOrElse("combination")}" - val explanation: String = { - val first = "sealed def y: Int = 1" - val second = "sealed lazy class z" - em"""You tried to use a modifier that is inapplicable for the type of item under modification - | - | Please see the official Scala Language Specification section on modifiers: - | https://www.scala-lang.org/files/archive/spec/2.11/05-classes-and-objects.html#modifiers - | - |Consider the following example: - |$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 - """ - } - } - case class WrongNumberOfParameters(expected: Int)(implicit ctx: Context) extends Message(WrongNumberOfParametersID) { val kind: String = "Syntax" diff --git a/compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala b/compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala index 21659929ec81..eb5c7f4930d6 100644 --- a/compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala +++ b/compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala @@ -1005,24 +1005,6 @@ class ErrorMessagesTests extends ErrorMessagesTest { assertEquals("method bar", symbol.show) } - @Test def modifiersNotAllowed = - 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.name)(code) - .expect { (ictx, messages) => - implicit val ctx: Context = ictx - assertMessageCount(1, messages) - val ModifiersNotAllowed(flags, sort) :: Nil = messages - assertEquals(modifierAssertion, flags.flagsString) - assertEquals(typeAssertion, sort) - } - } - @Test def wildcardOnTypeArgumentNotAllowedOnNew = checkMessagesAfter(RefChecks.name) { """ diff --git a/tests/neg/i5495.scala b/tests/neg/i5495.scala index 89d020669ece..5da23b98e10b 100644 --- a/tests/neg/i5495.scala +++ b/tests/neg/i5495.scala @@ -1,4 +1,4 @@ -lazy enum LazyList[+A] { // error +lazy enum LazyList[+A] { // error: sealed abstract types cannot be lazy enum case :: (head: A, tail: LazyList[A]) case Nil } \ No newline at end of file From 12a5d66ae23a4ebe95ce95a0812438ec496c7bc8 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 25 Dec 2019 18:53:17 +0100 Subject: [PATCH 3/5] Drop debug println --- compiler/src/dotty/tools/dotc/ast/untpd.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/ast/untpd.scala b/compiler/src/dotty/tools/dotc/ast/untpd.scala index 90e646d37e17..eab6cccc4ff4 100644 --- a/compiler/src/dotty/tools/dotc/ast/untpd.scala +++ b/compiler/src/dotty/tools/dotc/ast/untpd.scala @@ -250,7 +250,6 @@ object untpd extends Trees.Instance[Untyped] with UntypedTreeInfo { if this.flags.isAllOf(flags) then this else if compatible(this.flags, flags) then this | flags else - println(i"BAD") val what = if flags.isTermFlags then "values" else "types" ctx.error(em"${(flags & ModifierFlags).flagsString} $what cannot be ${this.flags.flagsString}", ctx.source.atSpan(span)) Modifiers(flags) From b8b4790987e1d7af9ced66980ce92edd5f1cfc4e Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 25 Dec 2019 19:27:56 +0100 Subject: [PATCH 4/5] Define dummy as unused error message id I am exasperated how over-engineered and badly designed the error message architecture is. Error messages used to be simple strings but are now a complete mess. Latest installment: Error message ids are an enum. So if a message is no longer needed, you can drop the enum value, right? Wrong! The error message tests rely on the actual ordinal number printed, so they fail if that number changes. --- .../dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.scala index eac53eff220a..3365459486a4 100644 --- a/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.scala +++ b/compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.scala @@ -91,6 +91,7 @@ enum ErrorMessageID extends java.lang.Enum[ErrorMessageID] { ExpectedTopLevelDefID, AnonymousFunctionMissingParamTypeID, SuperCallsNotAllowedInlineableID, + UNUSED1, // not used anymore, but left so that error numbers stay the same WildcardOnTypeArgumentNotAllowedOnNewID, FunctionTypeNeedsNonEmptyParameterListID, WrongNumberOfParametersID, From 56c1f5247f1c0c7ddebcd8dfc522ee3996063443 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 26 Dec 2019 10:15:07 +0100 Subject: [PATCH 5/5] Pre-check modifiers in Parsers Adapted from #5525: Check that only access modifiers are legal for enums (both definitions and cases). This is useful as a pre-check, to avoid confusing error messages later. --- .../dotty/tools/dotc/ast/DesugarEnums.scala | 4 +- .../dotty/tools/dotc/parsing/Parsers.scala | 11 +++++- tests/neg/i5525.scala | 37 +++++++++++++++++++ 3 files changed, 48 insertions(+), 4 deletions(-) create mode 100644 tests/neg/i5525.scala diff --git a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala index 50d1885e1561..112baf56c4c4 100644 --- a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala +++ b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala @@ -288,7 +288,7 @@ object DesugarEnums { val toStringDef = toStringMethLit(name.toString) val impl1 = cpy.Template(impl)(body = List(ordinalDef, toStringDef) ++ registerCall) .withAttachment(ExtendsSingletonMirror, ()) - val vdef = ValDef(name, TypeTree(), New(impl1)).withMods(mods | EnumValue) + val vdef = ValDef(name, TypeTree(), New(impl1)).withMods(mods.withAddedFlags(EnumValue, span)) flatTree(scaffolding ::: vdef :: Nil).withSpan(span) } } @@ -304,7 +304,7 @@ object DesugarEnums { else { val (tag, scaffolding) = nextOrdinal(CaseKind.Simple) val creator = Apply(Ident(nme.DOLLAR_NEW), List(Literal(Constant(tag)), Literal(Constant(name.toString)))) - val vdef = ValDef(name, enumClassRef, creator).withMods(mods | EnumValue) + val vdef = ValDef(name, enumClassRef, creator).withMods(mods.withAddedFlags(EnumValue, span)) flatTree(scaffolding ::: vdef :: Nil).withSpan(span) } } diff --git a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala index 425ad5a7060f..be0a43fec759 100644 --- a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala +++ b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala @@ -3330,22 +3330,29 @@ object Parsers { } } + private def checkAccessOnly(mods: Modifiers, where: String): Modifiers = + val mods1 = mods & (AccessFlags | Enum) + if mods1 ne mods then + syntaxError(s"Only access modifiers are allowed on enum $where") + mods1 + /** EnumDef ::= id ClassConstr InheritClauses [‘with’] EnumBody */ def enumDef(start: Offset, mods: Modifiers): TypeDef = atSpan(start, nameStart) { + val mods1 = checkAccessOnly(mods, "definitions") val modulName = ident() in.endMarkerScope(modulName) { val clsName = modulName.toTypeName val constr = classConstr() val templ = template(constr, isEnum = true) - finalizeDef(TypeDef(clsName, templ), mods, start) + finalizeDef(TypeDef(clsName, templ), mods1, start) } } /** EnumCase = `case' (id ClassConstr [`extends' ConstrApps] | ids) */ def enumCase(start: Offset, mods: Modifiers): DefTree = { - val mods1 = mods | EnumCase + val mods1 = checkAccessOnly(mods, "cases") | EnumCase accept(CASE) atSpan(start, nameStart) { diff --git a/tests/neg/i5525.scala b/tests/neg/i5525.scala new file mode 100644 index 000000000000..078078645d0c --- /dev/null +++ b/tests/neg/i5525.scala @@ -0,0 +1,37 @@ +abstract enum Foo1 {} // error: only access modifiers allowed +final enum Foo2 {} // error: only access modifiers allowed +sealed enum Foo3 {} // error: only access modifiers allowed +implicit enum Foo4 {} // error: only access modifiers allowed +lazy enum Foo5 {} // error: only access modifiers allowed +erased enum Foo6 {} // error: only access modifiers allowed +override enum Foo7 {} // error: only access modifiers allowed +inline enum Foo8 {} // error: only access modifiers allowed +opaque enum Foo9 {} // error: only access modifiers allowed + +enum Foo10 { + abstract case C1() // error: only access modifiers allowed + final case C2() // error: only access modifiers allowed + sealed case C3() // error: only access modifiers allowed + implicit case C4() // error: only access modifiers allowed + lazy case C5() // error: only access modifiers allowed + erased case C6() // error: only access modifiers allowed + override case C7() // error: only access modifiers allowed + private case C8() // ok + protected case C9() // ok +} + +enum Foo11 { + abstract case C1 // error: only access modifiers allowed + final case C2 // error: only access modifiers allowed + sealed case C3 // error: only access modifiers allowed + implicit case C4 // error: only access modifiers allowed + lazy case C5 // error: only access modifiers allowed + erased case C6 // error: only access modifiers allowed + override case C7 // error: only access modifiers allowed + private case C8 // ok + protected case C9 // ok +} + +enum Foo12 { // error: enums must contain at least one case + inline case C10() // error // error // error (inline treated as ident here) +} \ No newline at end of file