Fix #4815: Illegal modifier on local def

[abeln]

During typechecking, disallow the use of final in local defs.

e.g.

def foo = {
  final def bar = 42 // error: final in local def is disallowed
}

[allanrenucci]

This should be fixed in the parser. The parser currently rejects:

class Test {
  def foo = {
    private def bar = 1
  }
}

I would check why final is not rejected as well

[allanrenucci]

scalac rejects this. I think we should reject it as well

[abeln]

I'm not sure I understand. We do reject this. This is what this PR does.

[allanrenucci]

Sorry, it was meant for the line below (i.e. final val v = 42)

[abeln]

Ah, I see. There's at least one test that uses this feature (constant value expression):

• https://github.com/lampepfl/dotty/blob/946ba3f2d938b54e316749aef1c2ff117ecdb325/tests/run/tasty-eval/quoted_2.scala#L8
• Additionally, it seem to be explicitly allowed by the spec: https://scala-lang.org/files/archive/spec/2.12/04-basic-declarations-and-definitions.html#value-declarations-and-definitions

A constant value definition is of the form

final val x = e

where e is a constant expression. The final modifier must be present and no type annotation may be given. References to the constant value x are themselves treated as constant expressions; in the generated code they are replaced by the definition's right-hand side e.

[allanrenucci]

This part of the spec only specifies what a constant value is, not where it is allowed. Looking at the syntax part of the spec, it doesn't seems to be allowed

  BlockStat         ::=  Import
                      |  {Annotation} [‘implicit’ | ‘lazy’] Def
                      |  {Annotation} {LocalModifier} TmplDef
                      |  Expr1
  Def               ::=  PatVarDef
                      |  ‘def’ FunDef
                      |  ‘type’ {nl} TypeDef
                      |  TmplDef
  PatVarDef         ::=  ‘val’ PatDef
                      |  ‘var’ VarDef

[abeln]

Thanks for taking a look. Here's why I ended up adding the check in the typer.

There are four places I considered doing the check on:

• the list of modifiers that are allowed in front of local definitions (https://github.com/lampepfl/dotty/blob/36c2e34bc33a39a41ec383e997185613bf88c64c/compiler/src/dotty/tools/dotc/parsing/Tokens.scala#L229). This is what rules out your private def bar = 1 example.
  Unfortunately, final can appear in a local definition (for example, in front of a class): e.g.

def foo = {
  final class Bar;
}

• we could check the next token inside localDef (https://github.com/lampepfl/dotty/blob/946ba3f2d938b54e316749aef1c2ff117ecdb325/compiler/src/dotty/tools/dotc/parsing/Parsers.scala#L2546) and fail if it's a DEF and final is one of the modifiers.

This seemed rather ugly because the first thing defOrDcldoes is dispatches based on the next token, so we would add a bit of code duplication.

• we could add the check inside defOrDcl (https://github.com/lampepfl/dotty/blob/946ba3f2d938b54e316749aef1c2ff117ecdb325/compiler/src/dotty/tools/dotc/parsing/Parsers.scala#L2095), but it's called not only for local declarations, but in other cases as well, so this breaks things.

• finally, we can add the check in Checking.checkWellFormed (which is what I ended up doing). You'll notice there's already a check for (not) combining final with trait there (https://github.com/lampepfl/dotty/blob/36c2e34bc33a39a41ec383e997185613bf88c64c/compiler/src/dotty/tools/dotc/typer/Checking.scala#L388), so this seemed like a natural place.

Does that make sense? Is there a reason the check should be shoehorned into the Parser?

[allanrenucci]

I would look at what scalac does https://github.com/scala/scala/blob/874bf039af75361be84e4c89de0457ab019573ff/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala#L3247-L3301

[abeln]

Thanks for the link. I think I see how to make the Dotty version of localDef similar to the scalac one, but first we need to figure out the question in the comment above about constant value expressions. WDYT?

[abeln]

Moved the change to the parser a la scalac. PTAL.

[allanrenucci]

LGTM. Thanks @abeln 🎉

You can change the final val in dotty.tools.languageserver.Memory to a normal val

[abeln]

Done. PTAL.