8362885: A more formal way to mark javac's Flags that belong to a specific Symbol type only #26452

lahodaj · 2025-07-24T07:21:12Z

This PR proposes to improve handling of javac's Flags in two ways:

for each flag, there's now an informational annotation specifying what is the target Symbol type. Only targets right now are TypeSymbols, MethodSymbols and VarSymbols. If we ran out of flags for TypeSymbols, we could split those to module/package/class/type variable, but it does not seem to be quite necessary yet. There's an auxiliary special BLOCK, which is for JCBlock.
the manually handled Flags.Flag enum is replaced with autogenerated FlagsEnum

There may be some better to handle Flags eventually, but this hopefully improves the current situation at least somewhat, by providing more formal way to say the flags' target, and restricting the need to read comments and search for free flags.

As a side-effect of this annotation, the test/langtools/tools/javac/flags/FlagsTest.java now also prints which flags are free, for each Symbol type.

(I will remove the build label for now, until discussion on javac level is done, and will re-add it if we decide the goal to autogenerate the FlagsEnum makes sense.)

/label remove build

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8362885: A more formal way to mark javac's Flags that belong to a specific Symbol type only (Task - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26452/head:pull/26452
$ git checkout pull/26452

Update a local copy of the PR:
$ git checkout pull/26452
$ git pull https://git.openjdk.org/jdk.git pull/26452/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26452

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26452.diff

Using Webrev

Link to Webrev Comment

…cific Symbol type only

bridgekeeper · 2025-07-24T07:21:56Z

👋 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 · 2025-07-24T07:23:05Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-07-24T07:23:43Z

@lahodaj The build label was not set.

openjdk · 2025-07-24T07:23:56Z

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

build
compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

lahodaj · 2025-07-24T07:24:00Z

/label remove build

openjdk · 2025-07-24T07:25:21Z

@lahodaj
The build label was successfully removed.

mlbridge · 2025-07-24T07:28:58Z

Webrevs

01: Full - Incremental (e00209f0)
00: Full (0ab2ba48)

archiecobbs · 2025-07-24T15:25:33Z

This looks like a really nice improvement for maintainability.

? Would it make more sense (and/or be simpler?) to move the conflict-checking logic into FlagsGenerator?

It somehow seems odd to have a tool which can knowingly generate an invalid FlagsEnum.java file... and then defer the check for that until later, maybe, in a unit test... when we could just make the build fail instead.

? Is it worth adding some simple runtime check for correct FlagTarget usage?

For example (just brainstorming):

For each FlagTarget have FlagsGenerator output e.g. public static final long METHOD_FLAGS = 0x0301930420520017L;
At appropriate location(s) in the code, add Assert.check(this.flags_field & ~METHOD_FLAGS) == 0);

