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

[javac] some differences ecj <-> javac regarding 'recent' features #2959

Open
stephan-herrmann opened this issue Sep 15, 2024 · 16 comments
Open
Assignees
Labels
test Work on unit tests, no change of productive code

Comments

@stephan-herrmann
Copy link
Contributor

While taking stock of run.javac test results regarding Java 23 I observed the following differences which might be within reach for resolution:

  • PrimitiveInPatternsTestSH
  • MarkdownCommentTest:
    • 1 error, javac not complaining about unresolvable @see reference
  • ModuleCompilationTests
    • 4 errors
  • PatternMatching16Test -- is this a structural problem?
    • 83 failures, obviously not suitable for running back-to-back with javac23
    • 67 errors vs javac16
  • RecordPatternTest 3 errors, javac-generated code throws ArithmeticException instead of expected MatchException (see comments in tests):
    • testGH1977_instance_field()
    • testGH1977_static_field()
    • testIssue1985_2()
  • SwitchExpressionsYieldTest
    • currently 175 failures, many of which due to ill-configured javac invocation
    • remaining: 17 failures
  • SwitchPatternTest: 5 errors (some related to wrongly configured --enable-preview, others not yet analysed)
  • SwitchPatternTest21: 2 errors
    * testInternalDomination_this() (ecj no error, javac 2 errors)
    * testNaming() // javac jdk21 allows components to be named, but they can't be referenced
@stephan-herrmann stephan-herrmann self-assigned this Sep 15, 2024
stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Sep 15, 2024
+ fix arguments of javac invocations
+ remove a forgotten test filter

relates to eclipse-jdt#2959
@stephan-herrmann stephan-herrmann added the test Work on unit tests, no change of productive code label Sep 15, 2024
stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Sep 24, 2024
+ fix arguments of javac invocations
+ remove a forgotten test filter

relates to eclipse-jdt#2959
stephan-herrmann added a commit that referenced this issue Sep 24, 2024
…2960)

+ fix arguments of javac invocations
+ remove a forgotten test filter

relates to #2959
@stephan-herrmann
Copy link
Contributor Author

SwitchPatternTest21: 2 errors

  • testInternalDomination_this() (ecj no error, javac 2 errors)
  • testNaming() // javac jdk21 allows components to be named, but they can't be referenced

The class has been renamed to SwitchPatternTest22.

@srikanth-sankaran are those differences of interest for your current work?

testInternalDomination_this()

  • javac reports errors, that are not (yet?) detected by ecj:
X.java:5: error: this case label is dominated by a preceding case label
      case Object _, Integer _, X _ when o != null : System.out.println("Integer");
                     ^
X.java:5: error: this case label is dominated by a preceding case label
      case Object _, Integer _, X _ when o != null : System.out.println("Integer");
                                ^
2 errors

testNaming():

  • javac does not report the expected error, but only a warning:
X.java:6: warning: [fallthrough] possible fall-through into case
      default: System.out.println("Object");
      ^
1 warning

Test case has this comment "javac jdk21 allows components to be named, but they can't be referenced".

When those are clarified, we can accept this test class into the paradise of successful run.javac comparison :)

@stephan-herrmann
Copy link
Contributor Author

After #3218 also SwitchPatternTest is on the home stretch with just three test cases where javac reports errors which ecj doesn't see:

And then the errors from #3009 look similar:

X.java:26: error: illegal start of type
		case W<Short>.NG.I<Integer>.S<Long, Integer, X> e -> 42;
		                                                ^
1 error

@stephan-herrmann
Copy link
Contributor Author

After first successful builds I have set https://ci.eclipse.org/jdt/job/eclipse.jdt.core-run.javac-23/ to run weekly (on weekends)

Currently these test classes have opted in:

  • ImplicitlyDeclaredClassesTest
  • ModuleImportTests
  • PrimitivePatternTest
  • PrimitivePatternTestSH
  • SealedTypesTest
  • SuperAfterStatementsTest

More classes will be admitted once they are green in run.javac mode.

@srikanth-sankaran
Copy link
Contributor

More classes will be admitted once they are green in run.javac mode.

Thanks a lot for doing this!

In my first innings as well, I never seem to have paid close attention (scratch that, any attention) to this capability. In fact I seem to have invented a completely new mechanism that is a lot less sophisticated, see org.eclipse.jdt.core.tests.compiler.parser.AbstractSyntaxTreeTest.CHECK_JAVAC_PARSER

This is useful and I can participate in contributing and maintaining this in 2025 - after my reviews of Java 10-23 are over.

@stephan-herrmann
Copy link
Contributor Author

Latest build https://ci.eclipse.org/jdt/job/eclipse.jdt.core-run.javac-24/6/testReport/ (yes, focus is on 24 by now) shows a few failures in javac-comparison. This is with "build 24-ea+22-2649". Running the same tests locally with "build 24-ea+29-3578" those failures no longer occur.

I filed https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/5463 which should eventually grant us a green build.

stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Jan 4, 2025
+ acknowledge fix versions of javac bugs
+ let InterfaceMethodsTest opt-in to run.javac mode

Relates to eclipse-jdt#2959
stephan-herrmann added a commit that referenced this issue Jan 4, 2025
…3512)

+ acknowledge fix versions of javac bugs
   - https://bugs.openjdk.org/browse/JDK-8343306
   - https://bugs.openjdk.org/browse/JDK-8337980
