Issue #14689: Fix Javadoc summary period handling

checkstyle · Apr 25, 2024 · cc00495 · cc00495
1 parent 114a9df
commit cc00495
Show file tree

Hide file tree

Showing 9 changed files with 187 additions and 51 deletions.
diff --git a/...m/google/checkstyle/test/chapter7javadoc/rule72thesummaryfragment/SummaryJavadocTest.java b/...m/google/checkstyle/test/chapter7javadoc/rule72thesummaryfragment/SummaryJavadocTest.java
@@ -54,6 +54,7 @@ public void testIncorrect() throws Exception {
             "summary.javaDoc.missing");
 
         final String[] expected = {
+            "9: " + msgFirstSentence,
             "14: " + msgMissingDoc,
             "32: " + msgMissingDoc,
             "37: " + msgFirstSentence,

diff --git a/...tyle/test/chapter7javadoc/rule72thesummaryfragment/InputIncorrectSummaryJavaDocCheck.java b/...tyle/test/chapter7javadoc/rule72thesummaryfragment/InputIncorrectSummaryJavaDocCheck.java
@@ -6,7 +6,7 @@
  */
 class InputIncorrectSummaryJavaDocCheck {
 
-    /**
+/*warn*//**
      * As of JDK 1.1, replaced by {@link #setBounds(int,int,int,int)}
      */
     void foo3() {}

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java
@@ -21,6 +21,8 @@
 
 import java.util.Arrays;
 import java.util.BitSet;
+import java.util.Deque;
+import java.util.LinkedList;
 import java.util.Optional;
 import java.util.regex.Pattern;
 
@@ -48,7 +50,10 @@
  * Default value is {@code "^$"}.
  * </li>
  * <li>
- * Property {@code period} - Specify the period symbol at the end of first javadoc sentence.
+ * Property {@code period} - Specify the period symbol. Used to check the first sentence ends with a
+ * period. Periods that are not followed by a whitespace character are ignored (eg. the period in
+ * v1.0). Because some periods include whitespace built into the character, if this is set to a
+ * non-default value any period will end the sentence, whether it is followed by whitespace or not.
  * Type is {@code java.lang.String}.
  * Default value is {@code "."}.
  * </li>
@@ -154,7 +159,10 @@ public class SummaryJavadocCheck extends AbstractJavadocCheck {
     private Pattern forbiddenSummaryFragments = CommonUtil.createPattern("^$");
 
     /**
-     * Specify the period symbol at the end of first javadoc sentence.
+     * Specify the period symbol. Used to check the first sentence ends with a period. Periods that
+     * are not followed by a whitespace character are ignored (eg. the period in v1.0). Because some
+     * periods include whitespace built into the character, if this is set to a non-default value
+     * any period will end the sentence, whether it is followed by whitespace or not.
      */
     private String period = DEFAULT_PERIOD;
 
@@ -169,7 +177,11 @@ public void setForbiddenSummaryFragments(Pattern pattern) {
     }
 
     /**
-     * Setter to specify the period symbol at the end of first javadoc sentence.
+     * Setter to specify the period symbol. Used to check the first sentence ends with a period.
+     * Periods that are not followed by a whitespace character are ignored (eg. the period in v1.0).
+     * Because some periods include whitespace built into the character, if this is set to a
+     * non-default value any period will end the sentence, whether it is followed by whitespace or
+     * not.
      *
      * @param period period's value.
      * @since 6.2
@@ -218,13 +230,11 @@ private void validateUntaggedSummary(DetailNode ast) {
             log(ast.getLineNumber(), MSG_SUMMARY_JAVADOC_MISSING);
         }
         else if (!period.isEmpty()) {
-            final String firstSentence = getFirstSentence(ast);
-            final int endOfSentence = firstSentence.lastIndexOf(period);
-            if (!summaryDoc.contains(period)) {
+            final String firstSentence = getFirstSentence(ast, period);
+            if (!summaryDoc.contains(period) || firstSentence.isEmpty()) {
                 log(ast.getLineNumber(), MSG_SUMMARY_FIRST_SENTENCE);
             }
-            if (endOfSentence != -1
-                    && containsForbiddenFragment(firstSentence.substring(0, endOfSentence))) {
+            else if (containsForbiddenFragment(firstSentence)) {
                 log(ast.getLineNumber(), MSG_SUMMARY_JAVADOC);
             }
         }
@@ -579,28 +589,65 @@ private static String getStringInsideTag(String result, DetailNode detailNode) {
      * Finds and returns first sentence.
      *
      * @param ast Javadoc root node.
+     * @param period Period character.
      * @return first sentence.
      */
-    private static String getFirstSentence(DetailNode ast) {
+    private static String getFirstSentence(DetailNode ast, String period) {
+        final Deque<DetailNode> stack = new LinkedList<>();
+        stack.push(ast);
         final StringBuilder result = new StringBuilder(256);
-        final String periodSuffix = DEFAULT_PERIOD + ' ';
-        for (DetailNode child : ast.getChildren()) {
-            final String text;
-            if (child.getChildren().length == 0) {
-                text = child.getText();
+        boolean foundPeriod = false;
+        while (!stack.isEmpty()) {
+            final DetailNode node = stack.pop();
+            if (node.getChildren().length == 0
+                && appendUpToSentenceEndingPeriod(node.getText(), period, result)) {
+                foundPeriod = true;
+                break;
             }
-            else {
-                text = getFirstSentence(child);
+            // Pushing last child first means it will be processed last
+            for (int childIndex = node.getChildren().length - 1; childIndex >= 0; childIndex--) {
+                stack.push(node.getChildren()[childIndex]);
             }
+        }
+        if (!foundPeriod) {
+            result.setLength(0);
+        }
+        return result.toString();
+    }
 
-            if (text.contains(periodSuffix)) {
-                result.append(text, 0, text.indexOf(periodSuffix) + 1);
+    /**
+     * Find the end of a sentence, and append the sentence to the result if it ends with a period.
+     * If no period is present, append the whole string to the result. The end of sentence detection
+     * here could be replaced in the future by Java's built-in BreakIterator class.
+     *
+     * @param text string to append to result
+     * @param period period character to find
+     * @param result builder to append to
+     * @return true if a sentence ending period was found, false otherwise
+     */
+    private static boolean appendUpToSentenceEndingPeriod(
+            String text, String period, StringBuilder result) {
+        int periodIndex = text.indexOf(period);
+        boolean foundPeriod = false;
+        while (periodIndex >= 0) {
+            final int afterPeriodIndex = periodIndex + period.length();
+
+            // Handle western period separately as it is only the end of a sentence if followed
+            // by whitespace. Other period characters often include whitespace in the character.
+            if (!DEFAULT_PERIOD.equals(period)
+                || afterPeriodIndex >= text.length()
+                || Character.isWhitespace(text.charAt(afterPeriodIndex))) {
+                result.append(text, 0, periodIndex);
+                foundPeriod = true;
                 break;
             }
-
+            else {
+                periodIndex = text.indexOf(period, afterPeriodIndex);
+            }
+        }
+        if (!foundPeriod) {
             result.append(text);
         }
-        return result.toString();
+        return foundPeriod;
     }
-
 }
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheckTest.java
@@ -68,21 +68,22 @@ public void testInlineCorrect() throws Exception {
     @Test
     public void testIncorrect() throws Exception {
         final String[] expected = {
-            "24: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "42: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "47: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
-            "57: " + getCheckMessage(MSG_SUMMARY_JAVADOC),
-            "63: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "68: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "79: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "93: " + getCheckMessage(MSG_SUMMARY_JAVADOC),
-            "113: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "126: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
-            "131: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "136: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "142: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
-            "147: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "150: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "20: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
+            "25: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "43: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "48: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
+            "58: " + getCheckMessage(MSG_SUMMARY_JAVADOC),
+            "64: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "69: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "80: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "94: " + getCheckMessage(MSG_SUMMARY_JAVADOC),
+            "114: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "127: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
+            "132: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "137: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "143: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
+            "148: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "151: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
         };
         verifyWithInlineConfigParser(
                 getPath("InputSummaryJavadocIncorrect.java"), expected);
@@ -130,19 +131,20 @@ public void testNoPeriod() throws Exception {
     @Test
     public void testDefaultConfiguration() throws Exception {
         final String[] expected = {
-            "23: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "41: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "46: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
-            "62: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "67: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "78: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "112: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "125: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
-            "130: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "135: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "141: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
-            "146: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
-            "149: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "19: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
+            "24: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "42: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "47: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
+            "63: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "68: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "79: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "113: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "126: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
+            "131: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "136: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "142: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
+            "147: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
+            "150: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
         };
 
         verifyWithInlineConfigParser(
@@ -230,12 +232,31 @@ public void testPeriodAtEnd() throws Exception {
             "33: " + getCheckMessage(MSG_SUMMARY_JAVADOC_MISSING),
             "40: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
             "60: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
+            "70: " + getCheckMessage(MSG_SUMMARY_FIRST_SENTENCE),
         };
 
         verifyWithInlineConfigParser(
                 getPath("InputSummaryJavadocPeriodAtEnd.java"), expected);
     }
 
+    @Test
+    public void testForbiddenFragmentRelativeToPeriod() throws Exception {
+        final String[] expected = {
+            "23: " + getCheckMessage(MSG_SUMMARY_JAVADOC),
+        };
+
+        verifyWithInlineConfigParser(
+                getPath("InputSummaryJavadocForbiddenFragmentRelativeToPeriod.java"), expected);
+    }
+
+    @Test
+    public void testJapanesePeriod() throws Exception {
+        final String[] expected = CommonUtil.EMPTY_STRING_ARRAY;
+
+        verifyWithInlineConfigParser(
+                getPath("InputSummaryJavadocJapanesePeriod.java"), expected);
+    }
+
     @Test
     public void testHtmlFormatSummary() throws Exception {
         final String[] expected = {

diff --git a/...e/checks/javadoc/summaryjavadoc/InputSummaryJavadocForbiddenFragmentRelativeToPeriod.java b/...e/checks/javadoc/summaryjavadoc/InputSummaryJavadocForbiddenFragmentRelativeToPeriod.java
@@ -0,0 +1,28 @@
+/*
+SummaryJavadoc
+violateExecutionOnNonTightHtml = (default)false
+forbiddenSummaryFragments = ^$|fail-summary-fragment
+period = 。
+
+
+*/
+
+package com.puppycrawl.tools.checkstyle.checks.javadoc.summaryjavadoc;
+
+public class InputSummaryJavadocForbiddenFragmentRelativeToPeriod {
+
+    /**
+     * Summary sentence on its own line。
+     * <p>
+     * Another sentence that is not part of the summary,
+     * so this should not matter: fail-summary-fragment。
+     */
+    void foo1() {}
+
+    // violation below 'Forbidden summary fragment.'
+    /**
+     * Summary sentence containing default period mentioning version 1.1, then ending with correct
+     * period after disallowed words, fail-summary-fragment。
+     */
+    void foo2() {}
+}
diff --git a/...ppycrawl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocIncorrect.java b/...ppycrawl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocIncorrect.java
@@ -16,6 +16,7 @@
  */
 class InputSummaryJavadocIncorrect {
 
+    // violation below 'First sentence .* missing an ending period.'
     /**
      * As of JDK 1.1, replaced by {@link #setBounds(int,int,int,int)}
      */

diff --git a/...pycrawl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocIncorrect2.java b/...pycrawl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocIncorrect2.java
@@ -15,6 +15,7 @@
  */
 class InputSummaryJavadocIncorrect2 {
 
+    // violation below 'First sentence .* missing an ending period.'
     /**
      * As of JDK 1.1, replaced by {@link #setBounds(int,int,int,int)}
      */

diff --git a/...awl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocJapanesePeriod.java b/...awl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocJapanesePeriod.java
@@ -0,0 +1,24 @@
+/*
+SummaryJavadoc
+violateExecutionOnNonTightHtml = (default)false
+forbiddenSummaryFragments = (default)^$
+period = 。
+
+
+*/
+
+package com.puppycrawl.tools.checkstyle.checks.javadoc.summaryjavadoc;
+
+public class InputSummaryJavadocJapanesePeriod {
+
+    /**
+     * Summary sentence ending with correct period and no following whitespace。The Japanese
+     * period has whitespace built in!
+     */
+    void foo1() {}
+
+    /**
+     * 要約文。別の文。
+     */
+    void foo2() {}
+}
diff --git a/...ycrawl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocPeriodAtEnd.java b/...ycrawl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocPeriodAtEnd.java
@@ -12,7 +12,7 @@
 
 public class InputSummaryJavadocPeriodAtEnd {
     /**
-     * JAXB 1.0 only default validation event handler
+     * JAXB 1.0 only default validation event handler.
      */
     public static final byte NUL = 0;
     // violation below 'Summary javadoc is missing.'
@@ -65,5 +65,18 @@ public void foo5(){
      */
     public void foo6() {
 
+    }
+    // violation below 'First sentence .* missing an ending period.'
+    /**
+     * JAXB 1.0 missing end period
+     */
+    public void foo7() {
+
+    }
+    /**
+     *.period at beginning of line, then summary sentence.
+     */
+    public void foo8() {
+
     }
 }