Skip to content

Commit

Permalink
Move the check to find jsdoc in block comments (starting with /* rath…
Browse files Browse the repository at this point in the history
…er than /**) into the linter.

Due to the current code design, errors / warnings reported in RhinoErrorReporter cannot be suppressed. For the sake of tsickle generated code and / or third party code, we don't want to report this error. Moving it to the linter seems appropriate.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=220483845
  • Loading branch information
johnplaisted authored and brad4d committed Nov 8, 2018
1 parent bab8ee8 commit 37a03f3
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 92 deletions.
37 changes: 37 additions & 0 deletions src/com/google/javascript/jscomp/CheckJSDoc.java
Expand Up @@ -20,11 +20,13 @@


import com.google.common.collect.Iterables; import com.google.common.collect.Iterables;
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
import com.google.javascript.jscomp.parsing.parser.trees.Comment;
import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.JSTypeExpression; import com.google.javascript.rhino.JSTypeExpression;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token; import com.google.javascript.rhino.Token;
import java.util.Set; import java.util.Set;
import java.util.regex.Pattern;
import javax.annotation.Nullable; import javax.annotation.Nullable;


/** /**
Expand Down Expand Up @@ -80,6 +82,14 @@ final class CheckJSDoc extends AbstractPostOrderCallback implements HotSwapCompi
"@suppress annotation not allowed here. See" "@suppress annotation not allowed here. See"
+ " https://github.com/google/closure-compiler/wiki/@suppress-annotations"); + " https://github.com/google/closure-compiler/wiki/@suppress-annotations");


public static final DiagnosticType JSDOC_IN_BLOCK_COMMENT =
DiagnosticType.warning(
"JSC_JSDOC_IN_BLOCK_COMMENT",
"Non-JSDoc comment has annotations. Did you mean to start it with '/**'?");

private static final Pattern COMMENT_PATTERN =
Pattern.compile("(/|(\n[ \t]*))\\*[ \t]*@[a-zA-Z]+[ \t\n{]");

private final AbstractCompiler compiler; private final AbstractCompiler compiler;
private boolean inExterns; private boolean inExterns;


Expand All @@ -100,8 +110,35 @@ public void hotSwapScript(Node scriptRoot, Node originalRoot) {
NodeTraversal.traverse(compiler, scriptRoot, this); NodeTraversal.traverse(compiler, scriptRoot, this);
} }


/**
* Checks for block comments (e.g. starting with /*) that look like they are JsDoc, and thus
* should start with /**.
*/
private void checkJsDocInBlockComments(String fileName) {
if (!compiler.getOptions().preservesDetailedSourceInfo()) {
// Comments only available if preservesDetailedSourceInfo is true.
return;
}

for (Comment comment : compiler.getComments(fileName)) {
if (comment.type == Comment.Type.BLOCK) {
if (COMMENT_PATTERN.matcher(comment.value).find()) {
compiler.report(
JSError.make(
fileName,
comment.location.start.line + 1,
comment.location.start.column,
JSDOC_IN_BLOCK_COMMENT));
}
}
}
}

@Override @Override
public void visit(NodeTraversal t, Node n, Node parent) { public void visit(NodeTraversal t, Node n, Node parent) {
if (n.isScript()) {
checkJsDocInBlockComments(n.getSourceFileName());
}
JSDocInfo info = n.getJSDocInfo(); JSDocInfo info = n.getJSDocInfo();
validateTypeAnnotations(n, info); validateTypeAnnotations(n, info);
validateFunctionJsDoc(n, info); validateFunctionJsDoc(n, info);
Expand Down
5 changes: 3 additions & 2 deletions src/com/google/javascript/jscomp/DiagnosticGroups.java
Expand Up @@ -204,10 +204,11 @@ public DiagnosticGroup forName(String name) {
DiagnosticGroups.registerGroup("accessControls", VISIBILITY); DiagnosticGroups.registerGroup("accessControls", VISIBILITY);


public static final DiagnosticGroup NON_STANDARD_JSDOC = public static final DiagnosticGroup NON_STANDARD_JSDOC =
DiagnosticGroups.registerGroup("nonStandardJsDocs", DiagnosticGroups.registerGroup(
"nonStandardJsDocs",
RhinoErrorReporter.BAD_JSDOC_ANNOTATION, RhinoErrorReporter.BAD_JSDOC_ANNOTATION,
RhinoErrorReporter.INVALID_PARAM, RhinoErrorReporter.INVALID_PARAM,
RhinoErrorReporter.JSDOC_IN_BLOCK_COMMENT); CheckJSDoc.JSDOC_IN_BLOCK_COMMENT);


public static final DiagnosticGroup INVALID_CASTS = public static final DiagnosticGroup INVALID_CASTS =
DiagnosticGroups.registerGroup("invalidCasts", DiagnosticGroups.registerGroup("invalidCasts",
Expand Down
3 changes: 3 additions & 0 deletions src/com/google/javascript/jscomp/Linter.java
Expand Up @@ -21,6 +21,7 @@
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
import com.google.javascript.jscomp.parsing.Config.JsDocParsing;
import com.google.javascript.refactoring.ApplySuggestedFixes; import com.google.javascript.refactoring.ApplySuggestedFixes;
import com.google.javascript.refactoring.FixingErrorManager; import com.google.javascript.refactoring.FixingErrorManager;
import com.google.javascript.refactoring.SuggestedFix; import com.google.javascript.refactoring.SuggestedFix;
Expand Down Expand Up @@ -50,6 +51,8 @@ static void lint(Path path, Compiler compiler) throws IOException {
SourceFile file = SourceFile.fromFile(path.toString()); SourceFile file = SourceFile.fromFile(path.toString());
CompilerOptions options = new CompilerOptions(); CompilerOptions options = new CompilerOptions();
options.setLanguage(LanguageMode.ECMASCRIPT_NEXT); options.setLanguage(LanguageMode.ECMASCRIPT_NEXT);
options.setParseJsDocDocumentation(JsDocParsing.INCLUDE_DESCRIPTIONS_WITH_WHITESPACE);
options.setPreserveDetailedSourceInfo(true);


// These are necessary to make sure that suggested fixes are printed correctly. // These are necessary to make sure that suggested fixes are printed correctly.
options.setPrettyPrint(true); options.setPrettyPrint(true);
Expand Down
9 changes: 0 additions & 9 deletions src/com/google/javascript/jscomp/RhinoErrorReporter.java
Expand Up @@ -72,9 +72,6 @@ class RhinoErrorReporter {
static final DiagnosticType BAD_JSDOC_ANNOTATION = static final DiagnosticType BAD_JSDOC_ANNOTATION =
DiagnosticType.warning("JSC_BAD_JSDOC_ANNOTATION", "Parse error. {0}"); DiagnosticType.warning("JSC_BAD_JSDOC_ANNOTATION", "Parse error. {0}");


static final DiagnosticType JSDOC_IN_BLOCK_COMMENT =
DiagnosticType.warning("JSC_JSDOC_IN_BLOCK_COMMENT", "Parse error. {0}");

static final DiagnosticType INVALID_ES3_PROP_NAME = DiagnosticType.warning( static final DiagnosticType INVALID_ES3_PROP_NAME = DiagnosticType.warning(
"JSC_INVALID_ES3_PROP_NAME", "JSC_INVALID_ES3_PROP_NAME",
"Keywords and reserved words are not allowed as unquoted property " + "Keywords and reserved words are not allowed as unquoted property " +
Expand Down Expand Up @@ -141,12 +138,6 @@ private RhinoErrorReporter(AbstractCompiler compiler) {
replacePlaceHolders(SimpleErrorReporter.getMessage0("msg.bad.jsdoc.tag")), replacePlaceHolders(SimpleErrorReporter.getMessage0("msg.bad.jsdoc.tag")),
BAD_JSDOC_ANNOTATION) BAD_JSDOC_ANNOTATION)


.put(
Pattern.compile("^" + Pattern.quote(
"Non-JSDoc comment has annotations. "
+ "Did you mean to start it with '/**'?")),
JSDOC_IN_BLOCK_COMMENT)

.put( .put(
Pattern.compile( Pattern.compile(
"^Keywords and reserved words are not allowed as unquoted property.*"), "^Keywords and reserved words are not allowed as unquoted property.*"),
Expand Down
23 changes: 0 additions & 23 deletions src/com/google/javascript/jscomp/parsing/IRFactory.java
Expand Up @@ -162,7 +162,6 @@
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Set; import java.util.Set;
import java.util.regex.Pattern;
import javax.annotation.Nullable; import javax.annotation.Nullable;


/** /**
Expand All @@ -180,10 +179,6 @@ class IRFactory {
"If you are targeting newer versions of JavaScript, " + "If you are targeting newer versions of JavaScript, " +
"set the appropriate language_in option."; "set the appropriate language_in option.";


static final String SUSPICIOUS_COMMENT_WARNING =
"Non-JSDoc comment has annotations. " +
"Did you mean to start it with '/**'?";

static final String INVALID_ES3_PROP_NAME = static final String INVALID_ES3_PROP_NAME =
"Keywords and reserved words are not allowed as unquoted property " + "Keywords and reserved words are not allowed as unquoted property " +
"names in older versions of JavaScript. " + "names in older versions of JavaScript. " +
Expand Down Expand Up @@ -244,9 +239,6 @@ class IRFactory {
"implements", "interface", "let", "package", "private", "protected", "implements", "interface", "let", "package", "private", "protected",
"public", "static", "yield"); "public", "static", "yield");


private static final Pattern COMMENT_PATTERN =
Pattern.compile("(/|(\n[ \t]*))\\*[ \t]*@[a-zA-Z]+[ \t\n{]");

/** /**
* If non-null, use this set of keywords instead of TokenStream.isKeyword(). * If non-null, use this set of keywords instead of TokenStream.isKeyword().
*/ */
Expand Down Expand Up @@ -341,8 +333,6 @@ public static IRFactory transformTree(ProgramTree tree,
if ((comment.type == Comment.Type.JSDOC || comment.type == Comment.Type.IMPORTANT) if ((comment.type == Comment.Type.JSDOC || comment.type == Comment.Type.IMPORTANT)
&& !irFactory.parsedComments.contains(comment)) { && !irFactory.parsedComments.contains(comment)) {
irFactory.handlePossibleFileOverviewJsDoc(comment); irFactory.handlePossibleFileOverviewJsDoc(comment);
} else if (comment.type == Comment.Type.BLOCK) {
irFactory.handleBlockComment(comment);
} }
} }
} }
Expand Down Expand Up @@ -606,19 +596,6 @@ Node transformBlock(ParseTree node) {
return irNode; return irNode;
} }


/**
* Check to see if the given block comment looks like it should be JSDoc.
*/
private void handleBlockComment(Comment comment) {
if (COMMENT_PATTERN.matcher(comment.value).find()) {
errorReporter.warning(
SUSPICIOUS_COMMENT_WARNING,
sourceName,
lineno(comment.location.start),
charno(comment.location.start));
}
}

/** /**
* @return true if the jsDocParser represents a fileoverview. * @return true if the jsDocParser represents a fileoverview.
*/ */
Expand Down
17 changes: 17 additions & 0 deletions test/com/google/javascript/jscomp/CheckJsDocTest.java
Expand Up @@ -23,11 +23,13 @@
import static com.google.javascript.jscomp.CheckJSDoc.INVALID_DEFINE_ON_LET; import static com.google.javascript.jscomp.CheckJSDoc.INVALID_DEFINE_ON_LET;
import static com.google.javascript.jscomp.CheckJSDoc.INVALID_MODIFIES_ANNOTATION; import static com.google.javascript.jscomp.CheckJSDoc.INVALID_MODIFIES_ANNOTATION;
import static com.google.javascript.jscomp.CheckJSDoc.INVALID_NO_SIDE_EFFECT_ANNOTATION; import static com.google.javascript.jscomp.CheckJSDoc.INVALID_NO_SIDE_EFFECT_ANNOTATION;
import static com.google.javascript.jscomp.CheckJSDoc.JSDOC_IN_BLOCK_COMMENT;
import static com.google.javascript.jscomp.CheckJSDoc.MISPLACED_ANNOTATION; import static com.google.javascript.jscomp.CheckJSDoc.MISPLACED_ANNOTATION;
import static com.google.javascript.jscomp.CheckJSDoc.MISPLACED_MSG_ANNOTATION; import static com.google.javascript.jscomp.CheckJSDoc.MISPLACED_MSG_ANNOTATION;
import static com.google.javascript.jscomp.CheckJSDoc.MISPLACED_SUPPRESS; import static com.google.javascript.jscomp.CheckJSDoc.MISPLACED_SUPPRESS;


import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
import com.google.javascript.jscomp.parsing.Config.JsDocParsing;
import org.junit.Before; import org.junit.Before;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
Expand Down Expand Up @@ -57,6 +59,8 @@ public void setUp() throws Exception {
protected CompilerOptions getOptions() { protected CompilerOptions getOptions() {
CompilerOptions options = super.getOptions(); CompilerOptions options = super.getOptions();
options.setWarningLevel(DiagnosticGroups.MISPLACED_SUPPRESS, CheckLevel.WARNING); options.setWarningLevel(DiagnosticGroups.MISPLACED_SUPPRESS, CheckLevel.WARNING);
options.setParseJsDocDocumentation(JsDocParsing.INCLUDE_DESCRIPTIONS_WITH_WHITESPACE);
options.setPreserveDetailedSourceInfo(true);
return options; return options;
} }


Expand Down Expand Up @@ -983,4 +987,17 @@ public void testImplicitCastOnlyAllowedInExterns() {
"Element.prototype.innerHTML;"), "Element.prototype.innerHTML;"),
TypeCheck.ILLEGAL_IMPLICIT_CAST); TypeCheck.ILLEGAL_IMPLICIT_CAST);
} }

@Test
public void testJsDocInBlockComments() {
testSame("/** @type {X} */ let x;");
testSame("// @type {X}\nlet x;");

testWarning("/* @type {X} */ let x;", JSDOC_IN_BLOCK_COMMENT);

// jsdoc tags contain letters only, no underscores etc.
testSame("/* @cc_on */ var x = 3;");
// a jsdoc tag can't be immediately followed by a paren
testSame("/* @TODO(username) */ var x = 3;");
}
} }
58 changes: 0 additions & 58 deletions test/com/google/javascript/jscomp/parsing/ParserTest.java
Expand Up @@ -50,9 +50,6 @@


@RunWith(JUnit4.class) @RunWith(JUnit4.class)
public final class ParserTest extends BaseJSTypeTestCase { public final class ParserTest extends BaseJSTypeTestCase {
private static final String SUSPICIOUS_COMMENT_WARNING =
IRFactory.SUSPICIOUS_COMMENT_WARNING;

private static final String TRAILING_COMMA_MESSAGE = private static final String TRAILING_COMMA_MESSAGE =
"Trailing comma is not legal in an ECMA-262 object initializer"; "Trailing comma is not legal in an ECMA-262 object initializer";


Expand Down Expand Up @@ -1905,61 +1902,6 @@ public void testTrailingCommaWarning7() {
"'}' expected"); "'}' expected");
} }


