Skip to content

[Java] Add stubbing script#5943

Merged
smowton merged 23 commits intogithub:mainfrom
joefarebrother:java-stub
Aug 11, 2021
Merged

[Java] Add stubbing script#5943
smowton merged 23 commits intogithub:mainfrom
joefarebrother:java-stub

Conversation

@joefarebrother
Copy link
Contributor

@joefarebrother joefarebrother commented May 21, 2021

A QL-based tool for generating Java stubs, to replace the current ad-hoc internal tool. Based on the C# version.

Questions and limitations:

  • Can the interface be improved? Currently you need to have two files in the test directory, options0 and options1, which we probably don't want to commit.
  • Should we also generate a license comment for the generated stubs, or does it suffice to have a LICENSE.txt file alongside them? (not generated by this tool)
  • There's not currently a way to specify that stubs should be output across multiple directories, to support stubs for tests that have multiple dependencies
  • Annotations are currently stripped from the generated stubs, making it not possible to test queries that depend on them (e.g. "Every method annotated with a particular annotation is a source")

@github-actions github-actions bot added the Java label May 21, 2021
import Stubs

/** Declarations used by source code. */
class UsedInSource extends GeneratedDeclaration {
Copy link
Contributor

Choose a reason for hiding this comment

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

In C#, we're moving away from this minimal stub approach, which relies on the test input. Instead we're going to generate complete stubs of nuget packages. The stubbing API doesn't change, we're just using a different configuration: https://github.com/github/codeql/pull/5664/files#diff-fe12d2e3e080fe834b3b602ced8689b223d6785ee52c12b12268b413713e348a

Also, generating stubs for packages without extra filtering on types might help identify constructs that are not handled by the current stubbing implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

we're going to generate complete stubs of [...] packages

Definitely worth considering in the future, but this is currently not supported by the Java extractor, so it seems out of scope for this pull request.

@joefarebrother joefarebrother marked this pull request as ready for review June 3, 2021 15:36
@joefarebrother joefarebrother requested a review from a team as a code owner June 3, 2021 15:36
@joefarebrother joefarebrother added the no-change-note-required This PR does not need a change note label Jun 22, 2021
@sauyon
Copy link
Contributor

sauyon commented Jun 28, 2021

  • Can the interface be improved? Currently you need to have two files in the test directory, options0 and options1, which we probably don't want to commit.

Since the auto test generator uses a pom.xml, perhaps this could too? The output directory on the command line could be used to pretty easily generate the options1, too.

@joefarebrother
Copy link
Contributor Author

Since the auto test generator uses a pom.xml, perhaps this could too? The output directory on the command line could be used to pretty easily generate the options1, too.

Good suggestion; I'll look into it

@joefarebrother joefarebrother force-pushed the java-stub branch 2 times, most recently from 9d1b566 to fc141c5 Compare June 30, 2021 15:42

/** Exclude types from the standard library. */
private class DefaultLibs extends ExcludedPackage {
DefaultLibs() { this.getName().matches(["java.%", "javax.%", "jdk.%", "sun.%"]) }
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we need stubs for javax packages? (At least javax.servlet seemed to need stubbing).

Suggested change
DefaultLibs() { this.getName().matches(["java.%", "javax.%", "jdk.%", "sun.%"]) }
DefaultLibs() { this.getName().matches(["java.%", "jdk.%", "sun.%"]) }

@sauyon
Copy link
Contributor

sauyon commented Jul 22, 2021

(apologies, I didn't realize gh pr checkout set the upstream to the branch automatically...)

@smowton smowton merged commit 7a27043 into github:main Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants