Implement compiler-supplied ids for java compilation #29425
Conversation
hegyibalint
added
a:feature
| assertProblem(it, "WARNING", true) | ||
| fqid == 'compilation:java:java-compilation-warning' | ||
| details == 'redundant cast to java.lang.String' | ||
| assertLocations(it, true) |
There was a problem hiding this comment.
I was never a fan of boolean parameters for functions. Instead, It's not strictly related to this pr, but I'd recommend to introduce a new assertPreciseLocation method.
There was a problem hiding this comment.
I would not force it into this PR, but we could make an issue to review the structure there.
...ge-java/src/main/java/org/gradle/api/internal/tasks/compile/DiagnosticToProblemListener.java
Outdated
Show resolved
Hide resolved
...ge-java/src/main/java/org/gradle/api/internal/tasks/compile/DiagnosticToProblemListener.java
Outdated
Show resolved
Hide resolved
...ge-java/src/main/java/org/gradle/api/internal/tasks/compile/DiagnosticToProblemListener.java
Outdated
Show resolved
Hide resolved
| 'compilation:java:java-compilation-failed' : 'Java compilation error', | ||
| 'compilation:java:java-compilation-warning' : 'Java compilation warning', | ||
| 'compilation:java:java-compilation-advice' : 'Java compilation note', | ||
| // Flexible category, as the category id's last component, and the message, will be supplied by the compiler |
There was a problem hiding this comment.
For the most part, I'd prefer exact content listed here. At the same time, if the list becomes unmanageable (dozens of tests fail and the list of known IDs are unmanageable), then I'd switch to pattern matching. Is this the case here?
There was a problem hiding this comment.
I took a different approach here: I kept the literal message and only kept the end if IDs dynamic. I think this is a sweet spot for not making the list huge yet having confidence that the value incoming is a (probably) valid one.
| details == 'redundant cast to java.lang.String' | ||
| assertLocations(it, true) | ||
| severity == Severity.WARNING | ||
| fqid == 'compilation:java:compiler-warn-redundant-cast' |
There was a problem hiding this comment.
❓ Does it make sense to have the severity in the id? And would it make sense to kick out the compiler prefix? So we'd end up with something like compilation:java:redundant-cast.
There was a problem hiding this comment.
IMO the ID is just an ID, and it's not an issue if it contains redundant information.
Even if I'm pretty certain that the prefix here is stable (e.g., list of JDK7 compiler codes vs. mainline compiler codes), I would still avoid making an assumption of the code format (apart from the dot separation).
Do you have something in mind where the shorter ID might be beneficial?
There was a problem hiding this comment.
Fine with me. Will some humans end up reading the ID at some point?
There was a problem hiding this comment.
In Buildship we will soon have a UI for problems. Here's a screenshot from my wip pr.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. If you don't want the stale bot to close it, then set a milestone for it. |
|
This pull request has been automatically closed due to inactivity. |
hegyibalint
force-pushed
the
bhegyi/problems/java-compile/use-compiler-ids
branch
3 times, most recently
from
7a408b4 to
89c7618
Compare
hegyibalint
force-pushed
the
bhegyi/problems/java-compile/use-compiler-ids
branch
from
89c7618 to
e4ee553
Compare
...integ-testing/src/main/groovy/org/gradle/integtests/fixtures/problems/KnownProblemIds.groovy
Show resolved
Hide resolved
hegyibalint
force-pushed
the
bhegyi/problems/java-compile/use-compiler-ids
branch
from
3f61685 to
a088116
Compare
Signed-off-by: Bálint Hegyi <[email protected]>
|
WARN: Based on labels, this pull request addresses notable issue but no changes to release note found. |
|
The merge queue build has failed. Click here to see all failures. |
hegyibalint
deleted the
bhegyi/problems/java-compile/use-compiler-ids
branch
Our current approach to Java compilation diagnostic IDs and their respective label was a bit simple: we derived an ID and a message from the kind of diagnostic, which limited us to 5 categories overall.
The compiler provides us with separate keys for various problems, which we could use on our end to also create finer-grained reports.
Fixes #30514