Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9006033:
Issue926: NPE while traversing nodes + invalid string interpolation (Closed)
|
Created:
9 years ago by codefu Modified:
8 years, 12 months ago CC:
reviews_dartlang.org, mmendez Visibility:
Public. |
DescriptionIssue926: NPE while traversing nodes + invalid string interpolation
http://code.google.com/p/dart/issues/detail?id=926
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=2866
Patch Set 1 #Patch Set 2 : Adding test #
Total comments: 5
Patch Set 3 : Comments + Test changes #
Total comments: 18
Patch Set 4 : Not ready for review #
Messages
Total messages: 9 (0 generated)
http://codereview.chromium.org/9006033/diff/1001/compiler/java/com/google/dar... File compiler/java/com/google/dart/compiler/parser/DartParser.java (right): http://codereview.chromium.org/9006033/diff/1001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2121: if (!expect(Token.STRING_EMBED_EXP_END)) { Seems odd there is no error reporting here. http://codereview.chromium.org/9006033/diff/1001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2122: return done(new DartSyntheticErrorExpression()); also odd that this returns straight away. Maybe we could still return a valid DartStringInterpolation node by adding a dummy string and error node into the expressions list instead of a synthetic error for the entire expression? http://codereview.chromium.org/9006033/diff/1001/tests/language/src/StringInt... File tests/language/src/StringInterpolateNPENegativeTest.dart (right): http://codereview.chromium.org/9006033/diff/1001/tests/language/src/StringInt... tests/language/src/StringInterpolateNPENegativeTest.dart:7: print("$ this isn't really a language test since the code shouldn't even parse. I think it would be better placed in a JUnit test, say NegativeParserTest.java
Modifications to make the string interpolation more AST friendly and less assert happy. http://codereview.chromium.org/9006033/diff/1001/compiler/java/com/google/dar... File compiler/java/com/google/dart/compiler/parser/DartParser.java (right): http://codereview.chromium.org/9006033/diff/1001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2121: if (!expect(Token.STRING_EMBED_EXP_END)) { On 2011/12/21 13:24:24, zundel wrote: > Seems odd there is no error reporting here. Indeed, I didn't even notice it. I'll see if there's a test that exercises this. http://codereview.chromium.org/9006033/diff/1001/tests/language/src/StringInt... File tests/language/src/StringInterpolateNPENegativeTest.dart (right): http://codereview.chromium.org/9006033/diff/1001/tests/language/src/StringInt... tests/language/src/StringInterpolateNPENegativeTest.dart:7: print("$ On 2011/12/21 13:24:24, zundel wrote: > this isn't really a language test since the code shouldn't even parse. I think > it would be better placed in a JUnit test, say NegativeParserTest.java That was my first thought; I placed it here to share with the other implementations. Now that I think about it, this is more analysis driven and belongs in JUnit.
lgtm
http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... File compiler/java/com/google/dart/compiler/parser/DartParser.java (right): http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2160: && lookAhead.getSyntax().length() > 0 ? lookAhead.getSyntax() : null; nit: I don't like chaining multiple ternary operators together in one expression. I think it would be more readable as if stmts. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2161: if(errorText != null) { missing space between 'if' and ( http://codereview.chromium.org/9006033/diff/6001/compiler/javatests/com/googl... File compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java (right): http://codereview.chromium.org/9006033/diff/6001/compiler/javatests/com/googl... compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java:319: " print(\"$\");", what happens in this case? print(\"${\"); http://codereview.chromium.org/9006033/diff/6001/compiler/javatests/com/googl... compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java:322: errEx(ParserErrorCode.UNEXPECTED_TOKEN, 2, 22, 1), print(\"1 ${42} 2 ${} 3\"); ^ Rather than 'unexpected token' in this case, could we report something more helpful, like UNEXPECTED_TOKEN_IN_STRING_INTERPOLATION, or some other error related to string interpolation. I'm thinking where the developer unsuspectingly includes ${} as a regular string and has forgotten to escape the $
I really like moving some of the logic into a builder. If my comments are off-base, feel free to ignore them. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... File compiler/java/com/google/dart/compiler/parser/DartParser.java (right): http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2077: void add(DartStringLiteral string) { Given that string literals are also expressions, you might want to change the names of these methods to avoid errors based on accidental overloading. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2123: while (inString) { // Iterate until we find the last string segment. We could make the parser more robust by also ending single-line strings at the end of the line even if we don't find a STRING_LAST_SEGMENT, but I don't think we have end of line information from the scanner. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2152: DartExpression expr = parseExpression(); nit: I'd get rid of the local variable. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2161: if(errorText != null) { I'm probably confused about the sequence of tokens that will be returned, but this seems wrong. If we've parsed an expression and don't have the closing bracket, don't we need to either always or never add an error expression? Under what conditions will we get an empty token with an empty syntax? http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2164: inString = !(Token.STRING_LAST_SEGMENT == lookAhead); Can this condition actually happen? I would have thought that the scanner would not produce a STRING_LAST_SEGMENT if we are inside a string interpolation block. Won't it just interpret the next quote as the beginning of an embedded string. If this condition can happen, then won't this prevent us from adding the last segment to the builder?
http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... File compiler/java/com/google/dart/compiler/parser/DartParser.java (right): http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2077: void add(DartStringLiteral string) { On 2011/12/21 19:33:44, Brian Wilkerson wrote: > Given that string literals are also expressions, you might want to change the > names of these methods to avoid errors based on accidental overloading. Done. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2123: while (inString) { // Iterate until we find the last string segment. On 2011/12/21 19:33:44, Brian Wilkerson wrote: > We could make the parser more robust by also ending single-line strings at the > end of the line even if we don't find a STRING_LAST_SEGMENT, but I don't think > we have end of line information from the scanner. We have the position of each token, which encodes the line number. The scanner inserts a Token.ILLEGAL when it detects a new line in a single line string. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2152: DartExpression expr = parseExpression(); On 2011/12/21 19:33:44, Brian Wilkerson wrote: > nit: I'd get rid of the local variable. Done. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2160: && lookAhead.getSyntax().length() > 0 ? lookAhead.getSyntax() : null; On 2011/12/21 19:29:28, zundel wrote: > nit: I don't like chaining multiple ternary operators together in one > expression. I think it would be more readable as if stmts. Done. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2161: if(errorText != null) { On 2011/12/21 19:29:28, zundel wrote: > missing space between 'if' and ( Done. http://codereview.chromium.org/9006033/diff/6001/compiler/java/com/google/dar... compiler/java/com/google/dart/compiler/parser/DartParser.java:2164: inString = !(Token.STRING_LAST_SEGMENT == lookAhead); On 2011/12/21 19:33:44, Brian Wilkerson wrote: > Can this condition actually happen? I would have thought that the scanner would > not produce a STRING_LAST_SEGMENT if we are inside a string interpolation block. > Won't it just interpret the next quote as the beginning of an embedded string. > If this condition can happen, then won't this prevent us from adding the last > segment to the builder? The scanner did insert a STRING_LAST_SEGMENT, and we weren't consuming it, eventually hitting the assert at the top of the function the next time through. http://codereview.chromium.org/9006033/diff/6001/compiler/javatests/com/googl... File compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java (right): http://codereview.chromium.org/9006033/diff/6001/compiler/javatests/com/googl... compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java:319: " print(\"$\");", On 2011/12/21 19:29:28, zundel wrote: > what happens in this case? > print(\"${\"); I can easily add it: StringIllegalEmbedEnd.dart/StringIllegalEmbedEnd.dart:7: Unexpected token in string interpolation: ILLEGAL 6: print("$"); 7: print("${"); But it does continue parsing as if its in a string, so the errors after it are off. The runs back to searching for Illegal and splitting the string. http://codereview.chromium.org/9006033/diff/6001/compiler/javatests/com/googl... compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java:322: errEx(ParserErrorCode.UNEXPECTED_TOKEN, 2, 22, 1), On 2011/12/21 19:29:28, zundel wrote: > print(\"1 ${42} 2 ${} 3\"); > ^ > > Rather than 'unexpected token' in this case, could we report something more > helpful, like UNEXPECTED_TOKEN_IN_STRING_INTERPOLATION, or some other error > related to string interpolation. I'm thinking where the developer > unsuspectingly includes ${} as a regular string and has forgotten to escape the > $ The parseExpression() exercised in this code consumes the } and reports the error.
I addressed most of the nits and comments from the previous patch. I spent some
time looking at the interesting case of:
print("${");
And concluded that needs to be filed as another bug.
PTAL at the remaining patch.
http://codereview.chromium.org/9006033/diff/6001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java
(right):
http://codereview.chromium.org/9006033/diff/6001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java:319:
" print(\"$\");",
On 2011/12/21 23:10:30, codefu wrote:
> On 2011/12/21 19:29:28, zundel wrote:
> > what happens in this case?
> > print(\"${\");
Looking at this further, the scanner believes its still in a string embedded
expression. While experimenting with resting the string stack in the scanner
for non-multiline strings when hitting "illegal" (end of line), I've come to
believe there's much more work that has to be done to produce a clean AST,
beyond the scope of this bug/cl
lgtm (make sure you file an issue for the other problem you found.)
On 2011/12/22 19:12:59, zundel wrote: > lgtm > > (make sure you file an issue for the other problem you found.) Done: http://code.google.com/p/dart/issues/detail?id=946 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
