-
Notifications
You must be signed in to change notification settings - Fork 5.8k
8331051: Add an @since
checker test for java.base
module
#18934
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
8331051: Add an @since
checker test for java.base
module
#18934
Conversation
👋 Welcome back nizarbenalla! A progress list of the required criteria for merging this PR into |
@nizarbenalla This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 189 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@nizarbenalla The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Various comments inline. Overall, a great start.
Consider adding more explanatory comments for the bigger items, like classes and long/important methods. Imagine a future-person looking over your shoulder, asking what the more important items do or are for.
)); | ||
} | ||
|
||
private final class EffectiveSourceSinceHelper implements AutoCloseable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider putting a doc comment describing the purpose of this class. The name hints at something useful, but is not specific enough by itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it, thanks
fm.close(); | ||
} | ||
|
||
private static final class PatchModuleFileManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provide a short doc comment describing the purpose/use of this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it, thanks
try { | ||
fm.close(); | ||
} catch (IOException closeEx) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using ex.addSuppressed(closeEx);
instead of just dropping the exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 32edb4d
public String introducedStable; | ||
} | ||
|
||
//these were preview in before the introduction of the @PreviewFeature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: where do you find this information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used a script to get all the elements with @Deprecated(forRemoval=true, since=...)
, @jdk.internal.preview
, {@preview Associated with ....
in their javadoc.
And then had to cross reference with a document that had all the ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update
- added more detail to the initial comment - ordered imports correctly - renamed methods to be more consistent - added predicate methods and 'isDocumented' 'isMember' - added doc comments describing purpose of 'EffectiveSourceSinceHelper'
Webrevs
|
If not already planned, I recommend a much broader discussion/announcement of this test before it goes back. For example, a message to jdk-dev stating "The value of |
- This checker checks the values of the `@since` tag found in the documentation comment for an element against the release in which the element first appeared. | ||
- The source code containing the documentation comments is read from `src.zip` in the release of JDK used to run the test. | ||
- The releases used to determine the expected value of `@since` tags are taken from the historical data built into `javac`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Minor, suggest wrapping lines somewhere between 80-100 characters long)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was that, even when comparing files in a merge conflit, there wouldn't be problems here.
But I will fix, some lines in the code are a bit long as well
processModuleCheck(elements.getModuleElement(moduleName), ct, packagePath, javadocHelper); | ||
} catch (Exception e) { | ||
e.printStackTrace(); | ||
error("Initiating javadocHelperFailed " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want just e
, not e.getMessage()
because e
(actually e.toString()
) will include the name of the exception as well, whereas e.getMessage()
does not.)
URI uri = URI.create("jar:" + srcZip.toUri()); | ||
try (FileSystem zipFO = FileSystems.newFileSystem(uri, Collections.emptyMap())) { | ||
Path root = zipFO.getRootDirectories().iterator().next(); | ||
Path packagePath = root.resolve(moduleName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, and in uses of this name, including in method signatures, it is strange to see the variable named packagePath
since it appears to be the path of the unnamed package in the module.
modulePath
would likely be a better name, unless you are worried about possible confusion with the --module-path
option, in which case maybe moduleDir
or moduleDirectory
would work.
} | ||
} | ||
|
||
private void processModuleCheck(ModuleElement moduleElement, JavacTask ct, Path packagePath, EffectiveSourceSinceHelper javadocHelper) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment line 223, about use of packagePath
} | ||
} | ||
|
||
private String getModuleVersionFromFile(Path packagePath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment line 223, about packagePath
return version; | ||
} | ||
|
||
private String getPackageVersionFromFile(Path packagePath, ModuleElement.ExportsDirective ed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment line 223, about packagePath
public String introducedStable; | ||
} | ||
|
||
//these were preview in before the introduction of the @PreviewFeature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update
test/jdk/TEST.groups
Outdated
# Set of tests for `@since` checks in source code documentation | ||
jdk_since_check = \ | ||
tools/sincechecker/testjavabase/CheckSince_javaBase.java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, the symbol of a red line in a red circle is indicative of a missing final newline at the end of the file.
It is recommend that files end with exactly one newline character.
note on rules for Real and effective `@since: | ||
|
||
Real since value of an API element is computed as the oldest release in which the given API element was introduced. That is: | ||
- for modules, classes and interfaces, the release in which the element with the given qualified name was introduced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about packages? packages should be in this list as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small oversight, will fix
Path pkgInfo = packagePath.resolve(ed.getPackage() | ||
.getQualifiedName() | ||
.toString() | ||
.replaceAll("\\.", "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for readers: I will change this tomorrow to be more platform agnostic using File.separatorChar
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with .replace(".", File.separator)
|
||
|
||
/* | ||
- This checker checks the values of the `@since` tag found in the documentation comment for an element against the release in which the element first appeared. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the long lines mentioned by Jon which make the comments hard to read, I find it strange that every sentence is formatted as a list item with a leading dash. I think it's ok when describing different parts/steps of the algorithm, but at least the first sentence in the comment should not be a list item IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
|
||
public static void main(String[] args) throws Exception { | ||
if (args.length == 0) { | ||
throw new SkippedException("Test module not specified"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think SkippedException
is the right exception to throw here, since invoking the method with a missing argument is probably a configuration error that shouldn't be ignored. Maybe IllegalArgumentException
or just RuntimeException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it, now throwing IllegalArgumentException
.
.forEach(nestedClass -> processClassElement(nestedClass, version, types, elements)); | ||
} | ||
|
||
public void persistElement(Element explicitOwner, Element element, Types types, String version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure persistElement
is a good name for this, it sounds like the element is being stored permanently (beyond the runtime of this program). I can't think of a really good name, but I think processElement
would be ok.
Path testJdk = Paths.get(System.getProperty("test.jdk")); | ||
srcZip = testJdk.getParent().resolve("images").resolve("jdk").resolve("lib").resolve("src.zip"); | ||
} | ||
if (!Files.exists(srcZip) && !Files.isDirectory(srcZip)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition looks wrong. If exists
returns false, it also implies that isDirectory
returns false. I think there's no need to check for a (not-)directory here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3f226ef, I now only check if the file is readable.
-Changes `e.getMessages` to `e` -`File.separator` to be platform indepedant -Added a newline character at the end of files -Rename `persistElement` to `processElement` -Now throwing IllegalArgumentException instead of SkippedException
… (they were incubating) - Not checking elements enclosed withing a record - Only check if the file is readable using `Files.isReadable` - Dropped the use of `Files.exists` and `Files.isDirectory` - Use `--add-modules` option now to resolve certain modules
…ass will change after being created - Added a null check as `toString` can return an exception
I performed a merge and a tiny change to the error message by the test. Here is an example of the error output of the tool
|
|
||
String version = String.valueOf(i); | ||
Elements elements = ct.getElements(); | ||
elements.getModuleElement("java.base"); // forces module graph to be instantiated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lahodaj I've always considered this kind of line to be a workaround; it would be nice if we could have an RFE so this kind of line was unnecessary in code that wants to introspect the elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've filled:
https://bugs.openjdk.org/browse/JDK-8341342
version = effectiveSinceVersion(element.getEnclosingElement().getEnclosingElement(), element.getEnclosingElement(), types, elementUtils); | ||
} | ||
if (version == null) { | ||
//may be null for private elements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty statement for if
...
import com.sun.tools.javac.util.Pair; | ||
import jtreg.SkippedException; | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally very good. It's getting to be a really nice piece of functionality.
I added one suggestion for a minor improvement, about setting up the `EXCLUDE_LIST in a more module-specific way. The intent is that if we need to make module-specific changes to the exclude list in times going forward, we should do that in the module-specific test invocation, and not in the main module-independent code.
private static final Set<String> EXCLUDE_LIST = Set.of( | ||
"java.lang.classfile" | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not really be here, in the general code for a SinceChecker for all modules.
It would be better if this info was provided in command-line options, given when the test is run from the module-specific test (i.e. CheckSince_javaBase.java
).
This means you will have to write some additional simple option decoding in the main
method, starting line 21.
* @modules jdk.compiler/com.sun.tools.javac.api | ||
* jdk.compiler/com.sun.tools.javac.util | ||
* jdk.compiler/com.sun.tools.javac.code | ||
* @run main SinceChecker java.base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment about init-ing the EXCLUDE_LIST in the main SinceChecker, at about line 112.
You are already passing a module name as an option to the main test. Maybe the command here on line 33 could be something like
@run main SinceChecker java.base --exclude java.lang.classfile
where the argument after --exclude
is (logically) a comma-separated list of items to exclude. You could also do a whitespace-separated list up to the next option (i.e. item beginning with -
) or end of the list. In other words make it future-proof against adding more options in future.
For discussion: I see that in GitHub Actions, the test passes on all platforms. (That's good!). But it's not clear to me that it needs to run on all platforms. Generally, the API should be the same on all platforms, right (After all, we only generate one set of API documentation.) So, the question becomes, can a test determine if it is being run in a multi-platform context? We presumably don't want to stop the test running whenever a user runs it locally on their platform of choice, but equally, if the test is being run on many platforms, that's an unnecessary use/waste of resources. But, I guess we run a lot of tests that are essentially platform-independent on all platforms, although I guess there're are also testing the product running on top of the JVM, whereas here. we're primarily just checking the source. Anyway, this is really just an abstract academic discussion, not a blocker in any way for the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good to me.
Can we maybe set the reviewer count to 1? This needs an additional review and the last change was small. |
/reviewers 1 |
Thank you for the reviews; all. And the help to getting this work integrated. /integrate |
Going to push as commit c81aa75.
Your commit was automatically rebased without conflicts. |
@nizarbenalla Pushed as commit c81aa75. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This checker checks the values of the
@since
tag found in the documentation comment for an element against the release in which the element first appeared.Real since value of an API element is computed as the oldest release in which the given API element was introduced. That is:
Effective since value of an API element is computed as follows:
@since
tag in its javadoc, it is usedThe since checker verifies that for every API element, the real since value and the effective since value are the same, and reports an error if they are not.
Preview method are handled as per JEP 12, if
@PreviewFeature
is used consistently going forward then the checker doesn't need to be updated with every release. The checker has explicit knowledge of preview elements that came beforeJDK 14
because they weren't marked in a machine understandable way and preview elements that came beforeJDK 17
that didn't have@PreviewFeature
.Important note : We only check code written since
JDK 9
as the releases used to determine the expected value of@since
tags are taken from the historical data built intojavac
which only goes back that farThe intial comment at the beginning of
SinceChecker.java
holds more information into the program.I already have filed issues and fixed some wrong tags like in #18640, #18032, #18030, #18055, #18373, #18954, #18972.
Progress
Issue
@<!---->since
checker test forjava.base
module (Sub-task - P4)Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18934/head:pull/18934
$ git checkout pull/18934
Update a local copy of the PR:
$ git checkout pull/18934
$ git pull https://git.openjdk.org/jdk.git pull/18934/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18934
View PR using the GUI difftool:
$ git pr show -t 18934
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18934.diff
Webrev
Link to Webrev Comment