@Test
public void testSuspiciousBlockCommentWarning1() {
parseWarning("/* @type {number} */ var x = 3;", SUSPICIOUS_COMMENT_WARNING);
}

@Test
public void testSuspiciousBlockCommentWarning2() {
parseWarning("/* \n * @type {number} */ var x = 3;",
SUSPICIOUS_COMMENT_WARNING);
}

@Test
public void testSuspiciousBlockCommentWarning3() {
parseWarning("/* \n *@type {number} */ var x = 3;",
SUSPICIOUS_COMMENT_WARNING);
}

@Test
public void testSuspiciousBlockCommentWarning4() {
parseWarning(
" /*\n" +
" * @type {number}\n" +
" */\n" +
" var x = 3;",
SUSPICIOUS_COMMENT_WARNING);
}

@Test
public void testSuspiciousBlockCommentWarning5() {
parseWarning(
" /*\n" +
" * some random text here\n" +
" * @type {number}\n" +
" */\n" +
" var x = 3;",
SUSPICIOUS_COMMENT_WARNING);
}

@Test
public void testSuspiciousBlockCommentWarning6() {
parseWarning("/* @type{number} */ var x = 3;", SUSPICIOUS_COMMENT_WARNING);
}

@Test
public void testSuspiciousBlockCommentWarning7() {
// jsdoc tags contain letters only, no underscores etc.
parse("/* @cc_on */ var x = 3;");
}

@Test
public void testSuspiciousBlockCommentWarning8() {
// a jsdoc tag can't be immediately followed by a paren
parse("/* @TODO(username) */ var x = 3;");
}

@Test @Test
public void testCatchClauseForbidden() { public void testCatchClauseForbidden() {
parseError("try { } catch (e if true) {}", parseError("try { } catch (e if true) {}",
Expand Down

0 comments on commit 37a03f3

Please sign in to comment.