Skip to content

Conversation

IdrissRio
Copy link
Contributor

@IdrissRio IdrissRio commented Jul 29, 2025

This PR adds a library test to verify the Java extractor support for flexible constructor bodies, a feature introduced in JEP 513 and finalized in Java 25..

Please ignore any DCA linked.

@github-actions github-actions bot added the Java label Jul 29, 2025
@IdrissRio IdrissRio force-pushed the idrissrio/flexible-constructors branch from 357ed2f to 3667004 Compare July 29, 2025 13:40
@IdrissRio IdrissRio marked this pull request as ready for review July 30, 2025 06:47
@Copilot Copilot AI review requested due to automatic review settings July 30, 2025 06:47
@IdrissRio IdrissRio requested a review from a team as a code owner July 30, 2025 06:47
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a library test to verify Java extractor support for flexible constructor bodies, a new language feature introduced in JEP 513 and finalized in Java 25. The test validates that the extractor correctly handles constructors where statements can precede super() or this() calls.

Key changes:

  • Creates a new test directory structure for flexible constructor testing
  • Adds Java source code demonstrating various flexible constructor scenarios
  • Configures the test to use Java 25 with preview features enabled

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
java/ql/test/library-tests/flexible-constructors/options Configures extractor to use Java 25 with preview features
java/ql/test/library-tests/flexible-constructors/PrintAst.qlref References the AST printing query for test execution
java/ql/test/library-tests/flexible-constructors/PrintAst.expected Expected AST output for validation
java/ql/test/library-tests/flexible-constructors/FlexibleConstructors.java Test cases demonstrating flexible constructor syntax
java/ql/test/library-tests/flexible-constructors/CONSISTENCY/diags.expected Expected diagnostic output
Comments suppressed due to low confidence (1)

java/ql/test/library-tests/flexible-constructors/options:2

  • Java 25 does not exist as of my knowledge cutoff in January 2025. The latest stable Java version is Java 21 LTS, with Java 22 and 23 being non-LTS releases. Consider using an existing Java version or verify if this is intended for future testing.
//semmle-extractor-options: --javac-args --release 25 --enable-preview

@aschackmull
Copy link
Contributor

The test ought to include field initialization (using both implicit and explicit this) in the early construction context aka prologue.

@IdrissRio IdrissRio force-pushed the idrissrio/flexible-constructors branch from fbb688a to eb51682 Compare August 18, 2025 11:12
@@ -0,0 +1,2 @@
unexpectedDiagnostic
| <compilation flexible-constructors.testproj/trap/java/diagnostics/diagnostic.trap.gz> | -1 | 0 | file://:0:0:0:0 | Unknown location for jdk.internal.RequiresIdentity+Annotation | Unknown location for jdk.internal.RequiresIdentity+Annotation |
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This annotation is internally generated and has no specific source location. The issue is fixed in another PR (https://github.com/github/semmle-code/pull/53614/commits/ec9c0e744141ac02fbb1b62f3f5a4af2593168f1). The diags.expected can be removed once this PR is rebased on top of it.

aschackmull
aschackmull previously approved these changes Aug 26, 2025
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

I'm not quite sure what the unexpected diagnostic is about, otherwise LGTM.

@aschackmull aschackmull dismissed their stale review August 26, 2025 10:29

Forgot something.

@aschackmull
Copy link
Contributor

Got a bit hasty with the approval there. I'd like to verify that we're not breaking implicit initializer calls. So we should have some tests for that.
Details: Initializer expressions for fields are executed in a callable called <obinit>. In the extractor we insert calls to this method in any constructor that doesn't have a this(..) constructor call. Since this insertion might make some assumptions about this(..) being the first statement in the constructor body, if it exists, then I worry that flexible constructors might easily break this. So we should have some tests to cover this. You can copy the qltest java/ql/test/library-tests/prettyprint/pp.ql for this purpose as it will show you what has been extracted.

@IdrissRio
Copy link
Contributor Author

Got a bit hasty with the approval there. I'd like to verify that we're not breaking implicit initializer calls. So we should have some tests for that. Details: Initializer expressions for fields are executed in a callable called <obinit>. In the extractor we insert calls to this method in any constructor that doesn't have a this(..) constructor call. Since this insertion might make some assumptions about this(..) being the first statement in the constructor body, if it exists, then I worry that flexible constructors might easily break this. So we should have some tests to cover this. You can copy the qltest java/ql/test/library-tests/prettyprint/pp.ql for this purpose as it will show you what has been extracted.

Extremely good point. I'll add a test for this.

@IdrissRio IdrissRio force-pushed the idrissrio/flexible-constructors branch from ca31ff6 to 9cf867e Compare August 27, 2025 06:51
@IdrissRio IdrissRio requested a review from a team as a code owner August 27, 2025 06:51
@aschackmull
Copy link
Contributor

So I see that the object initializer call is still put after the super() call, and I'm guessing that this is indeed correct. The JEP is not exactly clear on that particular issue, though. Have you found either documentation for this or verified it through testing? I.e. what happens here?

class Sup {
  Sup() { pp(); }
  void pp() { }
}
class Foo extends Sup {
  String x = "<obinit> x";
  String y;
  Foo() {
    this.y = "prologue y";
    super();
  }
  void pp() {
    System.out.println(x + " " + y);
  }
}

I expect this will print "null prologue y", and this correspond with our placement of the <obinit> call according to the qltest, but it would be nice to verify.

@IdrissRio
Copy link
Contributor Author

Hi @aschackmull,
Yes, I ran a similar experiment. I started by first checking the JEP, but as you mentioned, there’s nothing there about initializers. So I relied on the latest definition in the [JLS](https://docs.oracle.com/javase/specs/jls/se24/html/jls-12.html#jls-12.5 + empirical experiments.
Just to confirm, in the snippet you posted we end up with x = null and y = prologue y.

I had to make some adjustments in the extractor for this (see the internal PR).

@IdrissRio IdrissRio assigned IdrissRio and unassigned IdrissRio Aug 27, 2025
@IdrissRio IdrissRio added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 27, 2025
@aschackmull
Copy link
Contributor

Just to confirm, in the snippet you posted we end up with x = null and y = prologue y

Excellent, thanks for confirming.

@@ -16,30 +16,6 @@ methodWithDuplicate
| AbstractCollection<E> | removeAll | Collection<?> |
| AbstractCollection<E> | retainAll | Collection<?> |
| AbstractCollection<E> | toArray | T[] |
| AbstractCollection<Entry<K,V>> | add | Entry<K,V> |
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with these kotlin test changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was related to the Java 25 upgrade. I had forgotten to update the merge target in the UI. I’ve fixed it now, and it no longer shows up.

@aschackmull
Copy link
Contributor

Quick question about the kotlin qltest changes, which appear unrelated to this PR. Otherwise LGTM now.

@IdrissRio IdrissRio changed the base branch from main to idrissrio/java-upgrade-fix August 27, 2025 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends on internal PR This PR should only be merged in sync with an internal Semmle PR Java Kotlin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants