Issue #335: Checks under neverSuppressedChecks group should give more precise violations

Author: Brijeshthummar02

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

@@ -52,6 +52,10 @@ public final 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));
@@ -176,13 +180,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));
         }
 
@@ -228,6 +232,79 @@ private boolean isNeverSuppressCheck(TreeWalkerAuditEvent event) {
         return result;
     }
 
+    /**
+     * Apply stricter causality matching for checks configured under neverSuppressedChecks.
+     *
+     * <p>Most checks report violations on the same line as the change, so the existing
+     * line matching logic works for them. However, some checks have violations whose
+     * causality is at a broader scope than the violation line itself:
+     *
+     * <ul>
+     * <li>{@code CovariantEquals} - violation is reported on method AST, but causality
+     * is type-level (missing {@code equals(Object)} in that class), so we attribute by
+     * enclosing type scope</li>
+     * </ul>
+     *
+     * <p>For all other checks, the fallback still applies and shows violations in touched
+     * files, so nothing is missed. As more checks with similar behavior are discovered,
+     * they can be added here with appropriate causality attribution logic.
+     *
+     * @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;
+    }
+
+    /**
+     * Checks whether any changed line falls within the enclosing type of the event.
+     *
+     * @param event audit event
+     * @return true if a changed line is inside the enclosing type scope
+     */
+    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);
+    }
+
+    /**
+     * Checks whether any changed line falls within the provided AST scope.
+     *
+     * @param ast enclosing AST node to evaluate
+     * @return true if a changed line is inside the AST scope
+     */
+    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

@@ -174,8 +174,8 @@ private void loadPatchFile() throws CheckstyleException {
                 final List<List<Integer>> lineRangeList =
                         loadPatchFileUtils.getLineRangeList();
                 final SuppressionPatchFilterElement element =
-                        new SuppressionPatchFilterElement(fileName,
-                                lineRangeList, neverSuppressedChecks);
+                        new SuppressionPatchFilterElement(fileName, lineRangeList,
+                                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 final 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,28 +69,32 @@ public final 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.
      *
-     * @param fileNameValue      names of filtered files
-     * @param lineRangeListValue list of line range for line number filtering
-     * @param neverSuppressedChecksValue set has user defined Checks to never
-     *                                   suppress if files are touched
+     * @param fileName      names of filtered files
+     * @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 fileNameValue,
-                                         List<List<Integer>> lineRangeListValue,
-                                         Set<String> neverSuppressedChecksValue) {
-        this.fileName = fileNameValue;
-        this.lineRangeList = lineRangeListValue;
-        this.neverSuppressedChecks = neverSuppressedChecksValue;
+    public SuppressionPatchFilterElement(String fileName, List<List<Integer>> lineRangeList,
+                                         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));
     }
 
@@ -126,14 +152,30 @@ private static boolean isTreeWalkerChecksMatching(AuditEvent event) {
     private boolean isLineMatching(AuditEvent event) {
         boolean result = false;
         if (event.getViolation() != null) {
-            for (List<Integer> lineRange : lineRangeList) {
-                final int startLine = lineRange.get(0) + 1;
-                final int endLine = lineRange.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> lineRange : lineRangeList) {
+            final int startLine = lineRange.get(0) + 1;
+            final int endLine = lineRange.get(1) + 1;
+            if (startLine == endLine) {
+                result = currentLine == startLine;
+            }
+            else {
                 result = currentLine >= startLine && currentLine < endLine;
-                if (result) {
-                    break;
-                }
+            }
+            if (result) {
+                break;
             }
         }
         return result;
@@ -156,6 +198,114 @@ private boolean isNeverSuppressCheck(AuditEvent event) {
         return result;
     }
 
+    /**
+     * Apply stricter causality matching for checks configured under neverSuppressedChecks.
+     *
+     * <p>Most checks report violations on the same line as the change, so the existing
+     * line matching logic works for them. However, a small number of checks report
+     * violations on a different line than the actual change, requiring special handling:
+     *
+     * <ul>
+     * <li>{@code UniqueProperties} - reports on first duplicate occurrence, not necessarily
+     * the changed line</li>
+     * <li>{@code RegexpMultiline} - reports on first line of multi-line match, change may
+     * be on other lines</li>
+     * </ul>
+     *
+     * <p>For all other checks, the fallback still applies and shows violations in touched
+     * files, so nothing is missed. As more checks with similar behavior are discovered,
+     * they can be added here with appropriate causality attribution logic.
+     *
+     * @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;
+    }
+
     /**
      * Checks if the check name set contains the event's check short name.
      *
@@ -166,7 +316,7 @@ private boolean isNeverSuppressCheck(AuditEvent event) {
     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

@@ -74,16 +74,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;
+    }
+
+}