SONARJAVA-572 Reduce severity, ignore enum constant and handle members having same visibility but different types

Author: Wohops

its/ruling/src/test/resources/expected/squid-S1845.json

@@ -24,10 +24,6 @@
 'project:com/sun/corba/se/impl/naming/cosnaming/BindingIteratorImpl.java':[
 178,
 ],
-'project:com/sun/corba/se/impl/oa/poa/AOMEntry.java':[
-245,
-246,
-],
 'project:com/sun/jmx/snmp/IPAcl/Parser.java':[
 168,
 232,
@@ -56,11 +52,8 @@
 42,
 50,
 ],
-'project:com/sun/org/apache/xerces/internal/dom/NamedNodeMapImpl.java':[
-418,
-422,
-426,
-430,
+'project:com/sun/org/apache/xerces/internal/dom/CoreDocumentImpl.java':[
+183,
 ],
 'project:com/sun/org/apache/xerces/internal/dom/events/EventImpl.java':[
 43,
@@ -134,12 +127,16 @@
 1996,
 2036,
 ],
+'project:java/awt/GridBagLayoutInfo.java':[
+84,
+],
 'project:java/awt/MediaTracker.java':[
 883,
 884,
 ],
 'project:java/awt/SentEvent.java':[
 45,
+48,
 ],
 'project:java/awt/SequencedEvent.java':[
 49,
@@ -156,14 +153,21 @@
 'project:java/io/Console.java':[
 373,
 ],
-'project:java/io/StreamTokenizer.java':[
-775,
-],
-'project:java/lang/invoke/FilterOneArgument.java':[
-53,
-],
-'project:java/lang/invoke/MethodHandleImpl.java':[
-251,
+'project:java/io/ObjectInputStream.java':[
+3426,
+],
+'project:java/lang/invoke/ToGeneric.java':[
+374,
+375,
+376,
+378,
+380,
+809,
+860,
+894,
+932,
+974,
+1020,
 ],
 'project:java/math/MutableBigInteger.java':[
 1187,
@@ -172,25 +176,20 @@
 712,
 713,
 ],
-'project:java/security/Signature.java':[
-551,
-587,
-621,
-657,
-],
-'project:java/util/Formatter.java':[
-4264,
+'project:java/util/HashMap.java':[
+1050,
 ],
 'project:java/util/Hashtable.java':[
-1007,
 1008,
 ],
-'project:java/util/concurrent/ThreadPoolExecutor.java':[
-379,
-1953,
+'project:java/util/LinkedHashMap.java':[
+388,
+],
+'project:java/util/concurrent/ConcurrentHashMap.java':[
+1285,
 ],
-'project:java/util/prefs/Base64.java':[
-214,
+'project:java/util/regex/Pattern.java':[
+1696,
 ],
 'project:javax/management/Query.java':[
 168,
@@ -201,41 +200,34 @@
 536,
 554,
 ],
-'project:javax/management/remote/rmi/RMIConnectionImpl.java':[
-1705,
-],
-'project:javax/management/remote/rmi/RMIConnector.java':[
-2466,
-],
 'project:javax/sql/RowSet.java':[
 107,
 2182,
 ],
 'project:javax/sql/rowset/BaseRowSet.java':[
 4504,
 ],
+'project:javax/swing/JProgressBar.java':[
+153,
+],
 'project:javax/swing/JScrollPane.java':[
 204,
 ],
 'project:javax/swing/ScrollPaneLayout.java':[
 69,
 ],
-'project:javax/swing/plaf/basic/BasicPopupMenuUI.java':[
-623,
-],
-'project:javax/swing/plaf/basic/BasicTreeUI.java':[
-4276,
-4289,
-4294,
-],
 'project:javax/swing/plaf/synth/SynthComboBoxUI.java':[
 539,
 ],
 'project:javax/swing/text/html/HTML.java':[
 379,
 ],
+'project:javax/swing/text/html/HTMLDocument.java':[
+1795,
+],
 'project:javax/swing/text/html/HTMLEditorKit.java':[
 636,
+1818,
 ],
 'project:javax/swing/text/html/parser/AttributeList.java':[
 49,
@@ -249,10 +241,6 @@
 'project:javax/swing/text/html/parser/Entity.java':[
 45,
 ],
-'project:javax/swing/text/html/parser/Parser.java':[
-394,
-445,
-],
 'project:javax/xml/datatype/XMLGregorianCalendar.java':[
 1052,
 ],

java-checks/src/main/java/org/sonar/java/checks/MembersDifferOnlyByCapitalizationCheck.java

@@ -44,7 +44,7 @@
   key = "S1845",
   name = "Methods and field names should not be the same or differ only by capitalization",
   tags = {"confusing"},
-  priority = Priority.CRITICAL)
+  priority = Priority.MAJOR)
 @ActivatedByDefault
 @SqaleSubCharacteristic(RulesDefinition.SubCharacteristics.UNDERSTANDABILITY)
 @SqaleConstantRemediation("10min")
@@ -99,7 +99,7 @@ private static boolean isOverriding(Symbol symbol) {
 
   private static boolean isInvalidMember(Symbol currentMember, Symbol knownMember) {
     if (!isOverriding(currentMember)) {
-      return !sameName(currentMember, knownMember) || (differentTypes(currentMember, knownMember) && bothPublic(currentMember, knownMember));
+      return differentTypes(currentMember, knownMember) ? sameVisibilityNotPrivate(currentMember, knownMember) : !sameName(currentMember, knownMember);
     }
     return false;
   }
@@ -108,6 +108,18 @@ private static boolean isValidIssueLocation(Symbol currentMember, Symbol knownMe
     return !sameOwner(currentMember, knownMember) || isOverriding(knownMember) || getDeclarationLine(currentMember) > getDeclarationLine(knownMember);
   }
 
+  private static boolean sameVisibilityNotPrivate(Symbol s1, Symbol s2) {
+    return bothPublic(s1, s2) || bothProtected(s1, s2) || bothPackageVisibility(s1, s2);
+  }
+
+  private static boolean bothPackageVisibility(Symbol s1, Symbol s2) {
+    return s1.isPackageVisibility() && s2.isPackageVisibility();
+  }
+
+  private static boolean bothProtected(Symbol s1, Symbol s2) {
+    return s1.isProtected() && s2.isProtected();
+  }
+
   private static boolean bothPublic(Symbol s1, Symbol s2) {
     return s1.isPublic() && s2.isPublic();
   }
@@ -172,19 +184,19 @@ private static List<Symbol> retrieveMembers(Symbol.TypeSymbol classSymbol) {
   private static List<Symbol> extractMembers(Symbol.TypeSymbol classSymbol, boolean ignorePrivate) {
     List<Symbol> results = Lists.newLinkedList();
     for (Symbol symbol : classSymbol.memberSymbols()) {
-      if ((variableButNotThisNorSuper(symbol) || methodButNotConstructor(symbol)) && !(symbol.isPrivate() && ignorePrivate)) {
+      if ((isVariableToExtract(symbol) || isMethodToExtract(symbol)) && !(symbol.isPrivate() && ignorePrivate)) {
         results.add(symbol);
       }
     }
     return results;
   }
 
-  private static boolean variableButNotThisNorSuper(Symbol symbol) {
+  private static boolean isVariableToExtract(Symbol symbol) {
     String name = symbol.name();
-    return symbol.isVariableSymbol() && !"this".equals(name) && !"super".equals(name);
+    return !symbol.isEnum() && symbol.isVariableSymbol() && !"this".equals(name) && !"super".equals(name);
   }
 
-  private static boolean methodButNotConstructor(Symbol symbol) {
+  private static boolean isMethodToExtract(Symbol symbol) {
     return symbol.isMethodSymbol() && !"<init>".equals(symbol.name());
   }
 }

java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S1845.html

@@ -1,12 +1,12 @@
-<p>Looking at the set of methods in a class, including superclass methods, and finding two methods or fields that differ only by capitalization is confusing to users of the class. It is similarly confusing to have a method and a field which differ only in capitalization or a method and a field with exactly the same name.</p>
+<p>Looking at the set of methods in a class, including superclass methods, and finding two methods or fields that differ only by capitalization is confusing to users of the class. It is similarly confusing to have a method and a field which differ only in capitalization or a method and a field with exactly the same name and visibility.</p>
 <p>In the case of methods, it may have been a mistake on the part of the original developer, who intended to override a superclass method, but instead added a new method with nearly the same name.</p>
 <p>Otherwise, this situation simply indicates poor naming. Method names should be action-oriented, and thus contain a verb, which is unlikely in the case where both a method and a member have the same name (with or without capitalization differences). However, renaming a public method could be disruptive to callers. Therefore renaming the member is the recommended action.</p>
 <h2>Noncompliant Code Example</h2>
 
 <pre>
 public class Car{
 
-  private DriveTrain drive;
+  public DriveTrain drive;
 
   public void tearDown(){...}
 
@@ -26,11 +26,11 @@ <h2>Compliant Solution</h2>
 <pre>
 public class Car{
 
-  private DriveTrain driveTrain;
+  private DriveTrain drive;
 
   public void tearDown(){...}
 
-  public void drive() {...}  // field was renamed
+  public void drive() {...}  // field visibility reduced
 }
 
 public class MyCar extends Car{

java-checks/src/test/files/checks/MembersDifferOnlyByCapitalizationCheck.java

@@ -19,22 +19,10 @@ public void myOtherMethoD() {} // Noncompliant {{Rename method "myOtherMethoD" t
   private static void gUl() {} // Noncompliant {{Rename method "gUl" to prevent any misunderstanding/clash with method "gul" defined on line 20.}}
   public void gul() {} // Compliant
 
-  public int tmp0;
-  public void tmp0() {} // Noncompliant {{Rename method "tmp0" to prevent any misunderstanding/clash with field "tmp0" defined on line 22.}}
-  
-  private int tmp1;
-  private void tmp1() {} // Compliant
-  
-  private int tmp2;
-  public void tmp2() {}; // Compliant
-  
-  public int tmp3;
-  private void tmp3() {}; // Compliant
-  
   private boolean qix;
 
   public Object myField; // Noncompliant
-  public void myField();
+  public void myField(); // Compliant, as it overrides the parent interface method
 
   public A() {}
   class MyInnerClass {}
@@ -61,3 +49,35 @@ public void myMethoD() {} // Noncompliant {{Rename method "myMethoD" to prevent
 
   public void fOo(int i) {} // Noncompliant
 }
+
+class Visibility {
+  public int tmp0;
+  private void tmp0() {}
+  public void tmp0(int i) {} // Noncompliant {{Rename method "tmp0" to prevent any misunderstanding/clash with field "tmp0" defined on line 54.}}
+  protected void tmp0(long l) {}
+  void tmp0(short s) {}
+  
+  private int tmp1;
+  private void tmp1() {} // Compliant - private members having same name are ignored
+  public void tmp1(int i) {}
+  protected void tmp1(long l) {}
+  void tmp1(short s) {}
+  
+  int tmp2;
+  private void tmp2() {}
+  public void tmp1(int i) {}
+  protected void tmp1(long l) {}
+  void tmp2(short s) {}  // Noncompliant {{Rename method "tmp2" to prevent any misunderstanding/clash with field "tmp2" defined on line 66.}}
+  
+  protected int tmp3;
+  private void tmp3() {}
+  public void tmp3(int i) {}
+  protected void tmp3(long l) {} // Noncompliant {{Rename method "tmp3" to prevent any misunderstanding/clash with field "tmp3" defined on line 72.}}
+  void tmp3(short s) {} 
+}
+
+public enum MyEnum {
+  FOO;
+  
+  public void foo() {} // Compliant
+}