Fix DirectivePrologue parsing problems.

Fix problems found by David-Sarah Hopwood after the following change was already submitted:

  http://codereview.appspot.com/136053

The changes made here are:

1. Documentation fixes.

2. Parsing of directive prologues no longer tries to do any semicolon insertion.

3. Fixed bug in conditional rendering of Directive -- the Directive should not render itself if it contains escape sequences, but the original code did this check incorrectly.

[MikeSamuel, 2009-11-19 01:57:23]

Funnily, I found myself mucking around with the same bit of code since a
directive prologue that ends a program causes a ParseException.

My patch is below.  Maybe we should synchronize.

Index: src/com/google/caja/parser/js/Parser.java
===================================================================
--- src/com/google/caja/parser/js/Parser.java	(revision 3854)
+++ src/com/google/caja/parser/js/Parser.java	(working copy)
@@ -73,16 +73,15 @@
  *
  * <xmp>
  * // Terminal productions
- * Program                 => <UseSubsetDirective>? <TerminatedStatement>*
+ * Program                 => <DirectivePrologue>? <TerminatedStatement>*
  * Statement               => <TerminalStatement>
  *                          | <NonTerminalStatement>
  * Expression              => <CommaOperator>
  *
  * // Non-terminal productions.  Indentation indicates that a nonterminal is
  * // used only by the unindented nonterminal above it.
- * UseSubsetDirective      => '"' 'use' <UseSubsetList>? '"'
- * UseSubsetList           => <UseSubset> <UseSubsetTail>*
- * UseSubsetTail           => ',' <UseSubset>
+ * DirectivePrologue       => <Directive> <DirectivePrologue>?
+ * Directive               => <StringLiteral> ';'
  * TerminatedStatement     => <BlockStatement>
  *                          | <SimpleStatement> <StatementTerminator>
  *   StatementTerminator   => ';'
@@ -323,62 +322,51 @@
     Mark startOfPrologue = tq.mark();
     List<Directive> directives = Lists.newArrayList();
 
