fix #2305: do not apply UnnecessaryBlock for the ECMAScript (JavaScript) destructuring assignments

[oleksandr-shvets]

This pull request addresses an issue with the PMD rule UnnecessaryBlock in ECMAScript (JavaScript) analysis. The original rule flagged unnecessary blocks in JavaScript code, including those within import statements and destructuring assignments.

Fixes #2305

To improve the rule's accuracy and usability, this pull request modifies the PMD rule configuration to exclude unnecessary blocks within import statements and destructuring assignments. This refinement ensures that the rule focuses on identifying genuinely redundant blocks in JavaScript code, enhancing code quality without unnecessary noise.

Changes Made:

Updated the XPath expression in the PMD rule to exclude unnecessary blocks within import statements and destructuring assignments.
Added conditions to the XPath expression to exclude blocks within relevant ECMAScript syntax constructs.

Testing:

Added test code to the UnnecessaryBlock.xml
And mvn test runs ok

Impact:

Enhances the accuracy of the PMD rule UnnecessaryBlock in ECMAScript analysis.
Improves the user experience by reducing false positives and noise in code analysis results.
Ensures consistent and reliable identification of unnecessary blocks in JavaScript code.

[adangel]

Thanks for the PR.

Please have a look at my comments.

[adangel]

Please revert the changes in this file completely

[adangel]

If you remove the semicolon at the end of this line, then the rule will find a unnecessary block for the following { a, b }, because the Rhino parser doesn't understand this line...

It seems, that Rhino produces an "EmptyStatement", if it doesn't understand the syntax and tries to continue parsing with the next statement.

We can make use of this, and avoid some more false positives, if we check, whether the preceding node in the AST is a EmptyStatement, e.g.

//(Block|Scope)[not(parent::FunctionNode or parent::IfStatement or parent::ForLoop or parent::ForInLoop or parent::ForOfLoop
    or parent::WhileLoop or parent::DoLoop or parent::TryStatement or parent::CatchClause
    or parent::VariableDeclarator or parent::ImportDeclaration)][not(preceding::EmptyStatement)]

Maybe we should duplicate this test case, but remove all semicolons.

Note, that the original sample code on #2305 (which is only the import statement) doesn't reproduce the problem... since import is parsed into a EmptyStatement and there is neither a Block nor a Scope...

[adangel]

Just figured out, that this can be simplified as:

//(Block|Scope)
    [not(
        parent::FunctionNode or
        parent::IfStatement or
        parent::ForLoop or
        parent::ForInLoop or
        parent::ForOfLoop or
        parent::WhileLoop or
        parent::DoLoop or
        parent::TryStatement or
        parent::CatchClause or
        parent::VariableDeclarator or
        parent::ImportDeclaration
    )]

so that we don't need to repeat the exclusions...

Also note: Rhino doesn't have a "ImportStatement", so adding this doesn't do anything.

There is also no "ForOfLoop" node type - Rhino parses both for-in-loops and for-of-loops as "ForInLoop" nodes.

There is also no "VariableDeclarator" node type in Rhino - it is called "VariableDeclaration". But it never can have a Block or Scope as a child, so it doesn't need to be excluded.

Update:
I guess, the fix (or better to say workaround for Rhino not parsing every statement correctly) would be:

//(Block|Scope)
    [not(
        parent::FunctionNode or
        parent::IfStatement or
        parent::ForLoop or
        parent::ForInLoop or
        parent::WhileLoop or
        parent::DoLoop or
        parent::TryStatement or
        parent::CatchClause
    )]
    [not(preceding::EmptyStatement)]

[adangel]

Note, that this sample code doesn't produce a UnnecessaryBlock, even with your rule improvements.

The only way to reproduce the bug is by not using semicolons. A simple example is

import { foo } from './someScript.js' // missing semicolon
const { isLoggedIn } = useAuth();  // <--- violation here: unnecessary block

(I've updated #2305 meanwhile)

Please add a separate test case for that.

[oleksandr-shvets]

[not(preceding::EmptyStatement)] fixes not using semicolons case, but it not violates in these cases "Bad, for", "Bad, for in", "Bad, if", "Bad, try".

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   "Bad, for" failed
[ERROR] ruleTests().Bad, for in
[INFO]   Run 1: PASS
[ERROR]   Run 2: "Bad, for in" failed
[INFO] 
[ERROR]   "Bad, if" failed
[ERROR]   "Bad, try" failed
[INFO] 
[ERROR] Tests run: 154, Failures: 0, Errors: 4, Skipped: 0

[oleksandr-shvets]

fixed by this rule:

                    //Scope[
                        count(ancestor::*) = 1
                        and not(preceding::EmptyStatement)
                    ]
                    | //SwitchCase[./Scope]
                    | //(Scope|Block)[
                        ./(Scope|Block)
                        and count(./*) = 1
                    ]

First (//Scope) for root Blocks
Second (//SwitchCase) for Switch Statements
And Third — for everything else.