Issue #335: Checks under neverSuppressedChecks group should give more…

src/main/java/com/puppycrawl/tools/checkstyle/filters/JavaPatchFilterElement.java

@@ -51,6 +51,10 @@ public class JavaPatchFilterElement implements TreeWalkerFilter {
      */
     private static final Map<String, List<Integer>> CHECK_TO_ANCESTOR_NODES_MAP = new HashMap<>();
 
+    /** Checks whose violations should be attributed at enclosing type scope. */
+    private static final Set<String> CLASS_SCOPE_NEVER_SUPPRESSED_CHECKS = new HashSet<>(
+            Arrays.asList("CovariantEquals"));
+
     static {
         CHECK_TO_ANCESTOR_NODES_MAP.put("ArrayTrailingComma",
                 Arrays.asList(TokenTypes.ARRAY_INIT));
@@ -165,13 +169,13 @@ public boolean accept(TreeWalkerAuditEvent event) {
 
         if (Strategy.CONTEXT == strategy) {
             result = isFileNameMatching(event)
-                    && (isNeverSuppressCheck(event)
+                    && (isMatchingByNeverSuppressedCheck(event)
                     || isMatchingByContextStrategy(event)
                     || isLineMatching(event));
         }
         else {
             result = isFileNameMatching(event)
-                    && (isNeverSuppressCheck(event)
+                    && (isMatchingByNeverSuppressedCheck(event)
                     || isLineMatching(event));
         }
 
@@ -217,6 +221,53 @@ private boolean isNeverSuppressCheck(TreeWalkerAuditEvent event) {
         return result;
     }
 
+    /**
+     * Apply stricter causality matching for checks configured under neverSuppressedChecks.
+     *
+     * @param event audit event
+     * @return true when violation can be attributed to changed lines in touched file
+     */
+    private boolean isMatchingByNeverSuppressedCheck(TreeWalkerAuditEvent event) {
+        boolean result = false;
+        if (isNeverSuppressCheck(event)) {
+            result = isLineMatching(event);
+            if (!result) {
+                final String checkShortName = getCheckShortName(event);
+                if (CLASS_SCOPE_NEVER_SUPPRESSED_CHECKS.contains(checkShortName)
+                        && strategy != Strategy.NEWLINE) {
+                    result = isChangedLineInsideEnclosingType(event);
+                }
+                else {
+                    result = true;
+                }
+            }
+        }
+        return result;
+    }
+
+    private boolean isChangedLineInsideEnclosingType(TreeWalkerAuditEvent event) {
+        final DetailAST eventAst = getEventAst(event);
+        DetailAST scopeAst = eventAst;
+        while (scopeAst != null && scopeAst.getType() != TokenTypes.CLASS_DEF
+                && scopeAst.getType() != TokenTypes.INTERFACE_DEF
+                && scopeAst.getType() != TokenTypes.ENUM_DEF
+                && scopeAst.getType() != TokenTypes.RECORD_DEF) {
+            scopeAst = scopeAst.getParent();
+        }
+        return isChangedLineInAstScope(scopeAst);
+    }
+
+    private boolean isChangedLineInAstScope(DetailAST ast) {
+        boolean result = false;
+        if (ast != null) {
+            final Map<String, Integer> childAstLineNoMap = getChildAstLineNo(ast);
+            final int childAstStartLine = childAstLineNoMap.get(MIN);
+            final int childAstEndLine = childAstLineNoMap.get(MAX);
+            result = lineMatching(childAstStartLine, childAstEndLine);
+        }
+        return result;
+    }
+
     /**
      * Is matching by line number.
      *

src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressionPatchFilter.java

@@ -163,7 +163,7 @@ private void loadPatchFile() throws CheckstyleException {
                 final List<List<Integer>> lineRangeList = loadPatchFileUtils.getLineRangeList();
                 final SuppressionPatchFilterElement element =
                         new SuppressionPatchFilterElement(fileName, lineRangeList,
-                                neverSuppressedChecks);
+                                neverSuppressedChecks, strategy);
                 filters.addFilter(element);
             }
         }

src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressionPatchFilterElement.java

@@ -20,8 +20,14 @@
 package com.puppycrawl.tools.checkstyle.filters;
 
 import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.util.List;
 import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import com.puppycrawl.tools.checkstyle.api.AuditEvent;
 import com.puppycrawl.tools.checkstyle.api.Filter;
@@ -32,6 +38,22 @@
  */
 public class SuppressionPatchFilterElement implements Filter {
 
+    /** Check name. */
+    private static final String UNIQUE_PROPERTIES_CHECK = "UniqueProperties";
+
+    /** Check name. */
+    private static final String REGEXP_MULTILINE_CHECK = "RegexpMultiline";
+
+    /** Suffix of check class names. */
+    private static final String CHECK_SUFFIX = "Check";
+
+    /** Escaped new line sequence in violation messages. */
+    private static final String ESCAPED_NEW_LINE = "\\n";
+
+    /** Message pattern to extract duplicated key from UniqueProperties violation. */
+    private static final Pattern UNIQUE_PROPERTIES_KEY_PATTERN =
+            Pattern.compile("Duplicated property '([^']+)'");
+
     /**
      * The String of file names.
      */
@@ -47,6 +69,9 @@ public class SuppressionPatchFilterElement implements Filter {
      */
     private final Set<String> neverSuppressedChecks;
 
+    /** Strategy used to build changed line ranges. */
+    private final Strategy strategy;
+
     /**
      * Constructs a {@code SuppressPatchFilterElement} for a
      * file name pattern.
@@ -55,19 +80,21 @@ public class SuppressionPatchFilterElement implements Filter {
      * @param lineRangeList list of line range for line number filtering
      * @param neverSuppressedChecks set has user defined Checks to never suppress
      *                              if files are touched
+     * @param strategy strategy used to build changed line ranges
      */
     public SuppressionPatchFilterElement(String fileName, List<List<Integer>> lineRangeList,
-                                         Set<String> neverSuppressedChecks) {
+                                         Set<String> neverSuppressedChecks, Strategy strategy) {
         this.fileName = fileName;
         this.lineRangeList = lineRangeList;
         this.neverSuppressedChecks = neverSuppressedChecks;
+        this.strategy = strategy;
     }
 
     @Override
     public boolean accept(AuditEvent event) {
         return isFileNameMatching(event)
                 && (isTreeWalkerChecksMatching(event)
-                || isNeverSuppressCheck(event)
+                || isMatchingByNeverSuppressedCheck(event)
                 || isLineMatching(event));
     }
 
@@ -124,14 +151,30 @@ private static boolean isTreeWalkerChecksMatching(AuditEvent event) {
     private boolean isLineMatching(AuditEvent event) {
         boolean result = false;
         if (event.getViolation() != null) {
-            for (List<Integer> aLineRangeList : lineRangeList) {
-                final int startLine = aLineRangeList.get(0) + 1;
-                final int endLine = aLineRangeList.get(1) + 1;
-                final int currentLine = event.getLine();
+            result = lineMatching(event.getLine());
+        }
+        return result;
+    }
+
+    /**
+     * Check if line intersects any changed line range.
+     *
+     * @param currentLine line number (1-based)
+     * @return true if line belongs to a changed range
+     */
+    private boolean lineMatching(int currentLine) {
+        boolean result = false;
+        for (List<Integer> aLineRangeList : lineRangeList) {
+            final int startLine = aLineRangeList.get(0) + 1;
+            final int endLine = aLineRangeList.get(1) + 1;
+            if (startLine == endLine) {
+                result = currentLine == startLine;
+            }
+            else {
                 result = currentLine >= startLine && currentLine < endLine;
-                if (result) {
-                    break;
-                }
+            }
+            if (result) {
+                break;
             }
         }
         return result;
@@ -154,10 +197,103 @@ private boolean isNeverSuppressCheck(AuditEvent event) {
         return result;
     }
 
+    /**
+     * Apply stricter causality matching for checks configured under neverSuppressedChecks.
+     *
+     * @param event event to process
+     * @return true when violation can be attributed to changed lines in touched file
+     */
+    private boolean isMatchingByNeverSuppressedCheck(AuditEvent event) {
+        boolean result = false;
+        if (isNeverSuppressCheck(event)) {
+            result = isLineMatching(event);
+            if (!result) {
+                final String checkShortName = getCheckShortName(event)
+                        .replaceAll(CHECK_SUFFIX, "");
+                if (UNIQUE_PROPERTIES_CHECK.equals(checkShortName)
+                        && strategy != Strategy.NEWLINE) {
+                    result = isUniquePropertiesViolationCausedByPatch(event);
+                }
+                else if (REGEXP_MULTILINE_CHECK.equals(checkShortName)
+                        && strategy != Strategy.NEWLINE) {
+                    result = isRegexpMultilineViolationCausedByPatch(event);
+                }
+                else {
+                    result = true;
+                }
+            }
+        }
+        return result;
+    }
+
+    /**
+     * UniqueProperties reports first duplicate occurrence line; we attribute it to patch
+     * when any duplicate key occurrence is touched.
+     *
+     * @param event event to process
+     * @return true if changed lines contain any occurrence of duplicated key
+     */
+    private boolean isUniquePropertiesViolationCausedByPatch(AuditEvent event) {
+        boolean result = false;
+        final String message = event.getMessage();
+        if (message != null) {
+            final Matcher matcher = UNIQUE_PROPERTIES_KEY_PATTERN.matcher(message);
+            if (matcher.find()) {
+                final String key = matcher.group(1);
+                try {
+                    final List<String> lines = Files.readAllLines(
+                            Paths.get(event.getFileName()), StandardCharsets.UTF_8);
+                    for (int index = 0; index < lines.size(); index++) {
+                        final String line = lines.get(index).trim();
+                        if (line.startsWith(key + "=") || line.startsWith(key + " =")) {
+                            if (lineMatching(index + 1)) {
+                                result = true;
+                                break;
+                            }
+                        }
+                    }
+                }
+                catch (IOException ioException) {
+                    result = false;
+                }
+            }
+        }
+        return result;
+    }
+
+    /**
+     * RegexpMultiline can report the first line of a multi-line match.
+     * We expand matching window by the number of line breaks in the pattern.
+     *
+     * @param event event to process
+     * @return true if any line in match window intersects patch lines
+     */
+    private boolean isRegexpMultilineViolationCausedByPatch(AuditEvent event) {
+        final String message = event.getMessage();
+        int linesInMatch = 1;
+        if (message != null) {
+            int index = message.indexOf(ESCAPED_NEW_LINE);
+            while (index != -1) {
+                linesInMatch++;
+                index = message.indexOf(ESCAPED_NEW_LINE, index + ESCAPED_NEW_LINE.length());
+            }
+        }
+
+        boolean result = false;
+        final int startLine = event.getLine();
+        for (int lineOffset = 0; lineOffset < linesInMatch; lineOffset++) {
+            if (lineMatching(startLine + lineOffset)) {
+                result = true;
+                break;
+            }
+        }
+        return result;
+    }
+
     private static boolean containsShortName(Set<String> checkNameSet,
                                       AuditEvent event) {
         final String checkShortName = getCheckShortName(event);
-        final String shortName = checkShortName.replaceAll("Check", "");
+        final String shortName = checkShortName.replaceAll(CHECK_SUFFIX, "");
         return checkNameSet.contains(checkShortName)
                 || checkNameSet.contains(shortName);
 

src/test/java/com/puppycrawl/tools/checkstyle/filters/SuppressionJavaPatchFilterTest.java

@@ -76,16 +76,24 @@ public void testNonExistentPatchFileWithTrueOptional() throws Exception {
     public void testNeverSuppressedChecks() throws Exception {
         testByConfig("neversuppressedchecks/CovariantEquals/"
                 + "checkID/context/defaultContextConfig.xml");
+        testByConfig("neversuppressedchecks/CovariantEquals/"
+                + "checkID/context/case2/defaultContextConfig.xml");
         testByConfig("neversuppressedchecks/CovariantEquals/"
                 + "checkID/newline/defaultContextConfig.xml");
         testByConfig("neversuppressedchecks/CovariantEquals"
                         + "/checkID/patchedline/defaultContextConfig.xml");
+        testByConfig("neversuppressedchecks/CovariantEquals"
+                + "/checkID/patchedline/case2/defaultContextConfig.xml");
         testByConfig("neversuppressedchecks/CovariantEquals/"
                 + "checkShortName/context/defaultContextConfig.xml");
+        testByConfig("neversuppressedchecks/CovariantEquals/"
+                + "checkShortName/context/case2/defaultContextConfig.xml");
         testByConfig("neversuppressedchecks/CovariantEquals/"
                 + "checkShortName/newline/defaultContextConfig.xml");
         testByConfig("neversuppressedchecks/CovariantEquals/"
                 + "checkShortName/patchedline/defaultContextConfig.xml");
+        testByConfig("neversuppressedchecks/CovariantEquals/"
+                + "checkShortName/patchedline/case2/defaultContextConfig.xml");
     }
 
     @Test

src/test/resources/com/puppycrawl/tools/checkstyle/filters/suppressionjavapatchfilter/neversuppressedchecks/CovariantEquals/checkID/context/case2/Test.java

@@ -0,0 +1,9 @@
+package TreeWalker.coding.CovariantEquals;
+
+// patched line outside any type
+public class Test {
+    public boolean equals(TreeWalker.coding.CovariantEquals.Test i) {  // violation without filter
+        return false;
+    }
+
+}

src/test/resources/com/puppycrawl/tools/checkstyle/filters/suppressionjavapatchfilter/neversuppressedchecks/CovariantEquals/checkID/context/case2/defaultContext.patch

@@ -0,0 +1,10 @@
+diff --git a/Test.java b/Test.java
+index 9af4e74..d6706f3 100644
+--- a/Test.java
++++ b/Test.java
+@@ -1,5 +1,6 @@
+ package TreeWalker.coding.CovariantEquals;
++// patched line outside any type
+ public class Test {
+     public boolean equals(TreeWalker.coding.CovariantEquals.Test i) {  // violation without filter
+         return false;

src/test/resources/com/puppycrawl/tools/checkstyle/filters/suppressionjavapatchfilter/neversuppressedchecks/CovariantEquals/checkID/context/case2/defaultContextConfig.xml

@@ -0,0 +1,18 @@
+<?xml version="1.0"?>
+<!DOCTYPE module PUBLIC
+          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
+          "https://checkstyle.org/dtds/configuration_1_3.dtd">
+<module name="Checker">
+    <module name="TreeWalker">
+        <module name="CovariantEquals">
+            <property name="id" value="covariantEquals"/>
+        </module>
+
+        <module name="com.puppycrawl.tools.checkstyle.filters.SuppressionJavaPatchFilter">
+            <property name="file" value="${tp}/defaultContext.patch" />
+            <property name="strategy" value="context" />
+            <property name="neverSuppressedChecks" value="covariantEquals" />
+        </module>
+    </module>
+</module>
+

src/test/resources/com/puppycrawl/tools/checkstyle/filters/suppressionjavapatchfilter/neversuppressedchecks/CovariantEquals/checkID/patchedline/case2/Test.java

@@ -0,0 +1,9 @@
+package TreeWalker.coding.CovariantEquals;
+
+// patched line outside any type
+public class Test {
+    public boolean equals(TreeWalker.coding.CovariantEquals.Test i) {  // violation without filter
+        return false;
+    }
+
+}

src/test/resources/com/puppycrawl/tools/checkstyle/filters/suppressionjavapatchfilter/neversuppressedchecks/CovariantEquals/checkID/patchedline/case2/defaultContext.patch

@@ -0,0 +1,10 @@
+diff --git a/Test.java b/Test.java
+index 9af4e74..d6706f3 100644
+--- a/Test.java
++++ b/Test.java
+@@ -1,5 +1,6 @@
+ package TreeWalker.coding.CovariantEquals;
++// patched line outside any type
+ public class Test {
+     public boolean equals(TreeWalker.coding.CovariantEquals.Test i) {  // violation without filter
+         return false;