[apex] Various false-negatives since 7.3.0 when using triggers (ApexCRUDViolation, CognitiveComplexity, OperationWithLimitsInLoop)

[stephen-carter-at-sf]

Affects PMD Version: 7.3.0

Description:

I'm one of the lead software engineers at Salesforce who maintains the Salesforce Code Analyzer product which incorporates PMD into our product. Each month before upgrading, we run a comparison report of our tool against a large number of repos containing apex code. During this month's upgrade from PMD 7.2.0 to 7.3.0 we found a number of false negatives (violations which used to exist in 7.2.0 that no longer exist with 7.3.0). But the release notes don't account for these.

Attached is the comparison report
PMD_7_2_vs_PMD_7_3.zip

If you download and unzip this report, navigate to PMD_7_2_vs_PMD_7_3 > overall_report.html where you will fine an aggregate page that shows:

Found 435 mismatches:
  * OperationWithHighCostInLoop: 2
  * ApexCRUDViolation: 64
  * CognitiveComplexity: 25
  * UnusedLocalVariable: 67
  * StdCyclomaticComplexity: 43
  * AvoidDeeplyNestedIfStmts: 185
  * CyclomaticComplexity: 20
  * OperationWithLimitsInLoop: 8
  * NcssMethodCount: 9
  * AvoidLogicInTrigger: 4
  * AvoidDebugStatements: 6
  * DebugsShouldUseLoggingLevel: 6
  * LocalVariableNamingConventions: 10
  * IfElseStmtsMustUseBraces: 8
  * IfStmtsMustUseBraces: 8
  * EmptyStatementBlock: 5

and you can navigate down the report to the individual repo reports. For example you will see the report for https://git.soma.salesforce.com/cli-scanner/SalesforceLabsUnmanaged.git contains 13 mismatches. Clicking its report.html link you'll find it says

Found 13 mismatches:
  * OperationWithHighCostInLoop: 1
  * ApexCRUDViolation: 7
  * CognitiveComplexity: 1
  * UnusedLocalVariable: 4

It will reveal a number of violations that PMD 7.2.0 found but PMD 7.3.0 did not find. For example:

• /chatter-unfollow-rules-unmanaged-package/main/default/triggers/createNameAndValidateRuleInfo.trigger on line 15
  used to have the OperationWithHighCostInLoop rule throw a violation but now it doesn't
• /chatter-unfollow-rules-unmanaged-package/main/default/triggers/limitNumLSRRules.trigger on line 3 used to have the ApexCRUDViolation rule throw a violation but now it doesn't

There are hundreds of these that it found. Overall, there still was over 99% match between but false negatives are something we were hoping your team could investigate further to see if these were intentional or not.

Running PMD through: CLI

[jsotuyod]

@stephen-carter-at-sf thanks for reporting this. My guess is this is most likely related to #4977, as it changed the parser and AST versions, but none of the existing tests regressed…

Unfortunately soma is not open outside of SF (I know, I used to be a VP of Eng there 😅)… but if there are any good Open Source Apex / Visual Force projects you can think of, it would be very useful to include them in our own regression-tester project list, which right now only includes Java projects.

[stephen-carter-at-sf]

@jsotuyod Although this is a really old list, this is what we currently use as a dummy test bed for our comparison tool:
repos.json
Although some of those are internal (like git.soma) ... many of those github repos I believe are indeed public.

But yeah, I just realized the examples I used in the above issue description do point to git.soma unfortunately. But the report has false negatives from public repos as well.

I'd recommend looking at the report for https://github.com/Groundwire/Groundwire-Moves-Management.git contained within that overall report. It has a few that I believe would be a good starting point:

Found 5 mismatches:

• NcssMethodCount: 1
• AvoidDeeplyNestedIfStmts: 3
• CyclomaticComplexity: 1
• ApexCRUDViolation: 1

All from the one file: /src/triggers/TaskAfterTrigger.trigger

[jsotuyod]

@stephen-carter-at-sf awesome! thanks for the insight

Unfortunately, I don't think this will get fixed before 7.5.0, as 7.4.0 is due tomorrow

[stephen-carter-at-sf]

Yeah I totally understand. The goal is not to hurry to get these fixed... but really to initiate an investigation to see if these changes were indeed intentional. Some of these changes may very well be correct. If our customers complain in some way (which false-negatives are less likely for them to complain) then we'll escalate this issue.

[adangel]

The rule UnusedLocalVariable got a bugfix in 7.3.0 (#5000), that's why these are not reported anymore. As far as I have seen, this was the only rule difference, that was not in a trigger.

All other differences are in triggers. Some false negatives have already been fixed by #5139 / #5146 in 7.5.0.

One case was, that the trigger couldn't be parsed with 7.2.0 but 7.3.0 now reported new violations (e.g. HVEMApprovalProcessAction.trigger contains method declarations...). This resulted in many new (correct) violations.

The remaining issues are to be fixed in 7.6.0.