8272234: Pass originating elements from Filer to JavaFileManager #5076

lahodaj · 2021-08-10T20:46:32Z

This is a first prototype of a patch that propagates originating elements from Filer (createSourceFile/createClassFile/createResource) to the corresponding methods in JavaFileManager. As file managers generally don't know about Elements, the Elements are first converted to their corresponding FileObjects (if any). As the currently existing methods only take one FileObject as a sibling of the newly created file, a new set of methods is proposed that take multiple originating files.

Any feedback on this prototype would be welcome.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issues

JDK-8272234: Pass originating elements from Filer to JavaFileManager
JDK-8275508: Pass originating elements from Filer to JavaFileManager (CSR) ⚠️ Issue is not open.

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5076/head:pull/5076
$ git checkout pull/5076

Update a local copy of the PR:
$ git checkout pull/5076
$ git pull https://git.openjdk.java.net/jdk pull/5076/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5076

View PR using the GUI difftool:
$ git pr show -t 5076

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5076.diff

…mprovements to the test.

bridgekeeper · 2021-08-10T20:46:37Z

👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-08-10T20:50:15Z

@lahodaj The following label will be automatically applied to this pull request:

compiler

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.

mlbridge · 2021-08-10T20:54:43Z

Webrevs

bridgekeeper · 2021-09-07T23:13:56Z

@lahodaj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2021-10-06T02:46:40Z

@lahodaj This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

lahodaj · 2021-10-06T12:48:50Z

/open

openjdk · 2021-10-06T12:49:47Z

@lahodaj @HostUserDetails{id=51319204, username='lahodaj', fullName='null'} this pull request is now open

vicente-romero-oracle · 2021-10-15T20:02:54Z

I guess this one needs a CSR right?

jonathan-gibbons

Nice solution overall to an awkward problem, as regards naming.
Yes, the new name is awkward and long, but there does not seem to be a satisfactory alternative using overloading, and accepting that, there is a reasonable correspondence between the terms "originating elements" in Filer and "originating files" here.

jonathan-gibbons · 2021-10-15T21:58:47Z

src/java.compiler/share/classes/javax/tools/JavaFileManager.java

+     * given package-oriented location.
+     *
+     * <p>The provided {@code originatingFiles} represent files that
+     * where in, an unspecified way, used to create the content of


spelling: first word should probably be were, not where.

jonathan-gibbons · 2021-10-15T21:59:31Z

src/java.compiler/share/classes/javax/tools/JavaFileManager.java

