Skip to content

Failed compile when Java annotation used as parameter of another Java annotation #19959

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

Closed
wiwiwa opened this issue Mar 16, 2024 · 7 comments
Closed
Assignees
Labels
area:annotations compat:java itype:bug regression This worked in a previous version but doesn't anymore

Comments

@wiwiwa
Copy link

wiwiwa commented Mar 16, 2024

Compiler version

Scala 3.3 and 3.4

Minimized code

//> using scala 3.4.0
//> using dep "jakarta.persistence:jakarta.persistence-api:3.1.0"

import jakarta.persistence.*

@Table(indexes=Array(Index(columnList="email")))
class User

Output

$ scala-cli run test.scala
Compiling project (Scala 3.4.0, JVM (20))
[error] ./test.scala:6:22
[error] object Index in package jakarta.persistence does not take parameters
[error] @Table(indexes=Array(Index(columnList="email")))
[error]                      ^^^^^
Error compiling project (Scala 3.4.0, JVM (20))
Compilation failed

Expectation

The code compiles with Scala <= 3.2, but does not compile with higher version of Scala. Expected to be working with Scala 3.4

@wiwiwa wiwiwa added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 16, 2024
@wiwiwa wiwiwa changed the title Failed compile when annotation used as parameter of another annotation Failed compile when Java annotation used as parameter of another Java annotation Mar 16, 2024
@Gedochao
Copy link
Contributor

Ok, so this is weird... So far I didn't manage to minimise it, despite the code from jakarta.persistance being relatively simple, so it may be something specific to the jakarta build.
We'd perhaps need a self-contained minimisation to tell more. (without the jakarta dependency).
The example indeed compiles on Scala 3.2.0, but doesn't on LTS nor on 3.4.0.

However, I wonder if it's a valid bug in the first place.
The following code compiles on 3.4.0:

//> using scala 3.4.0
//> using dep "jakarta.persistence:jakarta.persistence-api:3.1.0"

import jakarta.persistence.Index
import jakarta.persistence.Table

@Table(indexes=Array(new Index(columnList="email")))
class User

So the new keyword seems to be necessary to call the constructor here.
Should it be necessary, however?

@Gedochao Gedochao added stat:needs spec and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 18, 2024
@Gedochao
Copy link
Contributor

@sjrd Would you consider this a valid regression?
Should the new keyword be required when passing an anotation parameter like this?

@sjrd
Copy link
Member

sjrd commented Mar 18, 2024

You're not supposed to need a new in Scala 3, whether you're in annotations or not. So IMO this is a real regression.

@wiwiwa
Copy link
Author

wiwiwa commented Mar 19, 2024

Yes. Adding a new keyword fixed my problem. Thanks

@wiwiwa wiwiwa closed this as completed Mar 19, 2024
@som-snytt
Copy link
Contributor

using local java sources, 3.2.2 also fails to compile.

package trial;

import java.lang.annotation.*;

@Target({})
@Retention(RetentionPolicy.RUNTIME)
public @interface Index {
  String name() default "";

  String columnList();
}

package trial;

import java.lang.annotation.*;

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
public @interface Table {
  String name() default "";

  Index[] indexes() default {};
}

using

ThisBuild / compileOrder := CompileOrder.JavaThenScala

makes it compile, which is obviously a bug or limitation.

Nothing works on 3.3.3 or 3.4 except new.

Outside of annotation context, 3.2.2 crashes:

java.util.NoSuchElementException: class String while running genBCode

but 3.3 emits normal error.

That edit of Scala 2 spec on annotations isn't ported yet, but then perhaps "Host-platform Annotations" could be updated. Currently it says it should just work, unless it doesn't.

additional restrictions on the expressions which are valid as annotation arguments

probably doesn't intend to say that you have to use new. Java requires @.

Not sure how incremental compilation faked out javac here:

[success] Total time: 0 s, completed Mar 18, 2024, 9:28:07 PM
[info] compiling 1 Scala source and 1 Java source to .../i19959/target/scala-3.3.3/classes ...
[error] .../i19959/src/main/java/trial/Used.java:5:1: cannot find symbol
[error]   symbol: class Table
[error] Table
[error] .../i19959/src/main/java/trial/Used.java:5:1: cannot find symbol
[error]   symbol: class Index
[error] Index
[error] (Compile / compileIncremental) javac returned non-zero exit code
[error] Total time: 0 s, completed Mar 18, 2024, 9:28:07 PM

@som-snytt som-snytt reopened this Mar 19, 2024
@Gedochao Gedochao added regression This worked in a previous version but doesn't anymore and removed stat:needs spec stat:needs minimization Needs a self contained minimization labels Mar 19, 2024
@smarter
Copy link
Member

smarter commented May 1, 2024

You're not supposed to need a new in Scala 3, whether you're in annotations or not. So IMO this is a real regression.

This was actually an intended change, quoting from 3125665 :

This change is not fully backwards source compatible: this is illustrated
by the diffs in tests/run/repeatable/Test_1.scala:

-@FirstLevel_0(Array(Plain_0(4), Plain_0(5)))
+@FirstLevel_0(Array(new Plain_0(4), new Plain_0(5)))

Here, FirstLevel_0 takes an array of Plain_0 annotations as arguments, and in
previous releases of Scala 3 we could put Plain_0(4) in this array without
new. This is because the compiler generates a "constructor proxy" apply method
for classes, but this no longer works since Plain_0 is now a trait. While we
could potentially tweak the constructor proxy logic to handle this case, it
seems simpler to require a new here, both because Scala 2 does it too and
because it ensures that user code that inspects the annotation tree does not
have to deal with constructor proxies.

Given that the current encoding of constructors with annotations is already causing us headaches (#19951 (comment)), it's probably wiser to not make things more complicated by adding constructor proxies in the mix, at least not until #19951 has been fixed.

@Gedochao
Copy link
Contributor

Gedochao commented May 6, 2024

Okay, so this is blocked by #19951.

@hamzaremmal hamzaremmal closed this as not planned Won't fix, can't repro, duplicate, stale Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:annotations compat:java itype:bug regression This worked in a previous version but doesn't anymore
Projects
None yet
Development

No branches or pull requests

7 participants