Skip to content

Commit

Permalink
AVRO-3294: Improve IDL doc comment handling (apache#1453)
Browse files Browse the repository at this point in the history 
* AVRO-3294: Improve IDL doc comment handling

Use the previously introduced method `DocCommentHandler#clearDoc()` to
generate warnings for misplaced documentation. Also add documentation to
describe the tricky bits when using this method.

The brittleness of the solution proves that doc comments as special
tokens are a hack. However, making regular tokens out of them may break
existing `.avdl` files.

* AVRO-3294: Trigger build
  • Loading branch information
@opwvhk
opwvhk authored Feb 7, 2022
1 parent 8fbd830 commit 2681c1b
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 112 deletions.
8 changes: 8 additions & 0 deletions lang/java/compiler/src/main/java/org/apache/avro/compiler/idl/DocCommentHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,14 @@ static void setDoc(Token token) {
DOC.set(newDocComment);
}

/**
* Clear any documentation (and generate a warning if there was).
*
* This method should NOT be used after an optional component in a grammar
* (i.e., after a @code{[…]} or @code{…*} construct), because the optional
* grammar part may have already caused parsing a doc comment special token
* placed after the code block.
*/
static void clearDoc() {
DocComment oldDocComment = DOC.get();
if (oldDocComment != null) {
Expand Down
113 changes: 64 additions & 49 deletions lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1065,7 +1065,7 @@ Protocol CompilationUnit():
/*
* Declaration syntax follows.
*/
private Schema NamedSchemaDeclaration(Map<String, JsonNode> props):
private Schema NamedSchemaDeclaration(String doc, Map<String, JsonNode> props):
{
Schema s;
String savedSpace = this.namespace;
Expand All @@ -1076,9 +1076,9 @@ private Schema NamedSchemaDeclaration(Map<String, JsonNode> props):
this.namespace = getTextProp("namespace", props, token);
}
(
s = FixedDeclaration()
| s = EnumDeclaration()
| s = RecordDeclaration()
s = FixedDeclaration(doc)
| s = EnumDeclaration(doc)
| s = RecordDeclaration(doc)
)
{
this.namespace = savedSpace;
Expand Down Expand Up @@ -1125,11 +1125,12 @@ Schema UnionDefinition():

Protocol ProtocolDeclaration():
{
String name;
String doc, name;
Protocol p;
Map<String, JsonNode> props = new LinkedHashMap<>();
}
{
doc = Documentation()
( SchemaProperty(props) )*
{
if (props.containsKey("namespace"))
Expand All @@ -1138,7 +1139,7 @@ Protocol ProtocolDeclaration():
"protocol"
name = Identifier()
{
p = new Protocol(name, DocCommentHelper.getDoc(), namespace);
p = new Protocol(name, doc, namespace);
for (String key : props.keySet())
if ("namespace".equals(key)) { // already handled: ignore
} else { // add all other properties
Expand All @@ -1152,17 +1153,30 @@ Protocol ProtocolDeclaration():
}


Schema EnumDeclaration():
String Documentation():
{
//noinspection ResultOfMethodCallIgnored
getToken(1); // Parse, but don't consume, at least one token; this triggers parsing special tokens like doc comments.
}
{
// Don't parse anything, just return the doc string
{
return DocCommentHelper.getDoc();
}
}


Schema EnumDeclaration(String doc):
{
String name;
List<String> symbols;
String defaultSymbol = null;
}
{
"enum" { String doc = DocCommentHelper.getDoc(); }
"enum"
name = Identifier()
symbols = EnumBody()
[ <EQUALS> defaultSymbol=Identifier() <SEMICOLON>]
[ <EQUALS> defaultSymbol=Identifier() <SEMICOLON> { DocCommentHelper.clearDoc(); } ]
{
Schema s = Schema.createEnum(name, doc, namespace, symbols, defaultSymbol);
names.put(s.getFullName(), s);
Expand All @@ -1175,10 +1189,11 @@ List<String> EnumBody():
List<String> symbols = new ArrayList<>();
}
{
"{"
"{" { DocCommentHelper.clearDoc(); }
[ EnumConstant(symbols) ( "," EnumConstant(symbols) )* ]
"}"
{
DocCommentHelper.clearDoc();
return symbols;
}
}
Expand All @@ -1193,13 +1208,14 @@ void EnumConstant(List<String> symbols):

void ProtocolBody(Protocol p):
{
String doc;
Schema schema;
Message message;
Protocol importProtocol;
Map<String, JsonNode> props = new LinkedHashMap<>();
}
{
"{"
"{" { DocCommentHelper.clearDoc(); }
(
<IMPORT>
((( importProtocol = ImportIdl() | importProtocol = ImportProtocol()) {
Expand All @@ -1208,21 +1224,26 @@ void ProtocolBody(Protocol p):
p.getMessages().putAll(importProtocol.getMessages());
})
| schema = ImportSchema()
)
) {
DocCommentHelper.clearDoc();
}
|
doc = Documentation()
( SchemaProperty(props) )*
(
schema = NamedSchemaDeclaration(props)
schema = NamedSchemaDeclaration(doc, props)
|
message = MessageDeclaration(p, props) {
message = MessageDeclaration(p, doc, props) {
p.getMessages().put(message.getName(), message);
}
) { props.clear(); }
) {
props.clear();
}
) *
"}"

{
p.setTypes(names.values());
DocCommentHelper.clearDoc();
}
}

Expand Down Expand Up @@ -1273,7 +1294,7 @@ Schema ImportSchema() : {
}
}

Schema FixedDeclaration():
Schema FixedDeclaration(String doc):
{
String name;
Token sizeTok;
Expand All @@ -1282,14 +1303,14 @@ Schema FixedDeclaration():
"fixed" name = Identifier() "(" sizeTok = <INTEGER_LITERAL> ")"
";"
{
Schema s = Schema.createFixed(name, DocCommentHelper.getDoc(), this.namespace,
Integer.parseInt(sizeTok.image));
DocCommentHelper.clearDoc();
Schema s = Schema.createFixed(name, doc, this.namespace, Integer.parseInt(sizeTok.image));
names.put(s.getFullName(), s);
return s;
}
}

Schema RecordDeclaration():
Schema RecordDeclaration(String doc):
{
String name;
List<Field> fields = new ArrayList<>();
Expand All @@ -1302,14 +1323,14 @@ Schema RecordDeclaration():
)
name = Identifier()
{
Schema result = Schema.createRecord(
name, DocCommentHelper.getDoc(), this.namespace, isError);
Schema result = Schema.createRecord(name, doc, this.namespace, isError);
names.put(result.getFullName(), result);
}
"{"
"{" { DocCommentHelper.clearDoc(); }
( FieldDeclaration(fields) )*
"}"
{
DocCommentHelper.clearDoc();
result.setFields(fields);
return result;
}
Expand All @@ -1332,27 +1353,27 @@ private void SchemaProperty(Map<String, JsonNode> properties):

void FieldDeclaration(List<Field> fields):
{
String defaultDoc;
Schema type;
}
{
defaultDoc = Documentation()
type = Type()
VariableDeclarator(type, fields) ( "," VariableDeclarator(type, fields) )*
";"
VariableDeclarator(type, defaultDoc, fields) ( "," VariableDeclarator(type, defaultDoc, fields) )*
";" { DocCommentHelper.clearDoc(); }
}

void VariableDeclarator(Schema type, List<Field> fields):
void VariableDeclarator(Schema type, String defaultDoc, List<Field> fields):
{
String name;
String doc, name;
JsonNode defaultValue = null;
Map<String, JsonNode> props = new LinkedHashMap<>();
}
{
( SchemaProperty(props) )*

doc = Documentation()
( SchemaProperty(props) )*
name = Identifier()

[ <EQUALS> defaultValue=Json() ]

[ <EQUALS> defaultValue=Json() ]
{
Field.Order order = Field.Order.ASCENDING;
for (String key : props.keySet())
Expand All @@ -1361,7 +1382,7 @@ void VariableDeclarator(Schema type, List<Field> fields):

boolean validate = SchemaResolver.isFullyResolvedSchema(type);
Schema fieldType = fixOptionalSchema(type, defaultValue);
Field field = Accessor.createField(name, fieldType, DocCommentHelper.getDoc(), defaultValue, validate, order);
Field field = Accessor.createField(name, fieldType, doc == null ? defaultDoc : doc, defaultValue, validate, order);
for (String key : props.keySet())
if ("order".equals(key)) { // already handled: ignore
} else if ("aliases".equals(key)) { // aliases
Expand All @@ -1371,22 +1392,13 @@ void VariableDeclarator(Schema type, List<Field> fields):
Accessor.addProp(field, key, props.get(key));
}
fields.add(field);
DocCommentHelper.clearDoc();
}
}


String MessageDocumentation():
{}
{
// Don't parse anything, just return the doc string
{
return DocCommentHelper.getDoc();
}
}

private Message MessageDeclaration(Protocol p, Map<String, JsonNode> props):
private Message MessageDeclaration(Protocol p, String msgDoc, Map<String, JsonNode> props):
{
String msgDoc;
String name;
Schema request;
Schema response;
Expand All @@ -1395,13 +1407,12 @@ private Message MessageDeclaration(Protocol p, Map<String, JsonNode> props):
errorSchemata.add(Protocol.SYSTEM_ERROR);
}
{
msgDoc = MessageDocumentation()
response = ResultType()
name = Identifier()
response = ResultType() name = Identifier()
request = FormalParameters()
[ "oneway" {oneWay = true; } | "throws" ErrorList(errorSchemata) ]
";"
{
DocCommentHelper.clearDoc();
Schema errors = Schema.createUnion(errorSchemata);
if (oneWay && response.getType() != Type.NULL)
throw error("One-way message'"+name+"' must return void", token);
Expand All @@ -1426,19 +1437,23 @@ Schema FormalParameters():
List<Field> fields = new ArrayList<>();
}
{
"(" [ FormalParameter(fields) ( "," FormalParameter(fields) )* ] ")"
"(" { DocCommentHelper.clearDoc(); }
[ FormalParameter(fields) ( "," FormalParameter(fields) )* ] ")"
{
DocCommentHelper.clearDoc();
return Schema.createRecord(null, null, null, false, fields);
}
}

void FormalParameter(List<Field> fields):
{
String doc;
Schema type;
}
{
doc = Documentation()
type = Type()
VariableDeclarator(type, fields)
VariableDeclarator(type, doc, fields)
}

Schema Type():
Expand Down
37 changes: 17 additions & 20 deletions lang/java/compiler/src/test/idl/input/comments.avdl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,41 +27,38 @@ protocol Comments {
/** Dangling Enum8 */ A
/** Dangling Enum9 */;

// The "Dangling Enum9" doc comment above will be attributed to this enum.
enum NotUndocumentedEnum {D,E}
enum UndocumentedEnum {D,E}

/** Dangling Fixed1 */ fixed
/** Dangling Fixed2 */ DocumentedFixed
/** Dangling Fixed3 */(
/** Dangling Fixed4 */ 16
/** Dangling Fixed5 */)
/** Documented Fixed Type */;
/** Documented Fixed Type */ fixed
/** Dangling Fixed1 */ DocumentedFixed
/** Dangling Fixed2 */(
/** Dangling Fixed3 */ 16
/** Dangling Fixed4 */)
/** Dangling Fixed5 */;

fixed UndocumentedFixed(16);

/** Dangling Error1 */ error
/** Documented Error */ DocumentedError
/** Documented Error */ error
/** Dangling Error1 */ DocumentedError
/** Dangling Field1 */{
/** Dangling Field2 */string
/** Dangling Field3 */reason
/** Documented Field */;
/** Default Doc Explanation Field */string
/** Documented Reason Field */reason, explanation
/** Dangling Field2 */;
/** Dangling Error2 */}

// The "Dangling Error2" doc comment above will be attributed to this record.
record NotUndocumentedRecord {
record UndocumentedRecord {
string description;
}

/** Documented Method */ void
/** Dangling Param1 */ documentedMethod
/** Dangling Param2 */(
/** Dangling Param3 */ string
/** Dangling Param4 */ message
/** Documented Parameter */)
string /** Documented Parameter */ message,
/** Default Documented Parameter */ string defMsg
/** Dangling Param3 */)
/** Dangling Method1 */ throws
/** Dangling Method2 */ DocumentedError
/** Dangling Method3 */;

// The "Dangling Method3" doc comment above will be attributed to this method.
void notUndocumentedMethod(string message);
void undocumentedMethod(string message);
}
Loading

0 comments on commit 2681c1b

Please sign in to comment.