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