-    while (tq.peek().type == JsTokenType.STRING) {
+    while (!tq.isEmpty() && tq.peek().type == JsTokenType.STRING) {
       Mark startOfDirective = tq.mark();
       Token<JsTokenType> quotedString = tq.pop();
 
-      boolean isDirective = false;
-      switch(tq.peek().type) {
-        case STRING:
-        case REGEXP:
-        case WORD:
-        case INTEGER:
-        case FLOAT:
-        case KEYWORD:
-        case COMMENT:
-          isDirective = true;
-          mq.addMessage(
-              MessageType.SEMICOLON_INSERTED,
-              FilePosition.endOf(quotedString.pos));
+      if (!tq.checkToken(Punctuation.SEMI)) {
+        Token<JsTokenType> t = !tq.isEmpty() ? tq.peek() : null;
+        if ((t == null || !continuesExpr(t.text)) && semicolonInserted()) {
+          FilePosition semiPoint = FilePosition.endOf(tq.lastPosition());
+          MessageLevel lvl = tq.isEmpty()
+              || tq.lookaheadToken(Punctuation.RCURLY)
+              ? MessageLevel.LOG : MessageLevel.LINT;
+          mq.addMessage(MessageType.SEMICOLON_INSERTED, lvl, semiPoint);
+        } else {
+          tq.rewind(startOfDirective);
           break;
-        case PUNCTUATION:
-          if (tq.lookaheadToken(Punctuation.SEMI)) {
-            isDirective = true;
-            tq.pop();
-          } else if (tq.lookaheadToken(Punctuation.RCURLY)) {
-            isDirective = true;
-            mq.addMessage(
-                MessageType.SEMICOLON_INSERTED,
-                FilePosition.endOf(quotedString.pos));
-          }
-          break;
-        case LINE_CONTINUATION:
-          break;
+        }
       }
 
-      if (!isDirective) {
-        tq.rewind(startOfDirective);
-        break;
-      }
-
+      String unquoted = quotedString.text.substring(
+          1, quotedString.text.length() - 1);
       String decoded = StringLiteral.getUnquotedValueOf(quotedString.text);
-      if (!Directive.RecognizedValue.isDirectiveStringRecognized(decoded)) {
+      if (!unquoted.equals(decoded)
+          || !Directive.RecognizedValue.isDirectiveStringRecognized(unquoted))
{
         mq.addMessage(
             MessageType.UNRECOGNIZED_DIRECTIVE_IN_PROLOGUE,
             quotedString.pos, MessagePart.Factory.valueOf(decoded));
       }
 
-      directives.add(new Directive(
-          FilePosition.startOf(quotedString.pos), decoded));
+      Directive d = new Directive(posFrom(quotedString.pos), decoded);
+      finish(d, startOfDirective);
+      directives.add(d);
     }
 
     if (directives.isEmpty()) { return null; }
 
-    DirectivePrologue prologue =
-        new DirectivePrologue(posFrom(startOfPrologue), directives);
+    DirectivePrologue prologue = new DirectivePrologue(
+        posFrom(startOfPrologue), directives);
     finish(prologue, startOfPrologue);
     return prologue;
   }
+  private static boolean continuesExpr(String tokenText) {
+    return Operator.lookupOperation(tokenText, OperatorType.INFIX) != null
+        || Operator.lookupOperation(tokenText, OperatorType.BRACKET) != null
+        || Operator.lookupOperation(tokenText, OperatorType.TERNARY) != null;
+  }
 
   private LabeledStatement parseLoopOrSwitch(FilePosition start, String label)
       throws ParseException {
@@ -1256,6 +1244,9 @@
   }
 
   private static boolean isTerminal(Statement s) {
+    if (s instanceof LabeledStmtWrapper) {
+      return isTerminal(((LabeledStmtWrapper) s).getBody());
+    }
     return ((s instanceof Loop && !(s instanceof DoWhileLoop))
             || s instanceof Conditional || s instanceof FunctionDeclaration
             || s instanceof Block || s instanceof TryStmt
@@ -1265,9 +1256,7 @@
 
   private Statement parseTerminatedStatement() throws ParseException {
     Statement s = parseStatement();
-    if (!isTerminal(s)) {
-      checkSemicolon();
-    }
+    if (!isTerminal(s)) { checkSemicolon(); }
     return s;
   }

[MikeSamuel, 2009-11-19 02:00:31]

http://codereview.appspot.com/156090/diff/16/21
File src/com/google/caja/parser/js/Parser.java (right):

http://codereview.appspot.com/156090/diff/16/21#newcode330
src/com/google/caja/parser/js/Parser.java:330: if
(!tq.checkToken(Punctuation.SEMI)) {
Is this correct?
Can we include a reference to the draft verbiage which disallow semicolon
insertion here?

http://codereview.appspot.com/156090/diff/16/17
File tests/com/google/caja/parser/js/parsertest6.js (right):

http://codereview.appspot.com/156090/diff/16/17#newcode4
tests/com/google/caja/parser/js/parsertest6.js:4: "\\";  // Should not be
rendered
cool

[DavidSarah, 2009-11-19 03:25:45]

http://codereview.appspot.com/156090/diff/16/21
File src/com/google/caja/parser/js/Parser.java (right):

http://codereview.appspot.com/156090/diff/16/21#newcode330
src/com/google/caja/parser/js/Parser.java:330: if
(!tq.checkToken(Punctuation.SEMI)) {
On 2009/11/19 02:00:31, MikeSamuel wrote:
> Is this correct?
> Can we include a reference to the draft verbiage which disallow semicolon
> insertion here?

ES5 does allow semicolon insertion in a prologue; see
https://mail.mozilla.org/pipermail/es5-discuss/2009-September/003246.html

Whether you need to accept this in Valija is a different matter; I think it's
perfectly reasonable to treat it as an error (semicolon insertion is already an
error in Cajita, no?). But it looks like this code just stops looking for
directives if it doesn't see the semicolon, which seems wrong.

[DavidSarah, 2009-11-19 03:28:33]

I don't see the fix for the rendering problem in this CL; where should I be
looking?

[DavidSarah, 2009-11-19 03:29:59]

On 2009/11/19 03:28:33, DavidSarah wrote:
> I don't see the fix for the rendering problem in this CL; where should I be
looking?

Never mind, I see it now. LGTM.