[core] Abort on semantic errors#3892
Conversation
Generated by 🚫 Danger |
f683215 to
4fa7db2
Compare
This should be done more thoroughly in a future PR
|
Hm... there seems to be a general problem now - as the pmd regression tester says at the end: There are also many reporting of unresolved method calls and such - I guess these are the new reported semantic errors? E.g. The last line of the progress report seems to be: Hm... should be something like We should solve this before merging these PRs... |
|
Ok, so the message "[main] ERROR net.sourceforge.pmd.PMD - An error occurred while executing PMD." means: one processing error occurred. So, nothing out of the ordinary. But - PMD 7 meanwhile exits with failing exit code, if that happens: pmd/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java Lines 244 to 252 in 1deb4b7 I guess, we should restore what the comment (still) says: "// processing errors are ignored". What I don't understand is, why this happens only on this branch - the baseline on pmd/7.0.x seems to be created without problems - although we find processing errors. See also #2827 - handling of processing errors with respect to the exit code is still undefined |
|
Slowly I begin to understand: in the above snippet, we don't check |
|
Ok, it seems to be this error message, that is counted now and changes the exit code: Note: I can reproduce this message on branch pmd/7.0.x as well, but the exit code is 0 there... |
| SemanticErrorReporter reporter = SemanticErrorReporter.reportToLogger(configuration.getReporter(), LOG); | ||
| ParserTask task = new ParserTask( | ||
| languageVersion, | ||
| filename, | ||
| sourceCode, | ||
| SemanticErrorReporter.reportToLogger(LOG), | ||
| reporter, |
There was a problem hiding this comment.
Ok, this is the change: previously we used a new SemanticErrorReporter, now the logs are forwarded to the main message reporter as well, which makes PMD exit with 1 if there are errors logged...
|
I'm surprised there is a FieldAccess in a
That's because semantic errors were previously only forwarded to an slf4j logger, but this PR also forwards them to the MessageReporter, which increments its error count. I thought this was the correct behaviour, but maybe the SemanticErrorReporter should instead create a ProcessingError. I think there is a difference between a ProcessingError (PMD crashes on some file) and a semantic error (PMD detects that the file is not well-formed source code) though... |
|
Ok, I think, that's what we need to do:
Overall, I think, it makes sense to exit with 1 if PMD logged something suspiciously erroneous. We can think about later, whether we should distinguish them from "hard disruption" error condition caused by exceptions (which was till now the only reason why we exited with 1). And how they relate to the processing errors, which we collect in the report. |
Yes, probably, maybe. Assuming, that the code compiles, that PMD analyzes, all semantic errors probably point out a bug in PMD. But that would be the same for processing errors... Maybe the difference is just that (at least, how it's implemented atm): processing errors are unexpected errors and semantic errors are detectable error conditions... |
A few thoughts about this Building on the technical definition of fault and failure: A semantic error currently is basically a gracefully detected fault, which may or may not cause an error/failure later (in rule execution). The failure can be an exception (processing error), or inconsistent behavior of rules (FNs/FPs). Rules might also "just work" by chance (or, the semantic error itself is an FP). By that reasoning, we could probably tolerate more faults and only skip rule analysis when we know rule processing will almost surely fail. Currently things like ambiguous references are reported as errors, because that's what a Java compiler would do. But we're not a Java compiler, and skipping rule analysis because of an ambiguous reference seems like overkill. We should probably demote those to warnings, and print something like "warning: input file might be invalid, this may cause problems in rules" (and link to a page of doc with details). About processing errors, I think they are in the report mostly so that users don't think that the file they occured in is fine because it has no violations. If semantic errors skip rule analysis, then the same argument applies to them, and they should probably cause a processing error to be reported. This makes me wonder if we need semantic errors at all. Throwing exceptions (SemanticException?) would achieve nearly the same thing. One difference is that you can only throw one exception, whereas you can accumulate errors if you don't throw them. But we still need a way to skip rule analysis even if no exception is thrown. So in summary,
|
|
I'll try to implement those changes over the weekend |
Describe the PR
This makes us skip rules if semantic errors are reported. This is useful for better error reporting. Previously errors might have been reported by throwing an exception. This hides further errors, now we can also collect them. This makes #3891 more useful.
Related issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)