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

Add a specific error message for local final defs #20557

Merged
merged 2 commits into from
Jul 2, 2024
Merged

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Jun 11, 2024

Fixes #17579.

@mbovel mbovel changed the title Add an specific error message for local final defs Add a specific error message for local final defs Jun 11, 2024
@mbovel mbovel requested a review from odersky June 11, 2024 20:47
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

The main thing is to remove or motivate the restriction why GIVEN is excluded. Otherwise LGTM.

-- [E088] Syntax Error: tests/neg/17579.scala:6:10 ---------------------------------------------------------------------
6 | final private val v3 = 42 // error
| ^^^^^^^
| Expected start of definition
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a different error message here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see why: since private is also not a local modifier token.

Copy link
Contributor

Choose a reason for hiding this comment

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

This means we could generalize the solution. We could always parse all tokens and then flag those that are not legal at this point. But this could be another PR.

@mbovel
Copy link
Member Author

mbovel commented Jun 28, 2024

The main thing is to remove or motivate the restriction why GIVEN is excluded.

We thought we should not emit an error in this case, as final has a different meaning for classes, which given can be translated to. It makes sense to define a final class Foo in a local scope for example.

And there is an existing warning for final given:

trait Foo:
  def foo: Int

@main def f() =
  final given Foo with
    def foo = 42
~/scala-snippets-5 scala-cli -Xprint:typer final_given.scala
Compiling project (Scala 3.4.1, JVM (17))
[warn] ./final_given.scala:7:3
[warn] Modifier final is redundant for this definition
[warn]   final given Foo with
[warn]   ^^^^^
[[syntax trees at end of                     typer]] // /Users/mbovel/scala-snippets-5/final_given.scala
package final_given {
  trait Foo() extends Object {
    def foo: Int
  }
  final lazy module val final_given$package: final_given.final_given$package =
    new final_given.final_given$package()
  final module class final_given$package() extends Object() {
    this: final_given.final_given$package.type =>
    @main def f(): Unit =
      {
        final lazy module given val given_Foo: given_Foo = new given_Foo()
        final module class given_Foo() extends Object(), final_given.Foo {
          this: given_Foo.type =>
          def foo: Int = 42
        }
...

Does that make sense? Should I had a comment to explain this where we through the error?

@mbovel
Copy link
Member Author

mbovel commented Jul 1, 2024

I rebased on main, and added comments in the parser and in the test case.

@mbovel mbovel requested a review from odersky July 1, 2024 09:02
Co-Authored-By: Eugene Flesselle <eugene@flesselle.net>
Co-Authored-By: anna herlihy <herlihyap@gmail.com>
Co-Authored-By: Oliver Bračevac <bracevac@users.noreply.github.com>
@odersky odersky merged commit b481932 into scala:main Jul 2, 2024
24 checks passed
@mbovel mbovel deleted the mb/spree branch July 2, 2024 09:14
@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
WojciechMazur added a commit that referenced this pull request Jul 10, 2024
…21155)

Backports #20557 to the LTS branch.

PR submitted by the release tooling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report a clearer error message for unexpected final modifier
3 participants