+     * @throws IllegalStateException {@link #close} has been called
+     * and this file manager cannot be reopened
+     * @since 18
+     * @see Filer#createSourceFile


👍 yes, cross-linking to Filer is a good idea.

jonathan-gibbons · 2021-10-15T22:00:05Z

src/java.compiler/share/classes/javax/tools/JavaFileManager.java

+     * name</a> in the specified package in the given location.
+     *
+     * <p>The provided {@code originatingFiles} represent files that
+     * where in, an unspecified way, used to create the content of


spelling: were

jonathan-gibbons · 2021-10-15T22:05:06Z

src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacFiler.java

+        JavaFileObject[] originatingFiles = Arrays.asList(originatingElements)
+                .stream()
+                .filter(el -> el instanceof Symbol)
+                .map(el -> (Symbol) el)
+                .map(sym -> switch (sym.kind) {
+                    case PCK -> {
+                        if (((PackageSymbol) sym).package_info == null) {
+                            yield null;
+                        }
+                        yield ((PackageSymbol) sym).package_info.classfile;
+                    }
+                    case MDL -> {
+                        ModuleSymbol msym = (ModuleSymbol) sym;
+                        yield msym.module_info.classfile != null ? msym.module_info.classfile
+                                                                 : msym.module_info.sourcefile;
+                    }
+                    default -> sym.outermostClass().classfile;
+                })
+                .filter(fo -> fo != null)
+                .toArray(s -> new JavaFileObject[s]);


@jddarcy this is an example of why it would be good to have an API method in the Language Model world to get the "source" from which an Element was obtained, where "source" means either a .class or a .java file ... in other words, the Symbol.classfile field accessed in this code.

Acknowledged; it would be preferable if this mapping could be done in the standardized API.

openjdk · 2021-10-19T04:58:28Z

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@lahodaj please create a CSR request for issue JDK-8272234. This pull request cannot be integrated until the CSR request is approved.

jddarcy · 2021-10-19T04:58:58Z

I guess this one needs a CSR right?

A CSR is needed and I'd happily advance a Proposed request to Provisional in its current form.

…rdingJavaFileManager

vicente-romero-oracle · 2021-10-19T19:12:59Z

I made some minor edits to the CSR and added myself as reviewer

bridgekeeper · 2021-11-16T23:09:12Z

@lahodaj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

…elements-forwarding-tweak

openjdk · 2021-11-22T10:10:15Z

⚠️ @lahodaj This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

jddarcy · 2021-11-23T05:33:53Z

src/java.compiler/share/classes/javax/tools/JavaFileManager.java

+    default JavaFileObject getJavaFileForOutputForOriginatingFiles(Location location,
+                                        String className,
+                                        Kind kind,
+                                        FileObject... originatingFiles)


Perhaps explicitly state that null originatingFile or empty originatingFiles are no-ops.

Thanks - how about the following?
7c11f10

Spec update looks fine.

jonathan-gibbons · 2021-11-23T20:11:22Z

src/java.compiler/share/classes/javax/tools/JavaFileManager.java

+     * given package-oriented location.
+     *
+     * <p>The provided {@code originatingFiles} represent files that
+     * were in, an unspecified way, used to create the content of


misplaced comma. should be were, in an unspecified way,

jonathan-gibbons · 2021-11-23T20:12:45Z

src/java.compiler/share/classes/javax/tools/JavaFileManager.java

+     * @param location a package-oriented location
+     * @param className the name of a class
+     * @param kind the kind of file, must be one of {@link
+     * JavaFileObject.Kind#SOURCE SOURCE} or {@link
+     * JavaFileObject.Kind#CLASS CLASS}
+     * @param originatingFiles the files which are contributing to this newly created file;
+     *                         {@code null} is equivalent to empty {@code originatingFiles},
+     *                         meaning no known originating files


(minor/white-space) inconsistent indentation of @param tags

jonathan-gibbons · 2021-11-23T20:17:45Z

src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacFiler.java

+        JavaFileObject[] originatingFiles = Arrays.asList(originatingElements)
+                .stream()


Why not start with Stream.of(originatingElements) ?

jonathan-gibbons · 2021-11-23T20:19:04Z

test/langtools/tools/javac/processing/filer/TestOriginatingElements.java

+ * questions.
+ */
+
+/**


Should be /* not /**. This is not a documentation comment for javadoc.

jonathan-gibbons · 2021-11-23T20:22:01Z

test/langtools/tools/javac/processing/filer/TestOriginatingElements.java

+                }
+                List<String> options = List.of("-sourcepath", src.toString(),
+                                               "-processor", "TestOriginatingElements$P",
+                                               "-processorpath", System.getProperty("test.classes"),


This may be enough. but note there is also test.class.path .... one is just the output directory, the other is the full path.

jonathan-gibbons · 2021-11-23T20:23:12Z

test/langtools/tools/javac/processing/filer/TestOriginatingElements.java

+                ModuleElement mdl = processingEnv.getElementUtils().getModuleElement("m");
+                ModuleElement java_base = processingEnv.getElementUtils().getModuleElement("java.base");
+                PackageElement pack = processingEnv.getElementUtils().getPackageElement("p");
+                PackageElement lib1Pack = processingEnv.getElementUtils().getPackageElement("lib1");
+                PackageElement lib2Pack = processingEnv.getElementUtils().getPackageElement("lib2");


strongly suggest using a local variable for processingEnv.getElementUtils()

jonathan-gibbons

The recent commits all look good.

lahodaj · 2021-12-03T07:38:52Z

/integrate

openjdk · 2021-12-03T07:40:52Z

Going to push as commit 8907003.
Since your change was applied there have been 101 commits pushed to the master branch:

0fbd271: 8278115: gc/stress/gclocker/TestGCLockerWithSerial.java has duplicate -Xmx
bef5c7a: 8278116: runtime/modules/LoadUnloadModuleStress.java has duplicate -Xmx
473ecec: 8278174: runtime/cds/appcds/LambdaWithJavaAgent.java fails with release VMs
19ce33d: 8258512: serviceability/sa/TestJmapCore.java timed out on macOS 10.13.6
c93552c: 8277069: [REDO] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
b8ac0d2: 8276681: Additional malformed Javadoc inline tags in JDK source
b310f30: 8277396: [TESTBUG] In DefaultButtonModelCrashTest.java, frame is accessed from main thread
15345e3: 8276661: (fs) UserDefinedFileAttributeView no longer works with long path (win)
652b5f8: 8276674: Malformed Javadoc inline tags in JDK source
e0f1fc7: 8277358: Accelerate CRC32-C
... and 91 more: https://git.openjdk.java.net/jdk/compare/040b2c52d3e82048630fbd45a7db48a5e65204b7...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-03T07:41:08Z

@lahodaj Pushed as commit 8907003.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

lahodaj added 13 commits February 26, 2021 14:24

Pass originating elements to JavaFileManager.

b296646

Passing only originatingFiles to the JavaFileManager.

0de307e

Updating the names of the methods taking originating files; general i…

6743b5c

…mprovements to the test.

Fixing the TestClientCodeWrapper.java test,

ff6d6f6

Merge branch 'master' into ap-keep-originating-elements

4568d46

Improving the patch and API.

74ea1a9

Adding @SInCE

985cf87

Attempting to fix test.

ac11a34

More attempts to fix the test.

d2b5b17

Another attempt to fix the test.

761e82e

Merge branch 'master' into ap-keep-originating-elements

78d4df5

Updating @SInCE

9e99803

Improving testing

aa61e59

openjdk bot added the rfr Pull request is ready for review label Aug 10, 2021

openjdk bot added the compiler compiler-dev@openjdk.org label Aug 10, 2021

Cleanup.

c451208

bridgekeeper bot closed this Oct 6, 2021

openjdk bot reopened this Oct 6, 2021

lahodaj mentioned this pull request Oct 6, 2021

8274817: Allow JavacFiler subclasses and register them in Context #5840

Closed

3 tasks

lahodaj added 2 commits October 15, 2021 10:39

Merge branch 'master' into ap-keep-originating-elements

8ddc86c

Javadoc improvements.

9396bbb

jonathan-gibbons approved these changes Oct 15, 2021

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Oct 19, 2021

lahodaj added 3 commits October 19, 2021 11:23

Attempting to improve compatibility with existing subclasses of Forwa…

39793a3

…rdingJavaFileManager

Fixing javadoc.

f871a30

Fixing typo, as per review comments.

aa31c17

lahodaj added 2 commits October 20, 2021 15:21

Fixing test (need to exclude a private method from the test).

04562f2

Cleanup.

d959129

lahodaj added 4 commits November 22, 2021 08:24

Merge branch 'master' into ap-keep-originating-elements

2c3a8ab

Cleanup

bf2f3a4

Merge branch 'ap-keep-originating-elements' into ap-keep-originating-…

fd2d8c0

…elements-forwarding-tweak

Test cleanup.

da4781a

jddarcy reviewed Nov 23, 2021

View reviewed changes

Improving javadoc as suggested in the PR.

7c11f10

jonathan-gibbons reviewed Nov 23, 2021

View reviewed changes

lahodaj added 2 commits November 26, 2021 10:33

Applying changes as suggested on the review.

31d8162

Merge branch 'master' into ap-keep-originating-elements

e191d93

jonathan-gibbons approved these changes Dec 1, 2021

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Dec 1, 2021

openjdk bot closed this Dec 3, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 3, 2021

8272234: Pass originating elements from Filer to JavaFileManager #5076

8272234: Pass originating elements from Filer to JavaFileManager #5076

lahodaj commented Aug 10, 2021 •

edited by openjdk bot

bridgekeeper bot commented Aug 10, 2021

openjdk bot commented Aug 10, 2021

mlbridge bot commented Aug 10, 2021 •

edited

bridgekeeper bot commented Sep 7, 2021

bridgekeeper bot commented Oct 6, 2021

lahodaj commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

vicente-romero-oracle commented Oct 15, 2021

jonathan-gibbons left a comment

jonathan-gibbons Oct 15, 2021 •

edited

jonathan-gibbons Oct 15, 2021

jonathan-gibbons Oct 15, 2021

jonathan-gibbons Oct 15, 2021

jddarcy Oct 19, 2021

openjdk bot commented Oct 19, 2021

jddarcy commented Oct 19, 2021

vicente-romero-oracle commented Oct 19, 2021

bridgekeeper bot commented Nov 16, 2021

openjdk bot commented Nov 22, 2021

jddarcy Nov 23, 2021

lahodaj Nov 23, 2021

jddarcy Nov 24, 2021

jonathan-gibbons Nov 23, 2021

jonathan-gibbons Nov 23, 2021

jonathan-gibbons Nov 23, 2021

jonathan-gibbons Nov 23, 2021

jonathan-gibbons Nov 23, 2021

jonathan-gibbons Nov 23, 2021

jonathan-gibbons left a comment

lahodaj commented Dec 3, 2021

openjdk bot commented Dec 3, 2021

openjdk bot commented Dec 3, 2021

		JavaFileObject[] originatingFiles = Arrays.asList(originatingElements)
		.stream()

8272234: Pass originating elements from Filer to JavaFileManager #5076

8272234: Pass originating elements from Filer to JavaFileManager #5076

Conversation

lahodaj commented Aug 10, 2021 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Aug 10, 2021

openjdk bot commented Aug 10, 2021

mlbridge bot commented Aug 10, 2021 • edited

Webrevs

bridgekeeper bot commented Sep 7, 2021

bridgekeeper bot commented Oct 6, 2021

lahodaj commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

vicente-romero-oracle commented Oct 15, 2021

jonathan-gibbons left a comment

Choose a reason for hiding this comment

jonathan-gibbons Oct 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Oct 19, 2021

jddarcy commented Oct 19, 2021

vicente-romero-oracle commented Oct 19, 2021

bridgekeeper bot commented Nov 16, 2021

openjdk bot commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-gibbons left a comment

Choose a reason for hiding this comment

lahodaj commented Dec 3, 2021

openjdk bot commented Dec 3, 2021

openjdk bot commented Dec 3, 2021

lahodaj commented Aug 10, 2021 •

edited by openjdk bot

mlbridge bot commented Aug 10, 2021 •

edited

jonathan-gibbons Oct 15, 2021 •

edited