(Of course even better would be to check this at compile time using stronger typing (e.g., by replacing long flags_field with some EnumSet, etc.) but that's probably way beyond this first step.)

liach · 2025-07-24T18:12:05Z

test/langtools/tools/javac/flags/FlagsTest.java

+
+    private static void printFreeFlags(String comment, long freeFlags) {
+            System.err.print("free flags for " + comment + ": ");
+            for (int bit = 16; bit < 64; bit++) { //lowest 16 bits are used in classfiles, never suggest adding anything there


Replace 16 with Character.SIZE and 64 with Long.SIZE?

I don't think Character.SIZE is really appropriate, but I've declared U2_SIZE for 16, and used Long.SIZE for 64.

liach · 2025-07-24T18:15:04Z

make/langtools/tools/flagsgenerator/FlagsGenerator.java

+
+            try (PrintWriter out = new PrintWriter(Files.newBufferedWriter(Paths.get(args[1])))) {
+                out.println("""
+                            package com.sun.tools.javac.code;


Do we need a license header for generated sources?

CompilerProperties and other files that are generated using similar means don't have license headers, so I assume we don't need that.

jdlib · 2025-07-24T18:45:55Z

test/langtools/tools/javac/flags/FlagsTest.java

+    }
+
+    private static void verifyFlagsNonOverlapping() throws Throwable {
+        Map<FlagTarget, Map<Long, List<Field>>> target2Flag2Fields = computeTarget2Flag2Fields();


You could calculate this once in main and pass to verifyFlagNonOverlapping and findFreeFlags

Given this is a test, I would prefer to keep the test cases separate. This is a bit moot now, as the test is no doing the verification anymore.

- when conflict is detected, the generator fails - adding runtime checks - using constants for number of bits

mcimadamore · 2025-07-25T09:20:20Z

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Flags.java

    public static final int StandardFlags = 0x0fff;

    // Because the following access flags are overloaded with other
    // bit positions, we translate them when reading and writing class
    // files into unique bits positions: ACC_SYNTHETIC <-> SYNTHETIC,
    // for example.
+    @NotFlag
    public static final int ACC_SUPER    = 0x0020;


Why didn't we add annotation on these ACC ones? After all, they also overlap with previous flags -- e.g. ACC_SUPER overlaps with SYNCHRONIZED, and it works because the former goes on types and the latter on methods. So, isn't this the same trick we're pulling?

…plit of the TYPE target into CLASS/MODULE/PACKAGE/TYPE_VARIABLE.

mcimadamore · 2025-07-25T13:42:53Z

make/langtools/tools/flagsgenerator/FlagsGenerator.java

@@ -167,8 +168,11 @@ private static Object valueOfValueAttribute(AnnotationMirror am) {

    public enum FlagTarget {


Just flyby comment: symbol kind is an enum, so in principle we could use constants such as MTH, VAR, TYP as targets, if we wanted to connect the decls more with the actual code. But I like your approach as well (as symbol kind has also a lot of other more dubious stuff/noise)

lahodaj · 2025-07-25T13:46:00Z

This looks like a really nice improvement for maintainability.

? Would it make more sense (and/or be simpler?) to move the conflict-checking logic into FlagsGenerator?

It somehow seems odd to have a tool which can knowingly generate an invalid FlagsEnum.java file... and then defer the check for that until later, maybe, in a unit test... when we could just make the build fail instead.

Done.

? Is it worth adding some simple runtime check for correct FlagTarget usage?

For example (just brainstorming):
* For each `FlagTarget` have `FlagsGenerator` output e.g. `public static final long METHOD_FLAGS = 0x0301930420520017L;`

* At appropriate location(s) in the code, add `Assert.check(this.flags_field & ~METHOD_FLAGS) == 0);`

I've added the checks. It may be useful, but it is somewhat limited, as it cannot detect the most dangerous case where the flag with overloaded meaning is set incorrectly.

(Of course even better would be to check this at compile time using stronger typing (e.g., by replacing long flags_field with some EnumSet, etc.) but that's probably way beyond this first step.)

There were several experiments, in the past and currently ongoing, but it is difficult to do something that is safer and does not cost too much on performance/memory consumption. Value classes may or may not help.

mcimadamore · 2025-07-25T13:48:33Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

@@ -2127,6 +2127,9 @@ private boolean needPackageInfoClass(JCPackageDecl pd) {
    }

    public void visitModuleDef(JCModuleDecl tree) {
+        FlagsEnum.assertNoUnexpectedFlags(tree.sym.flags_field,


Not super sure about rolling this into Lower... but a separate visitor will also look a bit "too much"...

For instance, for type variables there's no way to check them at this point?

mcimadamore · 2025-07-25T14:38:18Z

There's a lot to like in this patch. Now flags cannot erroneously overlap, which will give us all javac developers more confidence that we're not stepping into each other toes. Also, by making the mechanism more formally correct, it's easier to see which "holes" we have available (and there's probably quite a lot left).

The only thing I'm unsure of, as pointed out in the review, is the dynamic checks. I believe that, as far as this PR is concerned, this feels like "scope creep". I think it would be cleaner if this PR only concerned about introducing the machinery. Then maybe we can brainstorm separately on what would be the best way to add some of these checks (but I'm skeptical that we can find anything that doesn't feel like a compromise).

For instance, another way to do this could be to do another combo test like the tree test, where we compile all the existing tests, and check the validity of all the flags on all the symbols. That's an offline way to make sure that what we do in javac is "sort of" correct, but one that doesn't leak into the code.

The real fix for this, as we know (and as @archiecobbs commented) is to move away from longs, but that's not what this PR is about.

lahodaj added 2 commits July 23, 2025 12:07

8362885: A more formal way to mark javac's Flags that belong to a spe…

9c7007f

…cific Symbol type only

Fixing tests.

0ab2ba4

openjdk bot added the rfr Pull request is ready for review label Jul 24, 2025

openjdk bot added build build-dev@openjdk.org compiler compiler-dev@openjdk.org labels Jul 24, 2025

openjdk bot removed the build build-dev@openjdk.org label Jul 24, 2025

liach reviewed Jul 24, 2025

View reviewed changes

jdlib reviewed Jul 24, 2025

View reviewed changes

Reflecting review feedback:

5bc1d6d

- when conflict is detected, the generator fails - adding runtime checks - using constants for number of bits

mcimadamore reviewed Jul 25, 2025

View reviewed changes

lahodaj added 2 commits July 25, 2025 12:14

Review feedback: marking ACC flags with the @use annotation, forces s…

b0fbd91

…plit of the TYPE target into CLASS/MODULE/PACKAGE/TYPE_VARIABLE.

Fixing toString value for flags that have ACC_ overloaded flags.

e00209f

mcimadamore reviewed Jul 25, 2025

View reviewed changes

		@@ -167,8 +168,11 @@ private static Object valueOfValueAttribute(AnnotationMirror am) {

		public enum FlagTarget {

8362885: A more formal way to mark javac's Flags that belong to a specific Symbol type only #26452

Are you sure you want to change the base?

8362885: A more formal way to mark javac's Flags that belong to a specific Symbol type only #26452

Uh oh!

Conversation

lahodaj commented Jul 24, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Jul 24, 2025

Uh oh!

openjdk bot commented Jul 24, 2025

Uh oh!

openjdk bot commented Jul 24, 2025

Uh oh!

openjdk bot commented Jul 24, 2025

Uh oh!

lahodaj commented Jul 24, 2025

Uh oh!

openjdk bot commented Jul 24, 2025

Uh oh!

mlbridge bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

archiecobbs commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore commented Jul 25, 2025

Uh oh!

Uh oh!

lahodaj commented Jul 24, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 24, 2025 •

edited

Loading