8000 Add synthetic constructors only to Java annotations by Jasper-M · Pull Request #10016 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

Add synthetic constructors only to Java annotations #10016

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 10, 2022
Merged

Add synthetic constructors only to Java annotations #10016

merged 2 commits into from
May 10, 2022

Conversation

Jasper-M
Copy link
Contributor
@Jasper-M Jasper-M commented Apr 26, 2022 •
edited by lrytz
Loading

Backport of #9981, but with improvements to be forward-ported.

@scala-jenkins scala-jenkins added this to the 2.12.17 milestone Apr 26, 2022
@Jasper-M
Copy link
Contributor Author

Is there some kind of 2.12 alternative for this javaVersion: 9+ thing?
Also, does anyone understand why the more conservative change resulted in this NullPointerException?

@SethTisue SethTisue modified the milestones: 2.12.17, 2.12.16 Apr 26, 2022
@SethTisue SethTisue requested review from lrytz and som-snytt April 26, 2022 14:26
@SethTisue SethTisue marked this pull request as draft April 26, 2022 14:26
@som-snytt
Copy link
Contributor

I did take a look earlier and wanted to 😃 on "go nuts". The unfortunate build error dissuaded me from looking further. I almost commented, "Just when you think it's an easy backport..."

@som-snytt
Copy link
Contributor

I'm required to do further penance because I broke the community build.

Is this considered a desirable backport because people on 2.12 are likely to be using -release or whatever the option is currently?

Or was it hoped to be fruit of a low-hanging variety. Like strawberries.

IIRC retronym did work on both branches: it's genuine retronym, not a knock-off.

I saw the NPE in test but hadn't looked yet.

@Jasper-M
Copy link
Contributor Author

I use it on 2.12. I don't know about other people.

@som-snytt
Copy link
Contributor
som-snytt commented Apr 27, 2022 •
edited
Loading

Lukas's javaVersion was an easier backport so I did that first, at your urging. POC

-- 1 - neg/t12565.scala                          [skipped on Java 17, only running on 19+]

#10021

@som-snytt
Copy link
Contributor

The zinc NPE is an interaction with Lukas's sbt/zinc@c25d956 where the common elements is lazy completion of annotation info, but I don't understand the cogs yet. That is really the point of the whole misadventure.

@SethTisue SethTisue modified the milestones: 2.12.16, 2.12.17 May 9, 2022
@SethTisue
Copy link
Member
SethTisue commented May 9, 2022 •
edited
Loading

@Jasper-M the window for 2.12.16 is closing soon

@lrytz lrytz force-pushed the backport-2.12/12565 branch from 65ca2da to 882bf61 Compare May 9, 2022 12:25
@lrytz
Copy link
Member
lrytz commented May 9, 2022

I can explain the NPE, a bunch of things playing together here...

  // current 2.13 version
  (sflags & (INTERFACE|JAVA_ANNOTATION)) == (INTERFACE|JAVA_ANNOTATION)

Note that we're using sflags, which is defined as val sflags = jflags.toScalaFlags. Interestingly:

// 2.12
scala> typeOf[Deprecated].typeSymbol.debugFlagString
res0: String = <java> <annotation>

// 2.13
scala> typeOf[Deprecated].typeSymbol.debugFlagString
val res0: String = <interface> abstract <java> <defaultparam/trait> <annotation>

which is due to this (really nice) cleanup: 7c9e3724ee#diff-b47f6177947626919b42f078a149a0e822e464c4a96b39c2dfbaef1f2e43ea34

So backporting the 2.13 version to 2.12 meant that no more artificial constructors were added to Java annotations at all (INTERFACE is not there on 2.12). This ended up in an NPE (instead of a better error message) because in typedSelect:

scala/src/compiler/scala/tools/nsc/typechecker/Typers.scala

Line 5069 in 29e696f

val sym = tree.symbol orElse member(qual.tpe, name) orElse inCompanionForJavaStatic(qual.symbol, name)

scala/src/compiler/scala/tools/nsc/typechecker/Typers.scala

Lines 5048 to 5051 in 29e696f

def inCompanionForJavaStatic(cls: Symbol, name: Name): Symbol =
if (!(context.unit.isJava && cls.isClass)) NoSymbol else {
context.javaFindMember(cls.typeOfThis, name, _ => true)._2
}

The qual here is new Deprecated, the selected name is <init>. The qual.symbol is null. The correct symbol here would be qual.tpe.typeSymbol.

I'm all for the nutty version, also for forward-porting it to 2.13.

@lrytz lrytz force-pushed the backport-2.12/12565 branch from 882bf61 to 5f67938 Compare May 9, 2022 14:31
@lrytz lrytz changed the title [backport] Refrain from adding ctor to class from ct.sym Add synthetic constructors only to Java annotations May 9, 2022
@lrytz lrytz marked this pull request as ready for review May 9, 2022 14:35
@lrytz lrytz modified the milestones: 2.12.17, 2.12.16 May 9, 2022
@Jasper-M
Copy link
Contributor Author
Jasper-M commented May 9, 2022

Thanks @lrytz and @som-snytt. I'm on holiday so I had more or less resigned to missing 2.12.16.

som-snytt
som-snytt approved these changes May 9, 2022
View reviewed changes
Copy link
Contributor
@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

It's no coincidence that the dashboard icon on my car for "lane keep assistance", that keeps you from driving off a cliff, is LKAS.

I'll offer the forward port.

som-snytt
som-snytt reviewed May 9, 2022
View reviewed changes
@som-snytt som-snytt mentioned this pull request May 10, 2022
Merged
@lrytz lrytz force-pushed the backport-2.12/12565 branch from 5f67938 to 63b21b2 Compare May 10, 2022 06:28
@lrytz lrytz force-pushed the backport-2.12/12565 branch from 63b21b2 to 1a33a74 Compare May 10, 2022 07:37
@som-snytt
Copy link
Contributor

The diff is an ideal +27 -27.

@SethTisue
Copy link
Member

@lrytz you already approved the forward-port; did you mean to approve this one as well?

@SethTisue SethTisue added the prio:hi high priority (used only by core team, only near release time) label May 10, 2022
@lrytz
Copy link
Member
lrytz commented May 10, 2022

Hard to say who should approve it :-) LGTsom & LGTM, so 🚀

@lrytz lrytz merged commit c850a83 into scala:2.12.x May 10, 2022
@SethTisue SethTisue removed the prio:hi high priority (used only by core team, only near release time) label May 10, 2022
This was referenced Jun 8, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@som-snytt som-snytt som-snytt approved these changes

@lrytz lrytz Awaiting requested review from lrytz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.12.16
Development

Successfully merging this pull request may close these issues.
5 participants
@Jasper-M @som-snytt @SethTisue @lrytz @scala-jenkins
0