Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
8278068: Fix next-line modifier (snippet markup) 
8277027: Treat unrecognized markup as snippet text, but warn about it

Reviewed-by: jjg
  • Loading branch information
@pavelrappo
pavelrappo committed Dec 7, 2021
1 parent 061017a commit a8a1fbc
Show file tree
Hide file tree
Showing 7 changed files with 201 additions and 36 deletions.
2 changes: 2 additions & 0 deletions ...k.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties
View file
Expand Up @@ -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=\
Expand Down
20 changes: 16 additions & 4 deletions ...jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java
View file
Expand Up @@ -269,8 +269,14 @@ private Content generateContent(Element holder, DocTree tag, TagletWriter writer
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)
Expand All @@ -284,8 +290,10 @@ private Content generateContent(Element holder, DocTree tag, TagletWriter writer
}

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;
Expand Down Expand Up @@ -363,12 +371,16 @@ private static String diff(String inline, String external) {
""".formatted(inline, external);
}

private StyledText parse(Resources resources, Optional<Language> language, String content) throws ParseException {
Parser.Result result = new Parser(resources).parse(language, content);
private StyledText parse(Resources resources, Diags diags, Optional<Language> language, String content) throws ParseException {
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",
Expand Down
47 changes: 22 additions & 25 deletions ...adoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/MarkupParser.java
View file
Expand Up @@ -31,11 +31,8 @@
import jdk.javadoc.internal.doclets.toolkit.Resources;

//
// markup-comment = { markup-tag } ;
// markup-tag = "@" , tag-name , {attribute} [":"] ;
//
// If optional trailing ":" is present, the tag refers to the next line
// rather than to this line.
// markup-comment = { markup-tag } [":"] ;
// markup-tag = "@" , tag-name , {attribute} ;
//

/**
Expand Down Expand Up @@ -76,15 +73,28 @@ public List<Parser.Tag> parse(String input) throws ParseException {
}

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) {
case '@' -> tags.add(readTag());
default -> nextChar();
if (ch == '@') {
tags.add(readTag());
} else {
break;
}
}

return tags;
}

Expand All @@ -94,26 +104,13 @@ protected Parser.Tag readTag() throws ParseException {
String name = readIdentifier();
skipWhitespace();

boolean appliesToNextLine = false;
List<Attribute> attributes = List.of();

if (ch == ':') {
appliesToNextLine = true;
nextChar();
} else {
attributes = attrs();
skipWhitespace();
if (ch == ':') {
appliesToNextLine = true;
nextChar();
}
}
List<Attribute> attributes = attrs();
skipWhitespace();

Parser.Tag i = new Parser.Tag();
i.nameLineOffset = nameBp;
i.name = name;
i.attributes = attributes;
i.appliesToNextLine = appliesToNextLine;

return i;
}
Expand Down
10 changes: 5 additions & 5 deletions ...dk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java
View file
Expand Up @@ -94,19 +94,19 @@ public Parser(Resources resources) {
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);

Expand Down Expand Up @@ -150,7 +150,7 @@ record OffsetAndLine(int offset, String line) { }
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();
Expand All @@ -166,7 +166,7 @@ record OffsetAndLine(int offset, String line) { }
}
}
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;
Expand Down
44 changes: 44 additions & 0 deletions test/langtools/jdk/javadoc/doclet/testSnippetTag/SnippetTester.java
View file
Expand Up @@ -26,9 +26,11 @@
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;
Expand Down Expand Up @@ -122,4 +124,46 @@ protected String getSnippetHtmlRepresentation(String pathToHtmlFile,
<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;
}
}
}
}
}
109 changes: 109 additions & 0 deletions test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java
View file
Expand Up @@ -603,6 +603,115 @@ public void testPositiveInlineTagMarkup_BlankLinesFromNextLineMarkup(Path base)
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)
Expand Down
5 changes: 3 additions & 2 deletions test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java
View file
Expand Up @@ -2418,11 +2418,12 @@ record TestCase(String input, String expectedError) { }
/* ---------------------- */
new TestCase("""
{@snippet :
hello // @highlight substring="
hello
there // @highlight substring="
}""",
"""
error: snippet markup: unterminated attribute value
hello // @highlight substring="
there // @highlight substring="
^
"""),
new TestCase("""
Expand Down

1 comment on commit a8a1fbc

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.