RFR: 8348427: DeferredLintHandler API should use JCTree instead of DiagnosticPosition

Archie Cobbs acobbs at openjdk.org
Fri Jan 24 18:20:46 UTC 2025


On Fri, 24 Jan 2025 11:05:15 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> Is the above code correct? E.g. should the call on `lint.logIfEnabled` occur on the `lint` that is the parameter to the lambda expression?

I wondered the same thing, and it seems plainly counter to the whole point of warning deferral. I haven't tried to figure it out yet - I assume it must work because when the lambda executes, the correct `Lint` object happens to still be in place.

But you got me curious... my theory is easy to test with this patch:

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
index fe40bf0dad1..3a23c5df6d6 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
@@ -244,7 +244,7 @@ MethodSymbol setMethod(MethodSymbol newMethod) {
      *  @param pos        Position to be used for error reporting.
      *  @param sym        The deprecated symbol.
      */
-    void warnDeprecated(DiagnosticPosition pos, Symbol sym) {
+    void warnDeprecated(Lint lint, DiagnosticPosition pos, Symbol sym) {
         if (sym.isDeprecatedForRemoval()) {
             if (!lint.isSuppressed(LintCategory.REMOVAL)) {
                 if (sym.kind == MDL) {
@@ -284,7 +284,7 @@ public void warnDeclaredUsingPreview(DiagnosticPosition pos, Symbol sym) {
      *  @param pos        Position to be used for error reporting.
      *  @param msg        A Warning describing the problem.
      */
-    public void warnRestrictedAPI(DiagnosticPosition pos, Symbol sym) {
+    public void warnRestrictedAPI(Lint lint, DiagnosticPosition pos, Symbol sym) {
         lint.logIfEnabled(log, pos, LintWarnings.RestrictedMethod(sym.enclClass(), sym));
     }
 
@@ -648,8 +648,8 @@ public void checkRedundantCast(Env<AttrContext> env, final JCTypeCast tree) {
                 && types.isSameType(tree.expr.type, tree.clazz.type)
                 && !(ignoreAnnotatedCasts && TreeInfo.containsTypeAnnotation(tree.clazz))
                 && !is292targetTypeCast(tree)) {
-            deferredLintHandler.report(_l -> {
-                lint.logIfEnabled(log, tree.pos(), LintWarnings.RedundantCast(tree.clazz.type));
+            deferredLintHandler.report(lint2 -> {
+                lint2.logIfEnabled(log, tree.pos(), LintWarnings.RedundantCast(tree.clazz.type));
             });
         }
     }
@@ -1324,7 +1324,7 @@ && checkDisjoint(pos, flags,
     private void warnOnExplicitStrictfp(JCTree tree) {
         deferredLintHandler.push(tree);
         try {
-            deferredLintHandler.report(_ -> lint.logIfEnabled(log, tree.pos(), LintWarnings.Strictfp));
+            deferredLintHandler.report(lint2 -> lint2.logIfEnabled(log, tree.pos(), LintWarnings.Strictfp));
         } finally {
             deferredLintHandler.pop();
         }
@@ -3785,7 +3785,7 @@ void checkDeprecated(Supplier<DiagnosticPosition> pos, final Symbol other, final
                 || s.isDeprecated() && !other.isDeprecated())
                 && (s.outermostClass() != other.outermostClass() || s.outermostClass() == null)
                 && s.kind != Kind.PCK) {
-            deferredLintHandler.report(_l -> warnDeprecated(pos.get(), s));
+            deferredLintHandler.report(lint2 -> warnDeprecated(lint2, pos.get(), s));
         }
     }
 
@@ -3850,7 +3850,7 @@ void checkPreview(DiagnosticPosition pos, Symbol other, Type site, Symbol s) {
 
     void checkRestricted(DiagnosticPosition pos, Symbol s) {
         if (s.kind == MTH && (s.flags() & RESTRICTED) != 0) {
-            deferredLintHandler.report(_l -> warnRestrictedAPI(pos, s));
+            deferredLintHandler.report(lint2 -> warnRestrictedAPI(lint2, pos, s));
         }
     }
 
@@ -4122,7 +4122,7 @@ void checkDivZero(final DiagnosticPosition pos, Symbol operator, Type operand) {
             int opc = ((OperatorSymbol)operator).opcode;
             if (opc == ByteCodes.idiv || opc == ByteCodes.imod
                 || opc == ByteCodes.ldiv || opc == ByteCodes.lmod) {
-                deferredLintHandler.report(_ -> lint.logIfEnabled(log, pos, LintWarnings.DivZero));
+                deferredLintHandler.report(lint2 -> lint2.logIfEnabled(log, pos, LintWarnings.DivZero));
             }
         }
     }
@@ -4135,8 +4135,8 @@ void checkDivZero(final DiagnosticPosition pos, Symbol operator, Type operand) {
      */
     void checkLossOfPrecision(final DiagnosticPosition pos, Type found, Type req) {
         if (found.isNumeric() && req.isNumeric() && !types.isAssignable(found, req)) {
-            deferredLintHandler.report(_ ->
-                lint.logIfEnabled(log, pos, LintWarnings.PossibleLossOfPrecision(found, req)));
+            deferredLintHandler.report(lint2 ->
+                lint2.logIfEnabled(log, pos, LintWarnings.PossibleLossOfPrecision(found, req)));
         }
     }
 
@@ -4335,8 +4335,8 @@ void checkDefaultConstructor(ClassSymbol c, DiagnosticPosition pos) {
                             // Warning may be suppressed by
                             // annotations; check again for being
                             // enabled in the deferred context.
-                            deferredLintHandler.report(_ ->
-                                lint.logIfEnabled(log, pos, LintWarnings.MissingExplicitCtor(c, pkg, modle)));
+                            deferredLintHandler.report(lint2 ->
+                                lint2.logIfEnabled(log, pos, LintWarnings.MissingExplicitCtor(c, pkg, modle)));
                         } else {
                             return;
                         }
@@ -4670,26 +4670,26 @@ private void checkVisible(DiagnosticPosition pos, Symbol what, PackageSymbol inP
 
     void checkModuleExists(final DiagnosticPosition pos, ModuleSymbol msym) {
         if (msym.kind != MDL) {
-            deferredLintHandler.report(_ ->
-                lint.logIfEnabled(log, pos, LintWarnings.ModuleNotFound(msym)));
+            deferredLintHandler.report(lint2 ->
+                lint2.logIfEnabled(log, pos, LintWarnings.ModuleNotFound(msym)));
         }
     }
 
     void checkPackageExistsForOpens(final DiagnosticPosition pos, PackageSymbol packge) {
         if (packge.members().isEmpty() &&
             ((packge.flags() & Flags.HAS_RESOURCE) == 0)) {
-            deferredLintHandler.report(_ ->
-                lint.logIfEnabled(log, pos, LintWarnings.PackageEmptyOrNotFound(packge)));
+            deferredLintHandler.report(lint2 ->
+                lint2.logIfEnabled(log, pos, LintWarnings.PackageEmptyOrNotFound(packge)));
         }
     }
 
     void checkModuleRequires(final DiagnosticPosition pos, final RequiresDirective rd) {
         if ((rd.module.flags() & Flags.AUTOMATIC_MODULE) != 0) {
-            deferredLintHandler.report(_ -> {
-                if (rd.isTransitive() && lint.isEnabled(LintCategory.REQUIRES_TRANSITIVE_AUTOMATIC)) {
+            deferredLintHandler.report(lint2 -> {
+                if (rd.isTransitive() && lint2.isEnabled(LintCategory.REQUIRES_TRANSITIVE_AUTOMATIC)) {
                     log.warning(pos, LintWarnings.RequiresTransitiveAutomatic);
                 } else {
-                    lint.logIfEnabled(log, pos, LintWarnings.RequiresAutomatic);
+                    lint2.logIfEnabled(log, pos, LintWarnings.RequiresAutomatic);
                 }
             });
         }

Turns out it's not quite that simple; the above patch breaks the build:

Building target 'test' in configuration 'macosx-aarch64-server-release'
Compiling up to 360 files for BUILD_jdk.compiler.interim
Updating support/src.zip
Compiling up to 368 files for jdk.compiler
/Users/archie/proj/jdk/src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java:142: warning: [deprecation] URL(URL,String) in URL has been deprecated
        URL retVal = new URL(base, input);
                     ^
error: warnings found and -Werror specified
/Users/archie/proj/jdk/src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/MemoryContext.java:269: warning: [restricted] Controller.enableNativeAccess(Module) is a restricted method.
            controller.enableNativeAccess(module);
                      ^
  (Restricted methods are unsafe and, if used incorrectly, might crash the Java runtime or corrupt memory)
1 error
2 warnings
make[3]: *** [/Users/archie/proj/jdk/build/macosx-aarch64-server-release/jdk/modules/jdk.compiler/_the.jdk.compiler_batch] Error 1
make[2]: *** [jdk.compiler-java] Error 2

So I don't know what's going on there exactly and have so far avoided that particular dark corner.

> I wonder if `enterDecl`/`leaveDecl` would be more evocative than push/pop. (but don't have any suggestion for the weird immediate mode).

Yes that naming would be better - if there were no immediate mode. But since immediate mode exists, the stack contains more than just declarations. Also, `push()` hopefully makes it more obvious that you are responsible putting in place a corresponding `pop()`.

> My general feeling here is that we'd be better off with a separate compilation step that runs after all the various front-end steps (e.g. after Flow).

I have that same feeling :) We have the new `warn()` compiler phase, so that would be the logical place to put the grand `flush()` operation.

I think this is do-able. Semantically things would get simpler, which means while the patch would be large, it would consist mostly of decomplexification. Rough sketch (this would include #23237):

* Move the deferral logic of `DeferredLintManager` into `Lint`; the `DeferredLintManager` singleton goes away.
* Lint has a single `flush()` method which is invoked _once_ per source file during the compiler `warn()` step
  * First, lexical deferrals are remapped to innermost containing `JCTree` 
  * Next, all warnings are emitted (in lexical order) by scanning the `JCCompilationUnit` and flushing each declaration
* Lint warnings are created via `Lint.logIfEnabled()` (or similar) at any time during compilation
  * If you can locate the warning with a `JCTree`, you supply that
  * Otherwise, you locate the warning with an integer `pos` source file offset
  * In either case, an assertion verifies that `flush()` has not yet been invoked

What do you think?

One thing I'm unclear of is whether `flush()` can assume that all of the accumulated warnings correspond to the same file. Might not be true with `CompilePolicy.BY_TODO`?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/23281#issuecomment-2613128868


More information about the compiler-dev mailing list