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.

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

Add support for JEP-409 (sealed classes) + Add javacOpt directive #19080

Merged
merged 4 commits into from
Feb 27, 2024
Merged

Add support for JEP-409 (sealed classes) + Add javacOpt directive #19080

merged 4 commits into from
Feb 27, 2024

Conversation

hamzaremmal
Copy link
Member

@hamzaremmal hamzaremmal commented Nov 25, 2023 •
edited
Loading

Closes #18533

This PR also adds the following directive to the vulpix infrastructure:

//> using javacOpt *

inspired by : https://scala-cli.virtuslab.org/docs/reference/directives#javac-options

@som-snytt
Copy link
Contributor

The scala 2 ticket links to the two PRs which may be helpful. The tricky part was that if permitted subclasses is not specified, you have to discover them in the source, but without requiring too much typechecking.

scala/bug#12171

@SethTisue SethTisue changed the title Add support for JEP-409 Add support for JEP-409 (sealed classes) Dec 14, 2023
@unkarjedy
Copy link
Contributor

Is this fix blocked by anything?

@hamzaremmal
Copy link
Member Author

No

@hamzaremmal hamzaremmal force-pushed the i18533 branch 2 times, most recently from 5e5021b to de004ca Compare February 14, 2024 17:05
@hamzaremmal hamzaremmal marked this pull request as ready for review February 14, 2024 20:01
@hamzaremmal
Copy link
Member Author

CI needs (at least) Java 21 to pass. Waiting for #18873.

hamzaremmal
hamzaremmal commented Feb 14, 2024
View reviewed changes
compiler/test/dotty/tools/utils.scala
private val directiveOptionsArg = raw"//> using options (.*)".r.unanchored
private val directiveJavacOptions = raw"//> using javacOpt (.*)".r.unanchored
Copy link
Member Author

@hamzaremmal hamzaremmal Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Inspired from scala-cli directives :https://scala-cli.virtuslab.org/docs/reference/directives#javac-options

hamzaremmal
hamzaremmal commented Feb 14, 2024
View reviewed changes
tests/pos/i18533/Cat.java
@@ -0,0 +1,7 @@
//> using javacOpt --enable-preview --source 17
Copy link
Member Author

@hamzaremmal hamzaremmal Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These options should be removed when CI is bumped to Java 21 (See #19701)

@som-snytt
Copy link
Contributor

There is a current syntax that is not yet migrated to cli-style just for avoiding old jvm.

./tests/run/t12348.scala:// test: -jvm 11+

@hamzaremmal
Copy link
Member Author

hamzaremmal commented Feb 15, 2024 •
edited
Loading

There is a current syntax that is not yet migrated to cli-style just for avoiding old jvm.


./tests/run/t12348.scala:// test: -jvm 11+

I'm familiar with this directive. I want to run the test at least once in the CI with the correct JVM before adding this directive.

@hamzaremmal hamzaremmal mentioned this pull request Feb 15, 2024
Merged
@hamzaremmal hamzaremmal changed the title Add support for JEP-409 (sealed classes) Add support for JEP-409 (sealed classes) + Add javacOpt directive Feb 15, 2024
@hamzaremmal hamzaremmal force-pushed the i18533 branch 3 times, most recently from f94d219 to 11d0290 Compare February 16, 2024 14:21
@dwijnand dwijnand assigned bishabosha and unassigned hamzaremmal Feb 19, 2024
bishabosha
bishabosha reviewed Feb 27, 2024
View reviewed changes
compiler/src/dotty/tools/dotc/typer/Namer.scala
@@ -1569,7 +1570,7 @@ class Namer { typer: Typer =>
if pclazz.is(Final) then
report.error(ExtendFinalClass(cls, pclazz), cls.srcPos)
else if pclazz.isEffectivelySealed && pclazz.associatedFile != cls.associatedFile then
if pclazz.is(Sealed) then
if pclazz.is(Sealed) && !pclazz.is(JavaDefined) then
Copy link
Member

@bishabosha bishabosha Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this would seem to allow extending a sealed Java class in a Scala file, perhaps use !isJava instead which tests the compilation unit. Or could it be legitimate to define a permitted sub class in Scala?

Copy link
Member

@bishabosha bishabosha Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually it is allowed, If I compile with scalac Cat.scala and Pet.java in run 1, then javac on Pet.java in run 2 with classpath of run 1 outputs, then it works

Copy link
Member

@bishabosha bishabosha Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

maybe it could be interesting to add this as a test case in a follow up

@bishabosha bishabosha merged commit d35eba0 into scala:main Feb 27, 2024
19 checks passed
@hamzaremmal hamzaremmal deleted the i18533 branch February 27, 2024 17:46
@hamzaremmal hamzaremmal mentioned this pull request Feb 28, 2024
Merged
hamzaremmal added a commit that referenced this pull request Feb 28, 2024
@hamzaremmal
Add scala3-bootstrapped/testCompilation to 'test_windows_fast' (#19810)
8bcd9a9 
In light of the issue (post merge) of #19080 . We should add test
compilation to windows_fast (not only windows_full).

Also see: #19802

cc @sjrd
@Kordyjan Kordyjan added this to the 3.4.2 milestone Mar 28, 2024
@WojciechMazur WojciechMazur mentioned this pull request Jul 1, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jul 2, 2024
@WojciechMazur
Backport "Add support for JEP-409 (sealed classes) + Add javacOpt dir…
7076d40 
…ective" to LTS (#20943)

Backports #19080 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
Reviewers

@nicolasstucki nicolasstucki nicolasstucki left review comments

@bishabosha bishabosha bishabosha approved these changes

Assignees

@bishabosha bishabosha

Labels
None yet
Projects
None yet
Milestone
3.4.2
Development

Successfully merging this pull request may close these issues.

Java parser doesn't support JEP 409 sealed classes
6 participants
@hamzaremmal @som-snytt @unkarjedy @nicolasstucki @bishabosha @Kordyjan