8348611: Eliminate DeferredLintHandler and emit warnings after attribution #24584
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 107 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 this pull request can not be integrated into git checkout JDK-8348611
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
|
openjdk
bot
added
the
merge-conflict
|
Hi Jan, thanks for taking a look.
Right - and to elaborate on this a bit: We added the new
Ah, I didn't realize that. In any case it should have the same semantics as before, because in both cases (before and after this change), the
If you throw an |
| private static final Context.Key<LintMapper> CONTEXT_KEY = new Context.Key<>(); | ||
|
|
||
| // Per-source file information. Note: during the parsing of a file, an entry exists but the FileInfo value is null | ||
| private final Map<JavaFileObject, FileInfo> fileInfoMap = new HashMap<>(); |
There was a problem hiding this comment.
would it makes sense to make this map a WeakHashMap? I'm thinking about cases where we are not using a ReusableContext and thus this map wont be cleared.
There was a problem hiding this comment.
would it makes sense to make this map a WeakHashMap? I'm thinking about cases where we are not using a ReusableContext and thus this map wont be cleared.
Frankly I'm a little fuzzy on that. If there were an issue with LintMapper.fileInfoMap, then wouldn't the same issue exist with AbstractLog.sourceMap? Note LintMapper.clear() and Log.clear() are called at the same time, so it seems in any given scenario, they are going to either both be cleared or both not be cleared.
There was a problem hiding this comment.
yes you are right we have other DSs with the same issue if any. I guess discuss and address it as a separate issue
There was a problem hiding this comment.
yes you are right we have other DSs with the same
issueif any. I guess discuss and address it as a separate issue
I'm not familiar with all of the various compiler life-cycle variants, but I guess it would boil down to the question, "In what scenarios would a Context that's not a ReusableContext be reused for multiple compilations?"
There was a problem hiding this comment.
FWIW, this is an instance field on a Context service - it will be GCed when the given javac instance is GCed. I think in most cases, use of weak maps is not necessary.
There was a problem hiding this comment.
I was worried about holding unnecessary memory withing one javac instance
| private record LintRange( | ||
| Span span, // declaration's lexical range | ||
| Lint lint, // the Lint configuration that applies at this declaration | ||
| Symbol symbol, // declaration symbol (for debug purposes only; null for root) |
There was a problem hiding this comment.
nit: should we keep this field in the final version? we can add it while debugging if needed
There was a problem hiding this comment.
nit: should we keep this field in the final version? we can add it while debugging if needed
I can remove it. Besides, this symbol is always the same object as the one that the Lint instance was created with, so if debugging were a concern, a better place to store it would be with the Lint instance anyway.
| Span span, // declaration's lexical range | ||
| Lint lint, // the Lint configuration that applies at this declaration | ||
| Symbol symbol, // declaration symbol (for debug purposes only; null for root) | ||
| List<LintRange> children // the nested declarations one level below this node |
There was a problem hiding this comment.
not a proposal to do any change, but I think that if instead of having a tree-like structure in which each range has, potentially, a number of children, we had one list of ranges sorted by its starting point. I think that we could find the right range doing a binary search. We could actually have both: the current tree and a flat list of lint ranges for searches. Again probably not necessary for general cases but there could be corner cases as in machine generated code for which having a faster retrieval could make the difference. But again we can do that if needed in the future
There was a problem hiding this comment.
I think at one point I had something like that, but it made things more complicated. Plain binary search doesn't work when you are searching in a list of (possibly nested) intervals instead of a list of points, because there can be multiple matches (arbitrarily many in fact). I think it would require an Interval tree.
There is also another annoying detail, which is that the nesting is not always proper: in some oddball cases (none of which I can specifically remember) there can be an AST node whose starting position is before the starting position of its parent AST node (or ending position after its parent). These instances could cause an Interval tree type of algorithm to glitch out.
There was a problem hiding this comment.
I see, interesting that you explored those options. Yes I also saw the interval tree solution but that seemed like an overkill, at least as an starting point.
|
@vicente-romero-oracle thanks for the re-review. @lahodaj @mcimadamore please re-review after 1f1247b whenever convenient. Thanks! |
mcimadamore
left a comment
mcimadamore
left a comment
There was a problem hiding this comment.
Still looks good to me :-)
|
/integrate |
|
Going to push as commit 3e60ab5.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@archiecobbs Pushed as commit 3e60ab5. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
Thanks @archiecobbs for all your hard work on this issue. I'm glad to see this integrated! |
|
@mcimadamore thanks very much for your reviews and helpful discussions. "Cleanups" like this are sometimes tedious to review so I appreciate your time on it! |
|
This PR fixes an issue that impacts JDK 25. See openrewrite/rewrite-templating#165 for details. Can/will it be backported for inclusion in JDK 25.0.1? (Asking here, as I don't have an account at https://bugs.openjdk.org/. Let me know if there's a better/preferred channel.) |
|
I took at look at the openrewrite issue but it's not specific on what the actual bug is. Also, this PR was mostly a refactoring so that doesn't clarify it either. Do you have a small reproducer test case that demonstrates the bug? |
|
Hey @archiecobbs! I used the reproducer in openrewrite/rewrite-templating#165 to The actual bug is described here: "somehow" when OpenRewrite's annotation processor is active, I'm currently working on a much smaller reproduction case; I'll post here once done. |
|
Alright, run the following script in an empty directory to create a #!/usr/bin/env bash
set -euo pipefail
# Creates files in the current directory to reproduce the issue.
# This script writes `pom.xml` and `src/main/java/com/example/Reproducer.java`.
mkdir -p src/main/java/com/example
cat > pom.xml <<'POM_EOF'
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>com.example</groupId>
<artifactId>reproducer</artifactId>
<version>0.0.1-SNAPSHOT</version>
<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
</properties>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_core</artifactId>
<version>2.42.0</version>
</dependency>
<dependency>
<groupId>javax.annotation</groupId>
<artifactId>javax.annotation-api</artifactId>
<version>1.3.2</version>
</dependency>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-bom</artifactId>
<version>3.27.4</version>
<type>pom</type>
<scope>import</scope>
</dependency>
<dependency>
<groupId>org.openrewrite.recipe</groupId>
<artifactId>rewrite-recipe-bom</artifactId>
<version>3.14.0</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>
<dependencies>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_core</artifactId>
</dependency>
<dependency>
<groupId>javax.annotation</groupId>
<artifactId>javax.annotation-api</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
</dependency>
<dependency>
<groupId>org.openrewrite</groupId>
<artifactId>rewrite-java</artifactId>
</dependency>
<dependency>
<groupId>org.openrewrite</groupId>
<artifactId>rewrite-templating</artifactId>
</dependency>
</dependencies>
<build>
<pluginManagement>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.14.0</version>
<configuration>
<compilerArgs>
<arg>-Xlint:all</arg>
</compilerArgs>
<parameters>true</parameters>
<release>17</release>
<showWarnings>true</showWarnings>
<failOnWarning>true</failOnWarning>
</configuration>
</plugin>
</plugins>
</pluginManagement>
</build>
<profiles>
<profile>
<id>break-it</id>
<build>
<pluginManagement>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<annotationProcessorPaths>
<path>
<groupId>org.openrewrite</groupId>
<artifactId>rewrite-templating</artifactId>
</path>
</annotationProcessorPaths>
</configuration>
</plugin>
</plugins>
</pluginManagement>
</build>
</profile>
</profiles>
</project>
POM_EOF
cat > src/main/java/com/example/Reproducer.java <<'JAVA_EOF'
package com.example;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import org.assertj.core.api.AbstractThrowableAssert;
final class Reproducer {
// XXX: This rule changes the `Throwable` against which subsequent assertions are made.
static final class AbstractThrowableAssertCauseIsSameAs {
@BeforeTemplate
@SuppressWarnings("deprecation" /* This deprecated API will be rewritten. */)
AbstractThrowableAssert<?, ? extends Throwable> before(
AbstractThrowableAssert<?, ? extends Throwable> throwableAssert, Throwable expected) {
return throwableAssert.hasCauseReference(expected);
}
@AfterTemplate
AbstractThrowableAssert<?, ? extends Throwable> after(
AbstractThrowableAssert<?, ? extends Throwable> throwableAssert, Throwable expected) {
return throwableAssert.cause().isSameAs(expected);
}
}
}
JAVA_EOFWith JDK 25, the build passes when running With JDK 24 and below (and the latest JDK 26 EA build) the build correctly passes with both commands. |
|
Hi @Stephan202, On the other hand, if you can reproduce the problem using only code that depends on supported APIs, then that would indeed be a bug and we'll track it down. That certainly could also be the case here (I haven't tried to determine that). Thanks. |
|
I am afraid I need to be even more strict: internal "APIs" are not supported, and are not kept stable. Any use of such "APIs" is not recommended, and it is solely up to the user to handle any updates/changes. My wild guess here is that openrewrite tries to suppress diagnostics (so that it can attribute the template without producing anything), but that fails for some reason (although I admit I don't immediately see the reason). It is hard to say, as the code simply ignores all exceptions: Also, for completeness, code like this: |
5 participants
This is a cleanup/refactoring of how lint warnings are logged and
@SuppressWarningsannotations applied.A central challenge with lint warnings is that warnings can be generated during any compiler phase, but whether a particular lint warning is suppressed via
@SuppressWarningscan't be known until after attribution. For example, the parser doesn't have enough information to interpret and apply@SuppressWarnings("text-blocks")to text blocks, or@SuppressWarnings("preview")to preview lexical features; instead, theDeferredLintHandleris used to workaround this limitation.In addition, several other factors complicate things:
Lintinstance requires manually tracking it with each declaration visited and applying/removing the@SuppressWarningsannotation there, if any@SuppressWarnings, while others are only suppressible via-Xlint:-fooflagsDeferredLintHandlerbecause even after speculation is complete, we may still not yet know whether a warning should be suppressed.Previously the logic to get all of this right was non-obviously woven around the code base. In particular, you needed to know somehow whether or not to use
DeferredLintHandler, and in what "mode".The overall goal of this PR is to simplify usage so that no matter where you are in the compiler, you can just invoke
log.warning()to log a warning and (mostly) forget about all of the details listed above.Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24584/head:pull/24584$ git checkout pull/24584Update a local copy of the PR:
$ git checkout pull/24584$ git pull https://git.openjdk.org/jdk.git pull/24584/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24584View PR using the GUI difftool:
$ git pr show -t 24584Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24584.diff
Using Webrev
Link to Webrev Comment