Allow whitespace and \ before the first newline of multiline string.

sdk/lib/_internal/compiler/implementation/string_validator.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 // Check the validity of string literals.
 
 library stringvalidator;
 
 import "dart:collection";
 
@@ skipping 18 matching lines @@
     String content = copyWithoutQuotes(source, leftQuote, rightQuote);
     return validateString(token,
                           token.charOffset + leftQuote,
                           content,
                           quoting);
   }
 
   static StringQuoting quotingFromString(String sourceString) {
     Iterator<int> source = sourceString.codeUnits.iterator;
     bool raw = false;
     int quoteLength = 1;

[Johnni Winther, 2014/06/18 08:43:44]

Rename to [leftQuoteLength]. Also in StringQuoting.getQuoting.

[floitsch, 2014/06/18 12:14:22]

Done.

     source.moveNext();
     int quoteChar = source.current;
     if (quoteChar == $r) {
       raw = true;
       source.moveNext();
       quoteChar = source.current;
     }
     assert(quoteChar == $SQ || quoteChar == $DQ);
     // String has at least one quote. Check it if has three.
-    // If it only have two, the string must be an empty string literal,
+    // If it only has two, the string must be an empty string literal,
     // and end after the second quote.
     bool multiline = false;
     if (source.moveNext() && source.current == quoteChar && source.moveNext()) {
       int code = source.current;
       assert(code == quoteChar);  // If not, there is a bug in the parser.
       quoteLength = 3;
-      // Check if a multiline string starts with a newline (CR, LF or CR+LF).
-      if (source.moveNext()) {
+      // Due to string-interpolations we are not guaranteed to see the trailing
+      // quoting characters. The invocations to `moveNext()` may therefore
+      // return false and the `current`-getter return `null`. The code does
+      // not need to handle this specially (as it will not find the newline
+      // characters).
+
+      // Check if a multiline string starts with optional whitespace followed by
+      // a newline (CR, LF or CR+LF).
+      // We also accept if the these characters are escaped by a backslash.
+      int newLineLength = 1;
+      bool foundWhitespace;
+      do {
+        foundWhitespace = false;

[Lasse Reichstein Nielsen, 2014/06/18 09:05:32]

This "break variable" is more confusing than a "while(true)" with explicit
breaks on newlines or non-whitespace.

If you want to keep it, just set it to true to begin with, and rename it to
"foundOnlyWhitespace" (or reverese its meaning as "foundNonWhitespace"), and
only set it to false in a final else branch, and use "break" in the
line-terminator cases. No need to change it back and forth each iteration.
Technically the newline cases are "wrong" now, because newlines are whitespace,
but foundWhitespace ends up false after seeing a newline.

[floitsch, 2014/06/18 12:14:22]

changed to while(true).
Initially switched away from it, since it ends with a 'break' now.

+        source.moveNext();

[Lasse Reichstein Nielsen, 2014/06/18 09:05:32]

Why is the return value of moveNext not checked?
If it works anyway, because a current returning null gives the correct result
anyway, please document it.

[floitsch, 2014/06/18 12:14:22]

Moved comment from line 56 to here.

         code = source.current;
-        if (code == $CR) {
-          quoteLength += 1;
+        if (code == $BACKSLASH) {
+          newLineLength++;
+          source.moveNext();
+          code = source.current;
+        }
+        if (code == $TAB || code == $SPACE) {
+          newLineLength++;
+          foundWhitespace = true;
+        } else if (code == $CR) {

[Lasse Reichstein Nielsen, 2014/06/18 09:05:32]

The spec says "If the first line of a multiline ...". What defines a line?
We are using either CRLF, CR or LF as line terminator. 

Ok, not a new issue, I just found http://dartbug.com/14073
We should keep doing what we are doing here: \r | \r\n | \n are line
terminators.

           if (source.moveNext() && source.current == $LF) {

[Lasse Reichstein Nielsen, 2014/06/18 09:05:32]

Does the spec require \\\r\\\n to work as a newline sequence? (I'd say "heck
no!", but then I wouldn't have allowed backslashes at all).
I think we can argue that \r\n is the line terminator, and \r\\\n is not, so \r
becomes the line terminator in that case, and the \\\n becomes part of the
string.

Ok, I filed http://dartbug.com/19514 for not allowing backslashes again.

-            quoteLength += 1;
+            newLineLength++;
           }
+          quoteLength += newLineLength;
         } else if (code == $LF) {
-          quoteLength += 1;
+          quoteLength += newLineLength;
         }
-      }
+      } while (foundWhitespace);
     }
     return StringQuoting.getQuoting(quoteChar, raw, quoteLength);

[Johnni Winther, 2014/06/18 08:43:44]

StringQuoting.getQuoting expects [quoteLength] to be limited.

[floitsch, 2014/06/18 12:14:23]

It shouldn't anymore. I changed it to be accept any quoteLength.

   }
 
   /**
    * Return the string [string] witout its [initial] first and [terminal] last
    * characters. This is intended to be used to remove quotes from string
    * literals (including an initial 'r' for raw strings).
    */
   String copyWithoutQuotes(String string, int initial, int terminal) {
     assert(0 <= initial);
     assert(0 <= terminal);
@@ skipping 126 matching lines @@
       return null;
     }
     // String literal successfully validated.
     if (quoting.raw || !containsEscape) {
       // A string without escapes could just as well have been raw.
       return new DartString.rawString(string, length);
     }
     return new DartString.escapedString(string, length);
   }
 }