Skip to content

Conversation

IdrissRio
Copy link
Contributor

@IdrissRio IdrissRio commented Jul 21, 2025

Support for Java 25 module import declarations:

  • Updated database schema to handle module import declarations
  • Added ModuleImportDeclaration class
  • Added tests to validate the new functionality
  • Added Change Note

See internal PR.

@IdrissRio IdrissRio changed the title Idrissrio/module import declarations Java: Add support to ModuleImportDeclaration Jul 21, 2025
@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch 3 times, most recently from 16b0695 to a16147c Compare July 22, 2025 10:44
@IdrissRio IdrissRio added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Jul 22, 2025
@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch 3 times, most recently from a24e4ae to 27cb373 Compare July 28, 2025 12:19
}
}

// Diagnostic Matches: Unknown location for jdk.internal.RequiresIdentity+Annotation No newline at end of file
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 is an internally generated annotation without a specific source location.
Excluding it from diagnostic will be addressed in a separate PR.

@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch from 27cb373 to e4848e0 Compare July 30, 2025 06:28
@IdrissRio IdrissRio marked this pull request as ready for review July 30, 2025 06:39
@Copilot Copilot AI review requested due to automatic review settings July 30, 2025 06:39
@IdrissRio IdrissRio requested a review from a team as a code owner July 30, 2025 06:39
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 support for Java 25 module import declarations, implementing the new import module syntax feature. The implementation includes database schema updates, a new ModuleImportDeclaration class, and comprehensive test coverage.

Key changes:

  • Database schema updated to handle the new module import kind (value 6 in imports table)
  • Added ModuleImportDeclaration class extending Import with methods to access module information and imported types/packages
  • Upgrade/downgrade scripts for database compatibility

Reviewed Changes

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

Show a summary per file
File Description
java/ql/lib/semmle/code/java/Import.qll Adds ModuleImportDeclaration class with methods to get module info and imported types/packages
java/ql/lib/config/semmlecode.dbscheme Updates imports table schema to include module import kind (6) with documentation
java/ql/lib/upgrades/.../semmlecode.dbscheme New schema version adding MODULEIMPORT kind to imports documentation
java/ql/lib/upgrades/.../old.dbscheme Previous schema version for upgrade compatibility
java/ql/lib/upgrades/.../upgrade.properties Upgrade script properties for module import declarations
java/downgrades/.../semmlecode.dbscheme Downgrade schema removing module import support
java/downgrades/.../upgrade.properties Downgrade script properties
java/ql/test/library-tests/module-import-declarations/* Test files validating module import functionality and expected outputs
java/ql/lib/change-notes/2025-07-21-module-import-declarations.md Release note documenting the new feature

@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch 3 times, most recently from e4848e0 to 3c0b807 Compare August 19, 2025 10:43
@IdrissRio IdrissRio requested a review from a team as a code owner August 19, 2025 10:43
@IdrissRio IdrissRio changed the base branch from main to idrissrio/java-upgrade-fix August 19, 2025 12:03
@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch from 3c0b807 to ccf5002 Compare August 21, 2025 08:18
@github-actions github-actions bot removed the Kotlin label Aug 21, 2025
@IdrissRio IdrissRio force-pushed the idrissrio/java-upgrade-fix branch from ba93ced to 564364b Compare August 25, 2025 09:34
@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch 2 times, most recently from 0877afe to 75107ba Compare August 25, 2025 12:15
@IdrissRio IdrissRio force-pushed the idrissrio/java-upgrade-fix branch 2 times, most recently from a34b362 to 5d2268f Compare September 2, 2025 18:19
Base automatically changed from idrissrio/java-upgrade-fix to main September 4, 2025 14:46
@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch from 75107ba to 8656461 Compare September 5, 2025 06:07
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

I can only review this at a very shallow level. Someone from @github/codeql-java should probably have a look too.

* A module import declaration, which imports an entire module.
*
* For example, `import module java.base;` imports all packages exported
* by the `java.base module`, making their types available for use.
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this maybe supposed to be:

Suggested change
* by the `java.base module`, making their types available for use.
* by the `java.base` module, making their types available for use.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment here mentions "types", but below I get a getAnImportedPackage. So what is actually being imported here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. The declaration makes the exported packages available, and through them the types defined in those packages become accessible for use. I'll rephrase the comment.

@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch 2 times, most recently from 27e7c17 to 6d8a014 Compare September 5, 2025 13:40
Comment on lines 184 to 187
exists(Package pkg |
pkg = this.getAnImportedPackage() and
result.getPackage() = pkg
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be more concise and still understandable.

Suggested change
exists(Package pkg |
pkg = this.getAnImportedPackage() and
result.getPackage() = pkg
)
result.getPackage() = this.getAnImportedPackage()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change applied, tested, and committed. Thanks 👍

@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch from 0083d5a to d5c5531 Compare September 5, 2025 17:47
Copy link
Contributor

@owen-mc owen-mc 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 particularly familiar with this layer of the java library, but it looks pretty inoffensive to me.

@IdrissRio IdrissRio force-pushed the idrissrio/module-import-declarations branch from d5c5531 to ed9ed43 Compare September 6, 2025 10:39
@IdrissRio IdrissRio merged commit c5cb86a into main Sep 6, 2025
15 of 17 checks passed
@IdrissRio IdrissRio deleted the idrissrio/module-import-declarations branch September 6, 2025 11:11
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 documentation Java
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants