Fix #4058: reject sealed and lazy for class parameters in parser #4070

Blaisorblade · 2018-03-04T22:54:03Z

Fix #4058.
Thanks to @allanrenucci for guidance. Rejecting lazy there follows Scalac, while sealed must be rejected there because of our consistency checks.

Intentionally mixing in small fixes/tests nearby.

Wondering about the strategy; I guess using a general modifiers parser and then rejecting most modifiers does make sense to offer better error messages, compared to only accepting the actually sensible modifiers.

Blaisorblade · 2018-03-04T23:45:26Z

tests/neg/i4058.scala

@@ -4,6 +4,6 @@ class C(abstract val a: Int) // error
 class D {
  def f(sealed a: Int) = 0 // error
  def g(lazy a: Int) = 0 // error
-  def g(ov erride a: Int) = 0 //error
+  def g(override a: Int) = 0 // error


Hadn't found testCompilation i4058 yet, my bad for not reading http://dotty.epfl.ch/docs/contributing/testing.html 😅

allanrenucci · 2018-03-05T07:11:40Z

Wondering about the strategy; I guess using a general modifiers parser and then rejecting most modifiers does make sense to offer better error messages, compared to only accepting the actually sensible modifiers

The modifier parser can take a set of allowed modifier as argument. But I believe in the case of lazy it is worth keeping the custom error message. I would not special case sealed and simply exclude it from the set of the allowed modifiers for parameters.

Blaisorblade · 2018-03-07T04:41:44Z

I would not special case sealed and simply exclude it from the set of the allowed modifiers for parameters.

Did that, then improved reporting: when modifiers runs into modifiers not allowed it says so, instead of "expected ... found 'sealed'". Code got trickier, so I added docs.

allanrenucci · 2018-03-07T10:05:17Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -1839,7 +1849,10 @@ object Parsers {
        val start = in.offset
        var mods = annotsAsMods()
        if (owner.isTypeName) {
-          mods = modifiers(start = mods) | ParamAccessor
+          // Adding ParamAccessor would crash
+          mods = modifiers(start = mods, allowed = modifierTokens &~ BitSet(SEALED)) | ParamAccessor


Why not modifierTokens - SEALED?

allanrenucci · 2018-03-07T10:06:42Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -2451,7 +2464,7 @@ object Parsers {

    /** BlockStatSeq ::= { BlockStat semi } [ResultExpr]
     *  BlockStat    ::= Import
-     *                 | Annotations [implicit] [lazy] Def
+     *                 | Annotations [implicit] [ghost] [lazy] Def


s/ghost/erased

allanrenucci · 2018-03-07T10:10:25Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -1714,7 +1722,7 @@ object Parsers {
     *                  |  override
     *  LocalModifier  ::= abstract | final | sealed | implicit | lazy
     */
-    def modifiers(allowed: BitSet = modifierTokens, start: Modifiers = Modifiers()): Modifiers = {
+    def modifiers(allowed: BitSet = modifierTokens, start: Modifiers = Modifiers(), parsedSeparately: BitSet = BitSet.empty): Modifiers = {


I am not convinced by this change of API. If the caller wants to parse some modifier separately, then he can exclude them from the allowed modifiers.

To get the nicer error I need to distinguish "not allowed" (which gives an error) from "parsed separately" (which doesn't). I had to use parsedSeparately only once.

If the API is too hard for the error, I can revert/separate these refactorings.

I think a better API would something along the line of:

/** ... * @param errorOn emit a syntax error when a parsed modifier is in errorOn */ def modifiers(allowed: BitSet, start: Modifiers, errorOn: BitSet) = { ... if (allowed.contains(in.token) { ...; loop() } else if (errorOn.contains(in.token) { syntaxError; loop() } else ... }

You can keep parsing modifiers after encountering an invalid modifier.

But since it is used only once, I think we should no complicate this API.

I noticed `sealed` while reviewing `modifiers`.

This might be overkill, since these modifiers aren't expected here.

Missing: tests. Had to add to `modifiers` a new `parsedSeparately` parameter, and since its interface is tricky I wrote some documentation.

Blaisorblade · 2018-03-07T13:30:52Z

Rebased and addressed most comments.

allanrenucci · 2018-03-07T14:41:05Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -2468,7 +2481,7 @@ object Parsers {
        else if (isDefIntro(localModifierTokens))
          if (in.token == IMPLICIT || in.token == ERASED) {
            val start = in.offset
-            var imods = modifiers(funArgMods)
+            var imods = modifiers(funArgMods, parsedSeparately = BitSet(LAZY))


I don't think we need this, the current error message is good:

72 | lazy def bar = 1 | ^ | modifier `lazy` is not allowed for this definition

That’s not the error message I’m fixing. Will separate the PR and add examples.

allanrenucci · 2018-03-07T14:41:58Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -1714,7 +1722,7 @@ object Parsers {
     *                  |  override
     *  LocalModifier  ::= abstract | final | sealed | implicit | lazy
     */
-    def modifiers(allowed: BitSet = modifierTokens, start: Modifiers = Modifiers()): Modifiers = {
+    def modifiers(allowed: BitSet = modifierTokens, start: Modifiers = Modifiers(), parsedSeparately: BitSet = BitSet.empty): Modifiers = {


I think a better API would something along the line of:

/** ... * @param errorOn emit a syntax error when a parsed modifier is in errorOn */ def modifiers(allowed: BitSet, start: Modifiers, errorOn: BitSet) = { ... if (allowed.contains(in.token) { ...; loop() } else if (errorOn.contains(in.token) { syntaxError; loop() } else ... }

You can keep parsing modifiers after encountering an invalid modifier.

But since it is used only once, I think we should no complicate this API.

odersky

Flag checking is generally done in Namer using checkWellFormed. I think this is where these checks should go as well.

Blaisorblade · 2018-03-07T21:13:59Z

SEALED must be rejected here, otherwise adding ParamAccessor will crash right away.
If you still want to move other checks later, I can try.

odersky · 2018-03-15T10:27:36Z

I believe we need a more systematic fix here. The root of the problem is that there are term flags and type flags and we are adding a term flag to a type flag or vice versa, which is not supported.

We should use in Parser a general flag combining method which detects and reports this problem. But don't change the parsed syntax! The syntax is given by syntax.md and should be parsed as such.

odersky · 2018-03-16T13:56:22Z

It turned out that the parser did already what I suggested. There was just one instance where the correct combiner method was not called. #4126 fixes this.

Blaisorblade requested a review from nicolasstucki March 4, 2018 22:54

Blaisorblade commented Mar 4, 2018

View reviewed changes

Blaisorblade force-pushed the fix-#4058 branch from 671b9a6 to a0d8d49 Compare March 5, 2018 13:49

allanrenucci reviewed Mar 7, 2018

View reviewed changes

Blaisorblade added 6 commits March 7, 2018 14:24

Fix scala#4058: reject sealed and lazy for class parameters in parser

9172a96

I noticed `sealed` while reviewing `modifiers`.

Add omitted @tailrec annotation (mostly for clarity)

8a79276

Test a few extra variants

6299785

This might be overkill, since these modifiers aren't expected here.

Don't special-case sealed

1d85ac7

Update grammar in comment to follow code

6f2effe

Report forbidden modifiers from modifiers

f4b3538

Missing: tests. Had to add to `modifiers` a new `parsedSeparately` parameter, and since its interface is tricky I wrote some documentation.

Blaisorblade force-pushed the fix-#4058 branch from a0d8d49 to f4b3538 Compare March 7, 2018 13:27

allanrenucci reviewed Mar 7, 2018

View reviewed changes

odersky requested changes Mar 7, 2018

View reviewed changes

nicolasstucki assigned odersky Mar 8, 2018

odersky assigned Blaisorblade and unassigned odersky Mar 15, 2018

odersky closed this Mar 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4058: reject sealed and lazy for class parameters in parser #4070

Fix #4058: reject sealed and lazy for class parameters in parser #4070

Blaisorblade commented Mar 4, 2018 •

edited

Loading

Blaisorblade Mar 4, 2018

allanrenucci commented Mar 5, 2018

Blaisorblade commented Mar 7, 2018

allanrenucci Mar 7, 2018

allanrenucci Mar 7, 2018

allanrenucci Mar 7, 2018

Blaisorblade Mar 7, 2018

Blaisorblade Mar 7, 2018

allanrenucci Mar 7, 2018

Blaisorblade commented Mar 7, 2018

allanrenucci Mar 7, 2018

Blaisorblade Mar 7, 2018

allanrenucci Mar 7, 2018

odersky left a comment

Blaisorblade commented Mar 7, 2018

odersky commented Mar 15, 2018

odersky commented Mar 16, 2018

Fix #4058: reject sealed and lazy for class parameters in parser #4070

Fix #4058: reject sealed and lazy for class parameters in parser #4070

Conversation

Blaisorblade commented Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

allanrenucci commented Mar 5, 2018

Blaisorblade commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blaisorblade commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky left a comment

Choose a reason for hiding this comment

Blaisorblade commented Mar 7, 2018

odersky commented Mar 15, 2018

odersky commented Mar 16, 2018

Blaisorblade commented Mar 4, 2018 •

edited

Loading