Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions make/CompileJavaModules.gmk
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,20 @@ MODULESOURCEPATH := $(call GetModuleSrcPath)
# Add imported modules to the modulepath
MODULEPATH := $(call PathList, $(IMPORT_MODULES_CLASSES))

################################################################################
# Setup preprocessor flags
# The output directory must be present in GENERATED_PREVIEW_SUBDIRS in Modules.gmk.
# Temporarily restrict this to java.base, but it can be expanded later.

ifeq ($(MODULE), java.base)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is only ever for java.base, then consider putting the whole thing in make/modules/java.base/Java.gmk, along with the ToolsJdk import. If it's intended to be expanded to a limited set of modules, consider making the conditional on a suitably named variable that can be set from make/modules/<module>/Java.gmk to activate this feature. If it's intended to cover all modules eventually, then leaving it here makes sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Restricting it to java.base is because it's used for every module, there's a build failure for one of the modules.
Conceptually it should be applied to all modules which can participate in preview behaviour (which is not all modules, but is more than java.base).
I just wanted to set the pieces in the right place, even if I only enable it for java.base for now.

PREPROCESSOR_FLAGS := \
-Xlint:-removal -Xlint:-processing \
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is probably contentious, and should be temporary, but there's a bug in javac which this work uncovered, and enabling annotation processing confuses javac into giving out unexpected warnings.

See: https://bugs.openjdk.org/browse/JDK-8378740

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm absolutely fine with not submitting this PR until the warnings are sorted out (esp. for java.base).

-Avalueclasses.outdir=$(SUPPORT_OUTPUTDIR)/gensrc-valueclasses \
-processor build.tools.valueclasses.GenValueClasses

PROCESSOR_PATH += $(BUILDTOOLS_OUTPUTDIR)/jdk_tools_classes
endif

################################################################################
# Copy zh_HK properties files from zh_TW (needed by some modules)

