8278068: Fix next-line modifier (snippet markup)

8277027: Treat unrecognized markup as snippet text, but warn about it

Reviewed-by: jjg

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties

@@ -373,6 +373,8 @@ doclet.snippet.contents.mismatch=\
 doclet.snippet.markup=\
  snippet markup: {0}
 
+doclet.snippet.markup.spurious=\
+  spurious markup
 doclet.snippet.markup.attribute.absent=\
   missing attribute "{0}"
 doclet.snippet.markup.attribute.simultaneous.use=\

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java

@@ -269,8 +269,14 @@ public class SnippetTaglet extends BaseTaglet {
         StyledText externalSnippet = null;
 
         try {
+            Diags d = (text, pos) -> {
+                var path = writer.configuration().utils.getCommentHelper(holder)
+                        .getDocTreePath(snippetTag.getBody());
+                writer.configuration().getReporter().print(Diagnostic.Kind.WARNING,
+                        path, pos, pos, pos, text);
+            };
             if (inlineContent != null) {
-                inlineSnippet = parse(writer.configuration().getDocResources(), language, inlineContent);
+                inlineSnippet = parse(writer.configuration().getDocResources(), d, language, inlineContent);
             }
         } catch (ParseException e) {
             var path = writer.configuration().utils.getCommentHelper(holder)
@@ -284,8 +290,10 @@ public class SnippetTaglet extends BaseTaglet {
         }
 
         try {
+            var finalFileObject = fileObject;
+            Diags d = (text, pos) -> writer.configuration().getMessages().warning(finalFileObject, pos, pos, pos, text);
             if (externalContent != null) {
-                externalSnippet = parse(writer.configuration().getDocResources(), language, externalContent);
+                externalSnippet = parse(writer.configuration().getDocResources(), d, language, externalContent);
             }
         } catch (ParseException e) {
             assert fileObject != null;
@@ -363,12 +371,16 @@ public class SnippetTaglet extends BaseTaglet {
                """.formatted(inline, external);
     }
 
-    private StyledText parse(Resources resources, Optional<Language> language, String content) throws ParseException {
+    private StyledText parse(Resources resources, Diags diags, Optional<Language> language, String content) throws ParseException {
-        Parser.Result result = new Parser(resources).parse(language, content);
+        Parser.Result result = new Parser(resources).parse(diags, language, content);
         result.actions().forEach(Action::perform);
         return result.text();
     }
 
+    public interface Diags {
+        void warn(String text, int pos);
+    }
+
     private static String stringValueOf(AttributeTree at) throws BadSnippetException {
         if (at.getValueKind() == AttributeTree.ValueKind.EMPTY) {
             throw new BadSnippetException(at, "doclet.tag.attribute.value.missing",

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/MarkupParser.java

@@ -31,11 +31,8 @@ import java.util.List;
 import jdk.javadoc.internal.doclets.toolkit.Resources;
 
 //
-// markup-comment = { markup-tag } ;
+// markup-comment = { markup-tag } [":"] ;
-//     markup-tag = "@" , tag-name , {attribute} [":"] ;
+//     markup-tag = "@" , tag-name , {attribute} ;
-//
-// If optional trailing ":" is present, the tag refers to the next line
-// rather than to this line.
 //
 
 /**
@@ -76,15 +73,28 @@ public final class MarkupParser {
     }
 
     protected List<Parser.Tag> parse() throws ParseException {
+        List<Parser.Tag> tags = readTags();
+        if (ch == ':') {
+            tags.forEach(t -> t.appliesToNextLine = true);
+            nextChar();
+        }
+        skipWhitespace();
+        if (ch != EOI) {
+            return List.of();
+        }
+        return tags;
+    }
+
+    protected List<Parser.Tag> readTags() throws ParseException {
         List<Parser.Tag> tags = new ArrayList<>();
-        // TODO: what to do with leading and trailing unrecognized markup?
+        skipWhitespace();
         while (bp < buflen) {
-            switch (ch) {
+            if (ch == '@') {
-                case '@' -> tags.add(readTag());
+                tags.add(readTag());
-                default -> nextChar();
+            } else {
+                break;
             }
         }
-
         return tags;
     }
 
@@ -94,26 +104,13 @@ public final class MarkupParser {
         String name = readIdentifier();
         skipWhitespace();
 
-        boolean appliesToNextLine = false;
+        List<Attribute> attributes = attrs();
-        List<Attribute> attributes = List.of();
+        skipWhitespace();
-
-        if (ch == ':') {
-            appliesToNextLine = true;
-            nextChar();
-        } else {
-            attributes = attrs();
-            skipWhitespace();
-            if (ch == ':') {
-                appliesToNextLine = true;
-                nextChar();
-            }
-        }
 
         Parser.Tag i = new Parser.Tag();
         i.nameLineOffset = nameBp;
         i.name = name;
         i.attributes = attributes;
-        i.appliesToNextLine = appliesToNextLine;
 
         return i;
     }

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java

@@ -94,19 +94,19 @@ public final class Parser {
         this.markupParser = new MarkupParser(resources);
     }
 
-    public Result parse(Optional<SnippetTaglet.Language> language, String source) throws ParseException {
+    public Result parse(SnippetTaglet.Diags diags, Optional<SnippetTaglet.Language> language, String source) throws ParseException {
         SnippetTaglet.Language lang = language.orElse(SnippetTaglet.Language.JAVA);
         var p = switch (lang) {
             case JAVA -> JAVA_COMMENT;
             case PROPERTIES -> PROPERTIES_COMMENT;
         };
-        return parse(p, source);
+        return parse(diags, p, source);
     }
 
     /*
      * Newline characters in the returned text are of the \n form.
      */
-    private Result parse(Pattern commentPattern, String source) throws ParseException {
+    private Result parse(SnippetTaglet.Diags diags, Pattern commentPattern, String source) throws ParseException {
         Objects.requireNonNull(commentPattern);
         Objects.requireNonNull(source);
 
@@ -150,7 +150,7 @@ public final class Parser {
                     parsedTags = markupParser.parse(maybeMarkup);
                 } catch (ParseException e) {
                     // translate error position from markup to file line
-                    throw new ParseException(e::getMessage, markedUpLine.start("markup") + e.getPosition());
+                    throw new ParseException(e::getMessage, next.offset() + markedUpLine.start("markup") + e.getPosition());
                 }
                 for (Tag t : parsedTags) {
                     t.lineSourceOffset = next.offset();
@@ -166,7 +166,7 @@ public final class Parser {
                     }
                 }
                 if (parsedTags.isEmpty()) { // (2)
-                    // TODO: log this with NOTICE;
+                    diags.warn(resources.getText("doclet.snippet.markup.spurious"), next.offset() + markedUpLine.start("markup"));
                     line = rawLine + (addLineTerminator ? "\n" : "");
                 } else { // (3)
                     hasMarkup = true;

test/langtools/jdk/javadoc/doclet/testSnippetTag/SnippetTester.java

@@ -26,9 +26,11 @@ import java.io.UncheckedIOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
+import java.nio.file.attribute.BasicFileAttributes;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
+import java.util.function.BiPredicate;
 import java.util.function.ObjIntConsumer;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -122,4 +124,46 @@ public class SnippetTester extends JavadocTester {
                 <pre class="snippet"%s><code%s>%s</code></pre>
                 </div>""".formatted(svgString, idString, langString, content);
     }
+
+    // There's JavadocTester.diff(), but its semantics is different; hence we
+    // use this method.
+    protected void match(Path path1, Path path2, BiPredicate<Path, BasicFileAttributes> filter) throws IOException {
+        checking("diff " + path1 + ", " + path2);
+        try (var paths1 = Files.find(path1, Integer.MAX_VALUE, filter).sorted();
+             var paths2 = Files.find(path2, Integer.MAX_VALUE, filter).sorted()) {
+            var it1 = paths1.iterator();
+            var it2 = paths2.iterator();
+            while (true) {
+                if (it1.hasNext() != it2.hasNext()) {
+                    failed(it1.hasNext() ? it1.next() : it2.next(), "missing");
+                    return;
+                }
+                if (!it1.hasNext()) {
+                    passed("match");
+                    return;
+                }
+                Path next1 = it1.next();
+                Path next2 = it2.next();
+                if (!path1.relativize(next1).equals(path2.relativize(next2))) {
+                    // compare directory tree to see the difference
+                    failed("mismatching names %s %s".formatted(next1, next2));
+                    return;
+                }
+                if (Files.isDirectory(next1) != Files.isDirectory(next2)) {
+                    // it'd be surprising to ever see this
+                    failed("mismatching types %s %s".formatted(next1, next2));
+                    return;
+                }
+                if (Files.isDirectory(next1)) {
+                    continue;
+                }
+                if (Files.size(next1) != Files.size(next2)
+                        || Files.mismatch(next1, next2) != -1L) {
+                    failed("mismatching contents: diff %s %s".formatted(next1.toAbsolutePath(),
+                            next2.toAbsolutePath()));
+                    return;
+                }
+            }
+        }
+    }
 }

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java

@@ -603,6 +603,115 @@ First line // @highlight :
         testPositive(base, testCases);
     }
 
+    @Test
+    public void testPositiveInlineTagMarkup_FalseMarkup(Path base) throws Exception {
+        var testCases = List.of(
+                new TestCase(
+                        """
+                        First line
+                        // @formatter:off
+                          Second Line
+                            Third line
+                            // @formatter:on
+                              Fourth line
+                        """,
+                        """
+                        First line
+                        // @formatter:off
+                          Second Line
+                            Third line
+                            // @formatter:on
+                              Fourth line
+                        """),
+                new TestCase("showThis",
+                        """
+                        First line
+                        // @formatter:off
+                          // @start region=showThis
+                          Second Line
+                            Third line
+                            // @end region
+                            // @formatter:on
+                              Fourth line
+                        """,
+                        """
+                        Second Line
+                          Third line
+                        """)
+        );
+        testPositive(base, testCases);
+    }
+
+    @Test
+    public void testPositiveInlineTagMarkup_NextLineTwoTags(Path base) throws Exception {
+        var firstTag = new String[]{
+                "@highlight string=firstWord",
+                "@replace string=secondWord replacement=replacedSecondWord",
+                "@link substring=firstWord target=java.lang.Object"};
+        var secondTag = new String[]{
+                "@highlight string=secondWord",
+                "@replace string=firstWord replacement=replacedFirstWord",
+                "@link substring=secondWord target=java.lang.Thread"};
+        List<TestCase> testCases = new ArrayList<>();
+        for (var f : firstTag) {
+            for (var s : secondTag)
+                for (var separator : List.of("", " ")) {
+                    var t = new TestCase(
+                            """
+                                first-line // %s %s%s:
+                                firstWord secondWord thirdWord
+                                """.formatted(f, s, separator),
+                            """
+                                first-line
+                                firstWord secondWord thirdWord // %s %s
+                                """.formatted(f, s));
+                    testCases.add(t);
+            }
+        }
+        testEquivalence(base, testCases);
+    }
+
+    record Snippet(String region, String snippet) { }
+
+    private void testEquivalence(Path base, List<TestCase> testCases) throws IOException {
+        // group all the testcases in just two runs
+        Path out1 = base.resolve("out1");
+        Path out2 = base.resolve("out2");
+        run(base.resolve("src1"), out1, testCases.stream().map(t -> new Snippet(t.region(), t.input())).toList());
+        run(base.resolve("src2"), out2, testCases.stream().map(t -> new Snippet(t.region(), t.expectedOutput())).toList());
+        match(out1, out2, (p, a) -> /* p.toString().endsWith(".html") */ true);
+    }
+
+    private void run(Path source, Path target, List<Snippet> snippets) throws IOException {
+        StringBuilder methods = new StringBuilder();
+        forEachNumbered(snippets, (i, n) -> {
+            String r = i.region.isBlank() ? "" : "region=" + i.region;
+            var methodDef = """
+
+                    /**
+                    {@snippet %s:
+                    %s}*/
+                    public void case%s() {}
+                    """.formatted(r, i.snippet(), n);
+            methods.append(methodDef);
+        });
+        var classDef = """
+                public class A {
+                %s
+                }
+                """.formatted(methods.toString());
+        Path src = Files.createDirectories(source);
+        tb.writeJavaFiles(src, classDef);
+        javadoc("-d", target.toString(),
+                "--limit-modules", "java.base",
+                "-quiet", "-nohelp", "-noindex", "-nonavbar", "-nosince",
+                "-notimestamp", "-notree", "-Xdoclint:none",
+                "-sourcepath", src.toString(),
+                src.resolve("A.java").toString());
+        checkExit(Exit.OK);
+        checkNoCrashes();
+    }
+
     private static String link(boolean linkPlain,
                                String targetReference,
                                String content)

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java

@@ -2418,11 +2418,12 @@ error: snippet markup: invalid attribute value
                 /* ---------------------- */
                 new TestCase("""
 {@snippet :
-    hello // @highlight substring="
+    hello
+    there // @highlight substring="
 }""",
                              """
 error: snippet markup: unterminated attribute value
-    hello // @highlight substring="
+    there // @highlight substring="
                                   ^
                              """),
                 new TestCase("""