8348427: DeferredLintHandler API should use JCTree instead of DiagnosticPosition#23281
8348427: DeferredLintHandler API should use JCTree instead of DiagnosticPosition#23281archiecobbs wants to merge 7 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back acobbs! A progress list of the required criteria for merging this PR into |
|
@archiecobbs This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 22 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@archiecobbs The following label will be automatically applied to this pull request:
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. |
| loggers.append(logger); | ||
| } | ||
| public void push(JCTree decl) { | ||
| Assert.check(decl.getTag() == Tag.MODULEDEF |
|
Question: looking more broadly at the deferred lint handler API, I see that in most cases (all but one) Is the above code correct? E.g. should the call on |
| } | ||
| loggers.append(logger); | ||
| } | ||
| public void push(JCTree decl) { |
There was a problem hiding this comment.
I wonder if enterDecl/leaveDecl would be more evocative than push/pop. (but don't have any suggestion for the weird immediate mode).
|
I have some doubts about the general deferred lint handler API - but my doubts are completely unrelated to your PR which, I think, does a nice job of making the API a little more tight, and easier to use for clients. Good job! |
|
(My doubts re DeferredLintHandler are whether it's a good abstraction or not. It seems to provide, on paper, a good service: allow for lint warnings to be correctly reported in earlier phases of the compiler. However, I note that:
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). E.g. up to Flow, we keep accumulating lint warnings grouped by declaration. Then, in a single pass we take care of updating Lint with the correct |
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 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: So I don't know what's going on there exactly and have so far avoided that particular dark corner.
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,
I have that same feeling :) We have the new 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 come after #23237):
What do you think? One thing I'm unclear of is whether |
This seems like a great plan. We don't have to get there in one step (and this PR can proceed separately). I just want to make sure that we end up somewhere around there :-) (as your quick experiment with Check suggests, some of this stuff is not trivial at all -- even the use of the immediate mode seems a bit odd to be honest).
I believe that in |
Sounds good. I created JDK-8348611 to capture the idea.
Thanks - that makes life simpler. |
|
|
||
| import java.util.ArrayDeque; | ||
| import java.util.ArrayList; | ||
| import java.util.Comparator; |
There was a problem hiding this comment.
Are these imports all used?
mcimadamore
left a comment
mcimadamore
left a comment
There was a problem hiding this comment.
Question: what happens if we get rid of the "immediate" and we always defer? Does this only affect the order in which diagnostics are reported, or is there something deeper?
I learn a little bit more about this. It looks like the only time immediate mode is used explicitly (i.e., other due to It looks like the other apparent use (here) never occurs because
I did some more exploration of this idea. The main trip-up at this point is the speculative compilation stuff, because in that case warnings that occur as a side effect should be discarded because they're not "real". This is done via |
Can we use a similar "flush" strategy ? E.g. make the warnings be part of an import declaration, and then process them as part of a flush? While it's good that you knocked one of the two usages off -- it's a bit sad to have this machinery "just" for one case. Separately, I wonder if warnings issued from import statements should be suppressible as well (perhaps piggy backing on the toplevel class |
mcimadamore
left a comment
mcimadamore
left a comment
There was a problem hiding this comment.
Regardless of the separate discussion re. "pushImmediate", this PR contains a lot of good changes, and I'm ok with it being integrated as is. We can see if we can get rid of the "immediate" state in a separate PR.
I think it would be useful to understand which warnings can be generated during See related change: https://hg.openjdk.org/jdk9/jdk9/langtools/rev/82384454947c Where we explicitly disable deprecation warnings on import since 9. Is there any other? |
If this was indeed the only possible case, there is a possible silver lining, because either warnings in imports are enabled (e.g. But I'm sure reality is more complex than that :-) |
|
Hi @mcimadamore, Thanks for the review & comments!
Agreed - it seems to be a result of mixing the ordering of attributing things and flushing the import java.security.PolicySpi;
public class A {
@SuppressWarnings("deprecation")
private PolicySpi foo;
@SuppressWarnings("deprecation")
public void m() throws Exception {
A.class.newInstance();
}
}If you comment out the one remaining "When to flush" is indeed a key issue for other reasons too. For example, it appears that
I ran across this as well and accommodated it my prototype by effectively pretending there is a
It would be nice if the compiler allowed |
|
/integrate |
|
Going to push as commit ba28119.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@archiecobbs Pushed as commit ba28119. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
2 participants
The purpose of
DeferredLintHandleris to allow@SuppressWarningsto be applied to warnings that are generated before@SuppressWarningsannotations themselves have been processed. The way this currently works is that warning callbacks are kept in aHashMapkeyed by the innermost containing module, package, class, method, or variable declarations (in the form of aJCModuleDecl,JCPackageDecl,JCClassDecl,JCMethodDecl, orJCVariableDecl). Later, when the compiler executes the attribution phase and the lint categories suppressed at each declaration are known, the corresponding warning callbacks are provided with an appropriately configuredLintinstance and "flushed".However, the
DeferredLintHandlerAPI usesDiagnosticPositioninstead ofJCTreefor registering and flushing deferred warnings. This opens the door for bugs where warnings are registered to an object which is not a declaration, and therefore ignored.In fact, this occurs once in the code (here) where a
JCExpressionis being passed, although in this case the bug appears to be harmless (because annotation values can't contain any type of declaration).The API should be tighted up, and furthermore an assertion should be added to verify that the JCTree being passed is actually a declaration supporting
@SuppressWarnings.In addition, there is a design flaw in the API: it's not possible to obtain the current immediate mode
Lintobject, so if an immediate modeLintobject is pushed/popped more than once, the secondLintobject will overwrite the first.To fix this, the API should be adjusted so the stack of current declarations and/or immediate mode
Lintobjects is managed by theDeferredLintHandleritself, by providing apush()andpop()methods in the API.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23281/head:pull/23281$ git checkout pull/23281Update a local copy of the PR:
$ git checkout pull/23281$ git pull https://git.openjdk.org/jdk.git pull/23281/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23281View PR using the GUI difftool:
$ git pr show -t 23281Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23281.diff
Using Webrev
Link to Webrev Comment