Expand Down Expand Up @@ -120,9 +134,11 @@ $(eval $(call SetupJavaCompilation, $(MODULE), \
EXCLUDE_PATTERNS := -files, \
KEEP_ALL_TRANSLATIONS := $(KEEP_ALL_TRANSLATIONS), \
TARGET_RELEASE := $(TARGET_RELEASE), \
PROCESSOR_PATH := $(PROCESSOR_PATH), \
JAVAC_FLAGS := \
$(DOCLINT) \
$(JAVAC_FLAGS) \
$(PREPROCESSOR_FLAGS) \
--module-source-path $(MODULESOURCEPATH) \
--module-path $(MODULEPATH) \
--system none, \
Expand Down
7 changes: 6 additions & 1 deletion make/common/JavaCompilation.gmk
Original file line number Diff line number Diff line change
Expand Up @@ -302,13 +302,18 @@ define SetupJavaCompilationBody
# including the compilation output on the classpath, so that incremental
# compilations in unnamed module can refer to other classes from the same
# source root, which are not being recompiled in this compilation:
$1_AUGMENTED_CLASSPATH += $$(BUILDTOOLS_OUTPUTDIR)/depend $$($1_BIN)
$1_AUGMENTED_CLASSPATH += $$($1_BIN)
$1_PROCESSOR_PATH += $$(BUILDTOOLS_OUTPUTDIR)/depend
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why this change? Do you really need to mess with the API digest plugin? If this is needed, then please document any new parameters on the SetupJavaCompilation macro.

Copy link
Copy Markdown
Contributor Author

@david-beaumont david-beaumont Mar 6, 2026

Choose a reason for hiding this comment

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

This changed because the compiler is bimodal and either takes the processor path from "processor-path" or it takes it from the service provider info (which I don't think is the right approach here).
However this also is the plugin path.

So if you add --processor-path you break the depend plugin, because it used to use the classpath, but now that mechanism is disabled in favour of the processor path.

Thus it is necessary to also add the depend plugin path to the processor path.

This is achieving exactly the same thing as before and is an internal detail, and without it, you cannot specify an annotation processor path.

I'm happy to add documentation wherever you feel it necessary/helpful, please make suggestion as you see fit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a couple of lines of docs in JavaCompilation.gmk (please take a look) but also am now wondering about the naming inconsistency between "CLASSPATH" and "PROCESSOR_PATH". Do you want me to change it to "PROCESSORPATH" ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't have a concern for the name. IMO "CLASSPATH" is a special case for historical reasons, it's just common practice to write it as one word. This variable represents an option where the words are separated.

I have a minor concern about modifying the external parameter variable. That is something we try to avoid (though I'm sure there are other instances present if you look closely). It would be better if there was a separate internal variable that was constructed in the macro and then used.

endif

ifneq ($$($1_AUGMENTED_CLASSPATH), )
$1_FLAGS += -cp $$(call PathList, $$($1_AUGMENTED_CLASSPATH))
endif

ifneq ($$($1_PROCESSOR_PATH), )
$1_FLAGS += --processor-path $$(call PathList, $$($1_PROCESSOR_PATH))
endif

# Make sure the dirs exist, or that one of the EXTRA_FILES, that may not
# exist yet, is in it.
$$(foreach d, $$($1_SRC), \
Expand Down
286 changes: 286 additions & 0 deletions make/jdk/src/classes/build/tools/valueclasses/GenValueClasses.java
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OpenJDK has been moving away from getXXXX style names to just XXX, consider doing that here in new code.

Original file line number Diff line number Diff line change
@@ -0,0 +1,286 @@
/*
* Copyright (c) 2026, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

package build.tools.valueclasses;

import com.sun.source.tree.ClassTree;
import com.sun.source.tree.CompilationUnitTree;
import com.sun.source.util.TreePath;
import com.sun.source.util.Trees;

import javax.annotation.processing.AbstractProcessor;
import javax.annotation.processing.ProcessingEnvironment;
import javax.annotation.processing.RoundEnvironment;
import javax.annotation.processing.SupportedAnnotationTypes;
import javax.annotation.processing.SupportedOptions;
import javax.lang.model.SourceVersion;
import javax.lang.model.element.Element;
import javax.lang.model.element.TypeElement;
import javax.tools.Diagnostic;
import javax.tools.FileObject;
import java.io.File;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;
import java.io.Reader;
import java.io.Writer;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;

import static java.nio.file.StandardOpenOption.CREATE_NEW;
import static java.util.stream.Collectors.groupingBy;

/**
* Annotation processor for generating preview sources of classes annotated as
* value classes for preview mode.
*
* <p>Classes seen by this processor (annotated with {@code @MigratedValueClass}
* will have their source files re-written into the specified output directory
* for compilation as preview classes. Note that more than one class in a given
* source file may be annotated.
*
* <p>Class re-writing is achieved by injecting the "value" keyword in front of
* class declarations for all annotated elements in the original source file.
*
* <p>Note that there are two annotations in use for value classes, but since
* we must generate sources for abstract classes, we only process one of them.
* <ul>
* <li>{@code @jdk.internal.ValueBased} appears on concrete value classes.
* <li>{@code @jdk.internal.MigratedValueClass} appears on concrete and
* abstract value classes.
* </ul>
*/
@SupportedAnnotationTypes({"jdk.internal.MigratedValueClass"})
@SupportedOptions("valueclasses.outdir")
public final class GenValueClasses extends AbstractProcessor {
// Matches preprocessor option flag in CompileJavaModules.gmk.
private static final String OUTDIR_OPTION_KEY = "valueclasses.outdir";

private ProcessingEnvironment processingEnv = null;
private Path outDir = null;
private Trees trees = null;

@Override
public synchronized void init(ProcessingEnvironment processingEnv) {
super.init(processingEnv);
this.processingEnv = processingEnv;
String outDir = this.processingEnv.getOptions().get(OUTDIR_OPTION_KEY);
if (outDir == null) {
throw new IllegalStateException(
"Must specify -A" + OUTDIR_OPTION_KEY + "=<output-directory-path>"
+ " for annotation processor: " + GenValueClasses.class.getName());
}
this.outDir = Path.of(outDir.replace('/', File.separatorChar));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Separator replacement is not necessary, windows supports both.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting. Good to know, though there's lots of code which still does it (which is what I drew inspiration from).

this.trees = Trees.instance(this.processingEnv);
}

/**
* Override to return latest version, since the runtime in which this is
* compiled doesn't know about development source versions.
*/
@Override
public SourceVersion getSupportedSourceVersion() {
return SourceVersion.latest();
}

@Override
public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment env) {
// We don't have direct access to MigratedValueClass classes here.
Optional<? extends TypeElement> valueClassAnnotation =
getAnnotation(annotations, "jdk.internal.MigratedValueClass");
if (valueClassAnnotation.isPresent()) {
getAnnotatedTypes(env, valueClassAnnotation.get()).stream()
.collect(groupingBy(this::getJavaSourceFile))
.forEach(this::generateValueClassSource);
}
// We may not be the only annotation processor to consume this annotation.
return false;
}

/** Find the annotation element by name in the given set. */
private static Optional<? extends TypeElement> getAnnotation(Set<? extends TypeElement> annotations, String name) {
return annotations.stream()
.filter(e -> e.getQualifiedName().toString().equals(name))
.findFirst();
}

/** Find the type elements (classes) annotated with the given annotation element. */
private static Set<TypeElement> getAnnotatedTypes(RoundEnvironment env, TypeElement annotation) {
Set<TypeElement> types = new HashSet<>();
for (Element e : env.getElementsAnnotatedWith(annotation)) {
if (!e.getKind().isClass()) {
throw new IllegalStateException(
"Unexpected element kind (" + e.getKind() + ") for element: " + e);
}
TypeElement type = (TypeElement) e;
if (type.getQualifiedName().isEmpty()) {
throw new IllegalStateException(
"Unexpected empty name for element: " + e);
}
types.add(type);
}
return types;
}

/**
* Write a transformed version of the given Java source file with the
* {@code value} keyword inserted before the class declaration of each
* annotated type element.
*/
private void generateValueClassSource(Path srcPath, List<TypeElement> classes) {
try {
// We know there's at least one element per source file (by construction).
TypeElement element = classes.getFirst();
Path relPath = getModuleRelativePath(srcPath, getPackageName(element));
Path outPath = outDir.resolve(getModuleName(element)).resolve(relPath);
Files.createDirectories(outPath.getParent());

List<Long> insertPositions =
classes.stream().map(this::getValueKeywordInsertPosition).sorted().toList();

try (Reader reader = new InputStreamReader(Files.newInputStream(srcPath));
Writer output = new OutputStreamWriter(Files.newOutputStream(outPath, CREATE_NEW))) {
long curPos = 0;
for (long nxtPos : insertPositions) {
int nextChunkLen = Math.toIntExact(nxtPos - curPos);
long written = new LimitedReader(reader, nextChunkLen).transferTo(output);
if (written != nextChunkLen) {
throw new IOException("Unexpected number of characters transferred."
+ " Expected " + nextChunkLen + " but was " + written);
}
curPos = nxtPos;
// curPos is at the end of the modifier section, so add a leading space.
// curPos ---v
// [modifiers] class... -->> [modifiers] value class...
output.write(" value");
}
// Trailing section to end-of-file transferred from original reader.
reader.transferTo(output);
}
} catch (IOException e) {
throw new RuntimeException(e);
}
}

/**
* Returns the character offset in the original source file at which to insert
* the {@code value} keyword. The offset is the end of the modifiers section,
* which must immediately precede the class declaration.
*/
private long getValueKeywordInsertPosition(TypeElement classElement) {
TreePath classDecl = trees.getPath(classElement);
ClassTree classTree = (ClassTree) classDecl.getLeaf();
CompilationUnitTree compilationUnit = classDecl.getCompilationUnit();
// Since annotations are held as "modifiers", and since we only process
// elements with annotations, the positions of the modifiers section must
// be well-defined.
long pos = trees.getSourcePositions().getEndPosition(compilationUnit, classTree.getModifiers());
if (pos == Diagnostic.NOPOS) {
throw new IllegalStateException("Missing position information: " + classElement);
}
return pos;
}

private Path getModuleRelativePath(Path srcPath, String pkgName) {
Path relPath = Path.of(pkgName.replace('.', File.separatorChar)).resolve(srcPath.getFileName());
if (!srcPath.endsWith(relPath)) {
throw new IllegalStateException(String.format(
"Expected trailing path %s for source file %s", relPath, srcPath));
}
return relPath;
}

private String getModuleName(TypeElement t) {
return processingEnv.getElementUtils().getModuleOf(t).getQualifiedName().toString();
}

private String getPackageName(TypeElement t) {
return processingEnv.getElementUtils().getPackageOf(t).getQualifiedName().toString();
}

private Path getJavaSourceFile(TypeElement type) {
return getFilePath(processingEnv.getElementUtils().getFileObjectOf(type));
}

private static Path getFilePath(FileObject file) {
return Path.of(file.toUri());
}

/**
* A forwarding reader which guarantees to read no more than
* {@code maxCharCount} characters from the underlying stream.
*/
private static final class LimitedReader extends Reader {
// These are short-lived, no need to null the delegate when closed.
private final Reader delegate;
// This should never go negative.
private int remainingChars;

/**
* Creates a limited reader which reads up to {@code maxCharCount} chars
* from the given stream.
*
* @param delegate underlying reader
* @param maxCharCount maximum chars to read (can be 0)
*/
LimitedReader(Reader delegate, int maxCharCount) {
this.delegate = Objects.requireNonNull(delegate);
this.remainingChars = Math.max(maxCharCount, 0);
}

@Override
public int read(char[] cbuf, int off, int len) throws IOException {
if (remainingChars > 0) {
int readLimit = Math.min(remainingChars, len);
int count = delegate.read(cbuf, off, readLimit);
// Only update remainingChars if something was read.
if (count > 0) {
if (count > remainingChars) {
throw new IOException(
"Underlying Reader exceeded requested read limit." +
" Expected at most " + readLimit + " but read " + count);
}
remainingChars -= count;
}
// Can return 0 or -1 here (the underlying reader could finish first).
return count;
} else if (remainingChars == 0) {
return -1;
} else {
throw new AssertionError("Remaining character count should never be negative!");
}
}

@Override
public void close() {
// Do not close the delegate since this is conceptually just a view.
}
}
}
1 change: 0 additions & 1 deletion make/modules/java.base/Gensrc.gmk
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ include gensrc/GensrcMisc.gmk
include gensrc/GensrcModuleLoaderMap.gmk
include gensrc/GensrcRegex.gmk
include gensrc/GensrcScopedMemoryAccess.gmk
include gensrc/GensrcValueClasses.gmk
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's remove this gmk file too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wait, didn't I?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, you did in the recent update.

include gensrc/GensrcVarHandles.gmk

################################################################################
Expand Down