Skip to content

[java] Fix false negative in AvoidLiteralsInIfCondition#2150

Merged
adangel merged 5 commits into
pmd:masterfrom
adangel:issue-2140
Dec 18, 2019
Merged

[java] Fix false negative in AvoidLiteralsInIfCondition#2150
adangel merged 5 commits into
pmd:masterfrom
adangel:issue-2140

Conversation

@adangel

@adangel adangel commented Dec 8, 2019

Copy link
Copy Markdown
Member

Fixes #2140

This also contains a fix in the Saxon XPath integration in PMD: When using the union operator, we sometimes threw some nodes of the result away...

Before submitting a PR, please check that:

  • The PR is submitted against master. The PMD team will merge back to support branches as needed.
  • ./mvnw clean verify passes. This will build and test PMD, execute PMD and checkstyle rules. Check this for more info

@adangel adangel added this to the 6.21.0 milestone Dec 8, 2019
@ghost

ghost commented Dec 8, 2019 •
edited by ghost
Loading

Copy link
Copy Markdown
1 Message
📖 This changeset introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report
✅ This changeset introduces 1393 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
Full report
✅ This changeset introduces 1196 new violations, 0 new errors and 0 new configuration errors,
removes 36 violations, 0 errors and 0 configuration errors.
Full report

Generated by 🚫 Danger

@adangel adangel mentioned this pull request Dec 9, 2019
Closed
@adangel

adangel commented Dec 9, 2019

Copy link
Copy Markdown
Member Author

Maybe we should add a property to enable the new behavior and keep the rule the same in PMD 6.x. In PMD 7, the default value of the property can be changed.
The new behavior find lots of new violations, which might be annoying if you just perform a minor update of PMD...

@adangel adangel added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Dec 14, 2019
@adangel

adangel commented Dec 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

TODOs:

  • verify the following examples:
    • if the property says “1”, then
    • if (1) -> ok
    • if (1+1) -> not ok, multiple literals in expression
    • if (a+1) -> ok! single literal, whitelisted
    • if (bodyStart >= 0 && bodyStart != (currentToken.length() - 1)) { -> ok! single literal per expression, both whitelisted
    • if (1 * 5) -> not ok - literal 5
    • if (a + 5) -> not ok
  • Add a property to enable expression checking, so that users can opt-in. It should not be enabled by default, since it introduces many new violations. Consider to switch this on by default with PMD 7.

adangel and others added 5 commits December 16, 2019 11:16
@adangel
[core] Fix XPath node compareOrder
8b66e0a 
In Saxon, NodeInfo.compareOrder is used to determine, whether a
node is already contained in a union set or not. For different nodes
it must return a value != 0. It should consider the position of the
nodes in document order. The new implementation uses now the
position of the nodes (begin line, begin column) to determine the order.
It might fail, if the position of the nodes are not correct.
@adangel
[java] AvoidLiteralsInIfCondition: false negative for expressions
ed25318 
Fixes pmd#2140
@adangel
[java] AvoidLiteralsInIfCondition: report Literal rather than IfCondi…
c0d0fd3 
…tion

If the IfCondition spans multiple lines, it easier to spot the
literal, if the violation is reported on the correct line where
the literal is located.
@adangel
[java] AvoidLiteralsInIfCondition: add additional check for expressions
1cfd14e
@adangel
[java] AvoidLiteralsInIfCondition: add property ignoreExpressions
7b5e970 
The property ignoreExpressions is set to true in order to keep
the rule backwards compatible.
@adangel adangel removed the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Dec 16, 2019
@adangel adangel merged commit 7b5e970 into pmd:master Dec 18, 2019
@adangel adangel deleted the issue-2140 branch December 18, 2019 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

6.21.0

Development

Successfully merging this pull request may close these issues.

[java] AvoidLiteralsInIfCondition: false negative for expressions

1 participant

@adangel