+ let InterfaceMethodsTest opt-in to run.javac mode

Relates to #2959
@stephan-herrmann
Copy link
Contributor Author

We have a green build with run.javac=OptIn and these test classes opting in:

  • ImplicitlyDeclaredClassesTest
  • InterfaceMethodsTest
  • ModuleImportTests
  • PrimitiveInPatternsTest
  • PrimitiveInPatternsTestSH
  • SealedTypesTests
  • SuperAfterStatementsTest

@stephan-herrmann
Copy link
Contributor Author

Recently, these tests had some issues

  • build failure due to releng issues and merge problems
  • changes in javac behavior between 23 and 24 causing our comparison to complain
  • ModuleImportTests had not been run (missing inclusion in TestAll).

All these have been resolved as of now, we have a green build again at https://ci.eclipse.org/jdt/job/eclipse.jdt.core-run.javac-24/15/

@stephan-herrmann
Copy link
Contributor Author

New test participating since #3616

  • PreviewFlagTest

@srikanth-sankaran
Copy link
Contributor

@srikanth-sankaran are those differences of interest for your current work?

Sorry for the late response.

I believe org.eclipse.jdt.core.tests.compiler.regression.SwitchPatternTest22.testNaming() is a bug in javac and ECJ behavior is correct.

org.eclipse.jdt.core.tests.compiler.regression.SwitchPatternTest22.testInternalDomination_this() OTOH looks like an issue with us. I have opened a separate ticket : #3626

@stephan-herrmann
Copy link
Contributor Author

stephan-herrmann commented Jan 28, 2025

@srikanth-sankaran are those differences of interest for your current work?

Sorry for the late response.

I believe org.eclipse.jdt.core.tests.compiler.regression.SwitchPatternTest22.testNaming() is a bug in javac and ECJ behavior is correct.

I agree, 14.11.1 has this normative sentence:

It is a compile-time error for a case label to have more than one case pattern and declare any pattern variables (other than those declared by a guard associated with the case label).

and this non-normative explanation:

If a case label with more than one case pattern could declare pattern variables, then it would not be clear which variables would be initialized if the case label were to apply.

Over to https://mail.openjdk.org/pipermail/compiler-dev/2025-January/029309.html

@stephan-herrmann
Copy link
Contributor Author

For now I disabled https://ci.eclipse.org/jdt/job/eclipse.jdt.core-run.javac-23/ after it showed a flood of failures, many (all?) of which are connected to #3586. Those failures got fixed in BETA_JAVA24 only (71b9831).

Focus is on Java 24 anyway.

stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Feb 1, 2025
+ more infra fixes for run.javac mode
+ don't run ModuleImportTests at 23 (needs 24-preview)
+ bookkeeping re https://bugs.openjdk.org/browse/JDK-8348928
  (SwitchPatternTest22 doesn't yet OptIn to run.javac)

Relates to eclipse-jdt#2959
stephan-herrmann added a commit that referenced this issue Feb 1, 2025
…3648)

+ more infra fixes for run.javac mode
+ don't run ModuleImportTests at 23 (needs 24-preview)
+ bookkeeping re https://bugs.openjdk.org/browse/JDK-8348928
  (SwitchPatternTest22 doesn't yet OptIn to run.javac)

Relates to #2959
@stephan-herrmann
Copy link
Contributor Author

After next merge from master, SwitchPatternTest22 wants to opt-in to run.javac mode, see #3626.

@stephan-herrmann
Copy link
Contributor Author

While working on #3687 I see LocalEnumTest as another candidate to opt in to run.javac. With pending fixes I see exactly two differences vis a vis javac:

  • test022: ecj complains that case MX.BLEU must be replaced with BLEU, javac is quiet
  • test140: cast from enum type to Runnable: javac complains, ecj is quiet

@srikanth-sankaran @mpalat @jarthana let me know if you recall any "recent" changes in either area. Thanks.

robstryker pushed a commit to robstryker/eclipse.jdt.core that referenced this issue Feb 6, 2025
…clipse-jdt#2960)

+ fix arguments of javac invocations
+ remove a forgotten test filter

relates to eclipse-jdt#2959
@srikanth-sankaran
Copy link
Contributor

@srikanth-sankaran @mpalat @jarthana let me know if you recall any "recent" changes in either area. Thanks.

Please see

https://bugs.openjdk.org/browse/JDK-8309372
https://bugs.openjdk.org/browse/JDK-8300542

Perhaps our implementation has gaps. Could you please raise a ticket after confirming this to be the case and assign it to me? I'll follow up.

BTW, I see that org.eclipse.jdt.core.tests.compiler.regression.EnumTest.test022() is not run for >= JDK21. This disabling happened on behalf of #1252 - which is interesting. It looks like we will allow qualified enums while switching on sealed interfaces but now while switching on enumeration themselves.

@srikanth-sankaran
Copy link
Contributor

See emission of cannotUseQualifiedEnumConstantInCaseLabel() in org.eclipse.jdt.internal.compiler.ast.CaseStatement.resolveConstantLabel(BlockScope, TypeBinding, TypeBinding, Expression)

@stephan-herrmann
Copy link
Contributor Author

Please welcome our new member (via #3713):

  • LocalEnumTest

😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Work on unit tests, no change of productive code
Projects
None yet
Development

No branches or pull requests

2 participants