From ac3e59a4b76c0038a1ada2b970c4a6945c374b99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 19 Mar 2018 01:34:28 +0100 Subject: [PATCH 1/7] Add new rule ControlStatementBraces --- .../resources/category/java/codestyle.xml | 51 ++++ .../rule/codestyle/CodeStyleRulesTest.java | 1 + .../codestyle/xml/ControlStatementBraces.xml | 238 ++++++++++++++++++ 3 files changed, 290 insertions(+) create mode 100644 pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 80ecbf4194..073deaac1d 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -453,6 +453,57 @@ boolean bar(int x, int y) { + + + Enforce a policy for braces on control statements. It is recommended to use braces on 'if ... else' + statements and loop statements, even if they are optional. This usually makes the code clearer, and + helps prepare the future when you need to add another statement. That said, this rule lets you control + which statements are required to have braces via properties. + + From 6.2.0 on, this rule supersedes WhileLoopMustUseBraces, ForLoopMustUseBraces, IfStmtMustUseBraces, + and IfElseStmtMustUseBraces. + + 3 + + + + + + + + + + + + + + + + + + + + + + While, no braces + 1 + + + + + While, no braces, allowed + false + 0 + + + + + While, with braces + 0 + + + + + Empty while + 1 + + + + + Empty while, allowed + true + 0 + + + + + For, no braces + 1 + + + + + For, no braces, allowed + false + 0 + + + + + For, with braces + 0 + + + + + Empty for + 1 + + + + + Empty while, allowed + true + 0 + + + + + If else, no braces + 2 + + + + + If else, no braces, allowed + false + 0 + + + + + If else, braces on if + 1 + + + + + If else, braces on else + 1 + + + + + If chain, partial braces + 2 + 5,10 + + + + From 2711d88e3cf7a3b2e995f19f9c38c8ed4012405b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Tue, 20 Mar 2018 17:46:54 +0100 Subject: [PATCH 2/7] Add support for case stmts A case label is flagged if one of its subordinate statements is not braced. The message could be more explicit, eg, if we flagged the unbraced statement instead of the label in case part of the label's statements are braced --- .../resources/category/java/codestyle.xml | 8 +- .../codestyle/xml/ControlStatementBraces.xml | 142 +++++++++++++++++- 2 files changed, 148 insertions(+), 2 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 073deaac1d..5b34a74322 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -474,7 +474,7 @@ boolean bar(int x, int y) { - + @@ -489,6 +489,12 @@ boolean bar(int x, int y) { | (: The violation is reported on the sub statement -- not the if statement :) //Statement[$checkIfElseStmt and parent::IfStatement and not(child::Block or child::IfStatement)] + | + (: Reports case labels if one of their subordinate statements is not braced. Ignores if there are several blocks. :) + //SwitchLabel[$checkCaseStmt] + [some $stmt (: in only the block statements until the next label :) + in following-sibling::BlockStatement except following-sibling::SwitchLabel[1]/following-sibling::BlockStatement + satisfies not($stmt/Statement/Block)] ]]> diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml index d95929d760..3e05fd0891 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml @@ -135,7 +135,7 @@ - Empty while, allowed + Empty for, allowed true 0 + + Do while, no braces + 1 + + + + + Do while, no braces, allowed + false + 0 + + + + + Do while, with braces + 0 + + + + + Empty Do while + 1 + + + + + Empty Do while, allowed + true + 0 + + + If else, no braces 2 @@ -235,4 +310,69 @@ ]]> + + + Case, no braces, allowed + 0 + + + + + Case, no braces + true + 2 + 6,9 + + + + + Case, dangling unbraced statement + true + 1 + 6 + + + + From 1ae8b1c8e68a5e8c30bc73b8e653357dc16f14e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Tue, 20 Mar 2018 17:20:26 +0100 Subject: [PATCH 3/7] Refine case statement treatment Every dangling unbraced statement will be flagged individually This complexifies the expression, and still ignores some cases, eg: case 3: { x++; } { y++; break; } in which the label should be flagged anyway --- .../resources/category/java/codestyle.xml | 19 ++++++++++++++----- .../codestyle/xml/ControlStatementBraces.xml | 6 +++--- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 5b34a74322..2ec838527c 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -490,11 +490,20 @@ boolean bar(int x, int y) { (: The violation is reported on the sub statement -- not the if statement :) //Statement[$checkIfElseStmt and parent::IfStatement and not(child::Block or child::IfStatement)] | - (: Reports case labels if one of their subordinate statements is not braced. Ignores if there are several blocks. :) - //SwitchLabel[$checkCaseStmt] - [some $stmt (: in only the block statements until the next label :) - in following-sibling::BlockStatement except following-sibling::SwitchLabel[1]/following-sibling::BlockStatement - satisfies not($stmt/Statement/Block)] + ( + if ($checkCaseStmt) + then for $label in //SwitchLabel + return if (empty($label/following-sibling::BlockStatement except $label/following-sibling::SwitchLabel[1]/following-sibling::BlockStatement)) + then () (: Ignore fallthrough case :) + else if (every $stmt in ($label/following-sibling::BlockStatement except $label/following-sibling::SwitchLabel[1]/following-sibling::BlockStatement) + satisfies not($stmt/Statement/Block)) + then $label (: Flag the label if every statement is unbraced :) + else for $stmt in ($label/following-sibling::BlockStatement except $label/following-sibling::SwitchLabel[1]/following-sibling::BlockStatement) + return if (not($stmt/Statement/Block)) + then $stmt (: Else flag unbraced statements individually :) + else () + else () + ) ]]> diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml index 3e05fd0891..8d3341fd64 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml @@ -354,17 +354,17 @@ Case, dangling unbraced statement true 1 - 6 + 9 Date: Tue, 20 Mar 2018 17:22:49 +0100 Subject: [PATCH 4/7] Revert "Refine case statement treatment" This reverts commit 580560c28c56cb57ca5350f9035856b1362400ec. --- .../resources/category/java/codestyle.xml | 19 +++++-------------- .../codestyle/xml/ControlStatementBraces.xml | 6 +++--- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 2ec838527c..5b34a74322 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -490,20 +490,11 @@ boolean bar(int x, int y) { (: The violation is reported on the sub statement -- not the if statement :) //Statement[$checkIfElseStmt and parent::IfStatement and not(child::Block or child::IfStatement)] | - ( - if ($checkCaseStmt) - then for $label in //SwitchLabel - return if (empty($label/following-sibling::BlockStatement except $label/following-sibling::SwitchLabel[1]/following-sibling::BlockStatement)) - then () (: Ignore fallthrough case :) - else if (every $stmt in ($label/following-sibling::BlockStatement except $label/following-sibling::SwitchLabel[1]/following-sibling::BlockStatement) - satisfies not($stmt/Statement/Block)) - then $label (: Flag the label if every statement is unbraced :) - else for $stmt in ($label/following-sibling::BlockStatement except $label/following-sibling::SwitchLabel[1]/following-sibling::BlockStatement) - return if (not($stmt/Statement/Block)) - then $stmt (: Else flag unbraced statements individually :) - else () - else () - ) + (: Reports case labels if one of their subordinate statements is not braced. Ignores if there are several blocks. :) + //SwitchLabel[$checkCaseStmt] + [some $stmt (: in only the block statements until the next label :) + in following-sibling::BlockStatement except following-sibling::SwitchLabel[1]/following-sibling::BlockStatement + satisfies not($stmt/Statement/Block)] ]]> diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml index 8d3341fd64..3e05fd0891 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml @@ -354,17 +354,17 @@ Case, dangling unbraced statement true 1 - 9 + 6 Date: Tue, 20 Mar 2018 17:28:37 +0100 Subject: [PATCH 5/7] Report multiple blocks under case label --- .../resources/category/java/codestyle.xml | 9 ++++--- .../codestyle/xml/ControlStatementBraces.xml | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 5b34a74322..5033857a44 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -490,11 +490,12 @@ boolean bar(int x, int y) { (: The violation is reported on the sub statement -- not the if statement :) //Statement[$checkIfElseStmt and parent::IfStatement and not(child::Block or child::IfStatement)] | - (: Reports case labels if one of their subordinate statements is not braced. Ignores if there are several blocks. :) + (: Reports case labels if one of their subordinate statements is not braced :) //SwitchLabel[$checkCaseStmt] - [some $stmt (: in only the block statements until the next label :) - in following-sibling::BlockStatement except following-sibling::SwitchLabel[1]/following-sibling::BlockStatement - satisfies not($stmt/Statement/Block)] + [count(following-sibling::BlockStatement except following-sibling::SwitchLabel[1]/following-sibling::BlockStatement) > 1 + or (some $stmt (: in only the block statements until the next label :) + in following-sibling::BlockStatement except following-sibling::SwitchLabel[1]/following-sibling::BlockStatement + satisfies not($stmt/Statement/Block))] ]]> diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml index 3e05fd0891..571e99cf2a 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/ControlStatementBraces.xml @@ -374,5 +374,31 @@ ]]> + + Case, dangling unbraced statement + true + 1 + 6 + + + From 603a306370396da62169e0b63ccb059f92dc9867 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Tue, 20 Mar 2018 17:42:46 +0100 Subject: [PATCH 6/7] Deprecate former rules, update changelog --- docs/pages/release_notes.md | 25 ++++++++++++++----- .../resources/category/java/codestyle.xml | 4 +++ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index fba50008f4..3e4bfdf941 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -47,13 +47,22 @@ On both scenarios, disabling the cache takes precedence over setting a cache loc #### New Rules -* The new Java rule `MissingOverride` (category `bestpractices`) detects overridden and implemented methods, - which are not marked with the `@Override` annotation. Annotating overridden methods with `@Override` ensures - at compile time that the method really overrides one, which helps refactoring and clarifies intent. +* The new Java rule [`MissingOverride`](pmd_rules_java_bestpractices.html#missingoverride) + (category `bestpractices`) detects overridden and implemented methods, which are not marked with the + `@Override` annotation. Annotating overridden methods with `@Override` ensures at compile time that + the method really overrides one, which helps refactoring and clarifies intent. -* The new Java rule `UnnecessaryAnnotationValueElement` (category `codestyle`) detects annotations with a single - element (`value`) that explicitely names it. That is, doing `@SuppressWarnings(value = "unchecked")` would be - flagged in favor of `@SuppressWarnings("unchecked")`. +* The new Java rule [`UnnecessaryAnnotationValueElement`](pmd_rules_java_codestyle.html#unnecessaryannotationvalueelement) + (category `codestyle`) detects annotations with a single element (`value`) that explicitely names it. + That is, doing `@SuppressWarnings(value = "unchecked")` would be flagged in favor of + `@SuppressWarnings("unchecked")`. + +* The new Java rule [`ControlStatementBraces`](pmd_rules_java_codestyle.html#controlstatementbraces) + (category `codestyle`) enforces the presence of braces on control statements where they are optional. + Properties allow to customize which statements are required to have braces. This rule replaces the now + deprecated rules `WhileLoopMustUseBraces`, `ForLoopMustUseBraces`, `IfStmtMustUseBraces`, and + `IfElseStmtMustUseBraces`. More than covering the use cases of those rules, this rule also supports + `do ... while` statements and `case` labels of `switch` statements (disabled by default). #### Modified Rules @@ -68,6 +77,10 @@ On both scenarios, disabling the cache takes precedence over setting a cache loc that allows to configure annotations that imply the method should be ignored. By default `@java.lang.Deprecated` is ignored. +#### Deprecated Rules + +* The Java rules `WhileLoopMustUseBraces`, `ForLoopMustUseBraces`, `IfStmtMustUseBraces`, and `IfElseStmtMustUseBraces` + are deprecated. They will be replaced by the new rule `ControlStatementBraces`, in the category `codestyle`. ### Fixed Issues diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 5033857a44..f51b6d1750 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -721,6 +721,7 @@ public class Foo { @@ -790,6 +791,7 @@ public interface GenericDao { @@ -828,6 +830,7 @@ if (foo) @@ -1803,6 +1806,7 @@ public class Foo { From 8dfef6c1659848b4e507309162854b7f4448eab6 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sat, 24 Mar 2018 22:13:27 +0100 Subject: [PATCH 7/7] Update changelog, refs #974 --- docs/pages/release_notes.md | 1 + pmd-core/src/main/resources/rulesets/releases/620.xml | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 3e4bfdf941..08efdf6b2a 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -90,6 +90,7 @@ On both scenarios, disabling the cache takes precedence over setting a cache loc * [#907](https://github.com/pmd/pmd/issues/907): \[java] UnusedPrivateField false-positive with @FXML * [#963](https://github.com/pmd/pmd/issues/965): \[java] ArrayIsStoredDirectly not triggered from variadic functions * java-codestyle + * [#974](https://github.com/pmd/pmd/issues/974): \[java] Merge *StmtMustUseBraces rules * [#983](https://github.com/pmd/pmd/issues/983): \[java] Detect annotations with single value element * java-design * [#837](https://github.com/pmd/pmd/issues/837): \[java] CFGs of declared but not called lambdas are treated as parts of an enclosing method's CFG diff --git a/pmd-core/src/main/resources/rulesets/releases/620.xml b/pmd-core/src/main/resources/rulesets/releases/620.xml index 70c17e4422..5a4a3a3613 100644 --- a/pmd-core/src/main/resources/rulesets/releases/620.xml +++ b/pmd-core/src/main/resources/rulesets/releases/620.xml @@ -10,5 +10,6 @@ This ruleset contains links to rules that are new in PMD v6.2.0 +