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

Arguments should be lifted in explicit constructor calls of annotation classes #22526

Closed
mbovel opened this issue Feb 5, 2025 · 7 comments · Fixed by #22553
Closed

Arguments should be lifted in explicit constructor calls of annotation classes #22526

mbovel opened this issue Feb 5, 2025 · 7 comments · Fixed by #22553

Comments

@mbovel
Copy link
Member

mbovel commented Feb 5, 2025

Compiler version

Scala 3.7.0-RC1-bin-20250204-d75ca7f-NIGHTLY

Minimized code

I fixed a bug in (#22035) where arguments were lifted, breaking assumptions later in the compiler.

However I haven't special-cased it for explicit constructor calls, so we currently get:

class ann(x: Int = 3, y: Int = 4) extends annotation.Annotation
class C(x: Int = 3, y: Int = 4)

@main def main =
  val a: Int @ann(y={println(1); 1}, x={println(2); 2}) = 42
  val b = new ann(y={println(1); 1}, x={println(2); 2})

  val c = new C(y={println(1); 1}, x={println(2); 2})
 ~/scala-snippets-6 scala -S 3.nightly -Xprint:typer --server=false annot-default-args.scala
Downloading Scala 3.7.0-RC1-bin-20250204-d75ca7f-NIGHTLY compiler
[[syntax trees at end of                     typer]] // /Users/mbovel/scala-snippets-6/annot-default-args.scala
...
    @main def main: Unit =
      {
        val a:
          Int @ann(
            x =
              {
                println(2)
                2
              }
            ,
            y =
              {
                println(1)
                1
              }
          )
         = 42
        val b: ann =
          new ann(
            x =
              {
                println(2)
                2
              }
            ,
            y =
              {
                println(1)
                1
              }
          )
        val c: C =
          {
            val y$1: Int =
              {
                println(1)
                1
              }
            val x$1: Int =
              {
                println(2)
                2
              }
            new C(x = x$1, y = y$1)
          }
        ()
      }
  }
...
}

2
1
1
2

Arguably, the arguments should still be lifted when the instance is constructed explicitly, as in b above.

@mbovel mbovel added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label area:annotations and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 5, 2025
@mbovel mbovel changed the title Arguments shoudl be lifted in explicit constructor calls of annotation classes Arguments should be lifted in explicit constructor calls of annotation classes Feb 5, 2025
@mbovel
Copy link
Member Author

mbovel commented Feb 5, 2025

Not sure how we could differentiate a and b; the annotation constructor is typed-checked exactly the same way in both cases.

@hamzaremmal
Copy link
Member

I'm more worried about the fact that the evaluation order is not preserved here...
We shouldn't print 2 and then 1, we should always print 1 and then 2

@lrytz
Copy link
Member

lrytz commented Feb 5, 2025

Not sure how we could differentiate a and b; the annotation constructor is typed-checked exactly the same way in both cases.

In the Scala 2 PR I added a typer mode: scala/scala#10976

@mbovel
Copy link
Member Author

mbovel commented Feb 6, 2025

We shouldn't print 2 and then 1, we should always print 1 and then 2

Yes yes, that's the point of this issue 😄

@mbovel
Copy link
Member Author

mbovel commented Feb 6, 2025

In the Scala 2 PR I added a typer mode: scala/scala#10976

In Dotty, I've been told a few times not to add Mode bits (we have 30 out of 32 used currently), and I would expect the same answer here.

The documentation of Mode says we could use a setting for low priority situations, but I also don't think it would be appropriate here because it is really an internal thing.

And passing a parameter from typedAnnotation down to TypedApply seems tricky.

Would you have an opinion about that @dwijnand? (Asking you because you wrote the doc comment of Mode.)

Higher-level question: is there a use-case for this? Couldn't we just forbid creating annotation instances explicitly?

@dwijnand
Copy link
Member

dwijnand commented Feb 6, 2025

Would you have an opinion about that @dwijnand? (Asking you because you wrote the doc comment of Mode.)

No strong opinion. I feel like I wrote what Martin wrote elsewhere (on Slack maybe?). I guess we just create an internal setting and set it.

@lrytz
Copy link
Member

lrytz commented Feb 7, 2025

In Dotty, I've been told a few times not to add Mode bits (we have 30 out of 32 used currently), and I would expect the same answer here.

Make it a Long?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants