From fd005e17bcdfbd20d94a1484d375707bd2a21538 Mon Sep 17 00:00:00 2001 From: Dan Rubel Date: Tue, 20 Mar 2018 12:01:26 +0000 Subject: [PATCH] Rework parsing of local function declarations This CL reworks parsing local variable and local function declarations to improve recovery and error messages. In addition, the fasta parser properly parses metadata before a local function declaration. Change-Id: Ia43c8bab3f3ce3b824390a4aa36d50b85f1ea9af Reviewed-on: https://dart-review.googlesource.com/46952 Commit-Queue: Dan Rubel Reviewed-by: Brian Wilkerson --- .../test/generated/parser_fasta_test.dart | 53 +--- pkg/analyzer/test/generated/parser_test.dart | 69 +++-- .../partial_code/local_variable_test.dart | 18 +- .../lib/src/fasta/parser/parser.dart | 238 +++++++++--------- .../src/fasta/parser/type_continuation.dart | 4 - 5 files changed, 167 insertions(+), 215 deletions(-) diff --git a/pkg/analyzer/test/generated/parser_fasta_test.dart b/pkg/analyzer/test/generated/parser_fasta_test.dart index b3465ea61d9f..1dd56a34bc84 100644 --- a/pkg/analyzer/test/generated/parser_fasta_test.dart +++ b/pkg/analyzer/test/generated/parser_fasta_test.dart @@ -568,6 +568,13 @@ class ErrorParserTest_Fasta extends FastaParserTestCase super.test_invalidUnicodeEscape_tooManyDigits_variable(); } + @override + @failingTest + void test_localFunction_annotation() { + // TODO(danrubel): Update fasta to allow metadata before local function. + super.test_localFunction_annotation(); + } + @override @failingTest void test_method_invalidTypeParameterComments() { @@ -679,38 +686,6 @@ class ErrorParserTest_Fasta extends FastaParserTestCase super.test_missingFunctionBody_invalid(); } - @override - @failingTest - void test_missingFunctionParameters_local_nonVoid_block() { - // TODO(brianwilkerson) Wrong errors: - // Expected 1 errors of type ParserErrorCode.MISSING_FUNCTION_PARAMETERS, found 0 - super.test_missingFunctionParameters_local_nonVoid_block(); - } - - @override - @failingTest - void test_missingFunctionParameters_local_nonVoid_expression() { - // TODO(brianwilkerson) Wrong errors: - // Expected 1 errors of type ParserErrorCode.MISSING_FUNCTION_PARAMETERS, found 0 - super.test_missingFunctionParameters_local_nonVoid_expression(); - } - - @override - @failingTest - void test_missingFunctionParameters_local_void_block() { - // TODO(brianwilkerson) Wrong errors: - // Expected 1 errors of type ParserErrorCode.MISSING_FUNCTION_PARAMETERS, found 0 - super.test_missingFunctionParameters_local_void_block(); - } - - @override - @failingTest - void test_missingFunctionParameters_local_void_expression() { - // TODO(brianwilkerson) Wrong errors: - // Expected 1 errors of type ParserErrorCode.MISSING_FUNCTION_PARAMETERS, found 0 - super.test_missingFunctionParameters_local_void_expression(); - } - @override @failingTest void test_missingFunctionParameters_topLevel_void_block() { @@ -1790,20 +1765,6 @@ class RecoveryParserTest_Fasta extends FastaParserTestCase super.test_functionExpression_named(); } - @override - @failingTest - void test_incompleteLocalVariable_beforeIdentifier() { - // TODO(brianwilkerson) reportUnrecoverableErrorWithToken - super.test_incompleteLocalVariable_beforeIdentifier(); - } - - @override - @failingTest - void test_incompleteLocalVariable_beforeKeyword() { - // TODO(brianwilkerson) reportUnrecoverableErrorWithToken - super.test_incompleteLocalVariable_beforeKeyword(); - } - @override @failingTest void test_incompleteTypeArguments_field() { diff --git a/pkg/analyzer/test/generated/parser_test.dart b/pkg/analyzer/test/generated/parser_test.dart index 353465d46773..07813d89e023 100644 --- a/pkg/analyzer/test/generated/parser_test.dart +++ b/pkg/analyzer/test/generated/parser_test.dart @@ -3130,15 +3130,10 @@ class Foo { void test_expectedToken_parseStatement_afterVoid() { parseStatement("void}", expectedEndOffset: 4); - listener.assertErrors(usingFastaParser - ? [ - expectedError(ParserErrorCode.MISSING_STATEMENT, 0, 4), - expectedError(ParserErrorCode.EXPECTED_TOKEN, 4, 1) - ] - : [ - expectedError(ParserErrorCode.EXPECTED_TOKEN, 4, 1), - expectedError(ParserErrorCode.MISSING_IDENTIFIER, 4, 1) - ]); + listener.assertErrors([ + expectedError(ParserErrorCode.EXPECTED_TOKEN, 4, 1), + expectedError(ParserErrorCode.MISSING_IDENTIFIER, 4, 1) + ]); } void test_expectedToken_semicolonMissingAfterExport() { @@ -3832,7 +3827,8 @@ class Wrong { expectNotNullIfNoErrors(statement); listener.assertErrors(usingFastaParser ? [ - expectedError(ParserErrorCode.EXPECTED_TOKEN, 7, 1), + expectedError(ParserErrorCode.EXPECTED_TOKEN, 8, 1), + expectedError(ParserErrorCode.MISSING_IDENTIFIER, 9, 1), expectedError(ParserErrorCode.EXPECTED_TOKEN, 10, 1) ] : [ @@ -4161,6 +4157,23 @@ class Wrong { ]); } + void test_localFunction_annotation() { + CompilationUnit unit = + parseCompilationUnit("class C { m() { @Foo f() {} } }"); + expect(unit.declarations, hasLength(1)); + ClassDeclaration declaration = unit.declarations[0]; + expect(declaration.members, hasLength(1)); + MethodDeclaration member = declaration.members[0]; + BlockFunctionBody body = member.body; + expect(body.block.statements, hasLength(1)); + FunctionDeclarationStatement statement = body.block.statements[0]; + if (usingFastaParser) { + expect(statement.functionDeclaration.metadata, hasLength(1)); + Annotation metadata = statement.functionDeclaration.metadata[0]; + expect(metadata.name.name, 'Foo'); + } + } + void test_method_invalidTypeParameterComments() { enableGenericMethodComments = true; createParser('void m/**/() {}'); @@ -4388,36 +4401,36 @@ class Wrong { [expectedError(ParserErrorCode.MISSING_FUNCTION_BODY, 0, 6)]); } - @failingTest void test_missingFunctionParameters_local_nonVoid_block() { // The parser does not recognize this as a function declaration, so it tries // to parse it as an expression statement. It isn't clear what the best // error message is in this case. - parseStatement("int f { return x;}"); - listener.assertErrors( - [expectedError(ParserErrorCode.MISSING_FUNCTION_PARAMETERS, 4, 1)]); + parseStatement("int f { return x;}", expectedEndOffset: 6); + listener + .assertErrors([expectedError(ParserErrorCode.EXPECTED_TOKEN, 6, 1)]); } - @failingTest void test_missingFunctionParameters_local_nonVoid_expression() { // The parser does not recognize this as a function declaration, so it tries // to parse it as an expression statement. It isn't clear what the best // error message is in this case. parseStatement("int f => x;"); - listener.assertErrors( - [expectedError(ParserErrorCode.MISSING_FUNCTION_PARAMETERS, 4, 1)]); + listener.assertErrors(usingFastaParser + ? [expectedError(ParserErrorCode.MISSING_FUNCTION_PARAMETERS, 6, 2)] + : [expectedError(ParserErrorCode.EXPECTED_TOKEN, 0, 3)]); } void test_missingFunctionParameters_local_void_block() { - parseStatement("void f { return x;}"); - listener.assertErrors( - [expectedError(ParserErrorCode.MISSING_FUNCTION_PARAMETERS, 5, 1)]); + parseStatement("void f { return x;}", expectedEndOffset: 7); + listener.assertErrors(usingFastaParser + ? [expectedError(ParserErrorCode.EXPECTED_TOKEN, 7, 1)] + : [expectedError(ParserErrorCode.MISSING_FUNCTION_PARAMETERS, 5, 1)]); } void test_missingFunctionParameters_local_void_expression() { parseStatement("void f => x;"); listener.assertErrors( - [expectedError(ParserErrorCode.MISSING_FUNCTION_PARAMETERS, 5, 1)]); + [expectedError(ParserErrorCode.MISSING_FUNCTION_PARAMETERS, 7, 2)]); } void test_missingFunctionParameters_topLevel_nonVoid_block() { @@ -4675,8 +4688,9 @@ class Wrong { void test_missingStatement_afterVoid() { parseStatement("void;"); - listener - .assertErrors([expectedError(ParserErrorCode.MISSING_STATEMENT, 0, 4)]); + listener.assertErrors(usingFastaParser + ? [expectedError(ParserErrorCode.MISSING_IDENTIFIER, 4, 1)] + : [expectedError(ParserErrorCode.MISSING_STATEMENT, 4, 1)]); } void test_missingTerminatorForParameterGroup_named() { @@ -15400,6 +15414,15 @@ abstract class StatementParserTestMixin implements AbstractParserTestCase { expect(variableList.variables, hasLength(1)); } + void test_parseVariableDeclaration_equals_builtIn() { + VariableDeclarationStatement statement = parseStatement('int set = 0;'); + assertNoErrors(); + expect(statement.semicolon, isNotNull); + VariableDeclarationList variableList = statement.variables; + expect(variableList, isNotNull); + expect(variableList.variables, hasLength(1)); + } + void test_parseWhileStatement() { var statement = parseStatement('while (x) {}') as WhileStatement; assertNoErrors(); diff --git a/pkg/analyzer/test/src/fasta/recovery/partial_code/local_variable_test.dart b/pkg/analyzer/test/src/fasta/recovery/partial_code/local_variable_test.dart index 4c2f0103833f..641e476920a9 100644 --- a/pkg/analyzer/test/src/fasta/recovery/partial_code/local_variable_test.dart +++ b/pkg/analyzer/test/src/fasta/recovery/partial_code/local_variable_test.dart @@ -157,23 +157,7 @@ class LocalVariableTest extends PartialCodeTest { "int _s_;", allFailing: true), new TestDescriptor( - 'typeName', 'int a', [ParserErrorCode.EXPECTED_TOKEN], "int a;", - failing: [ - 'assert', - 'break', - 'continue', - 'do', - 'if', - 'for', - 'labeled', - 'localFunctionNonVoid', - 'localFunctionVoid', - 'localVariable', - 'switch', - 'try', - 'return', - 'while' - ]), + 'typeName', 'int a', [ParserErrorCode.EXPECTED_TOKEN], "int a;"), new TestDescriptor( 'var', 'var', diff --git a/pkg/front_end/lib/src/fasta/parser/parser.dart b/pkg/front_end/lib/src/fasta/parser/parser.dart index d48c0a3c67f3..296347cab7cc 100644 --- a/pkg/front_end/lib/src/fasta/parser/parser.dart +++ b/pkg/front_end/lib/src/fasta/parser/parser.dart @@ -99,7 +99,8 @@ import 'type_info.dart' computeType, isGeneralizedFunctionType, isValidTypeReference, - noTypeInfo; + noTypeInfo, + voidTypeInfo; import 'util.dart' show optional; @@ -2494,98 +2495,6 @@ class Parser { } continue optional; - case TypeContinuation.ExpressionStatementOrDeclaration: - assert(begin.isIdentifier || identical(begin.stringValue, 'void')); - if (!inPlainSync && optional("await", begin)) { - return parseExpressionStatement(beforeBegin); - } - - if (looksLikeType && token.isIdentifier) { - Token afterId = token.next; - - int afterIdKind = afterId.kind; - if (looksLikeVariableDeclarationEnd(afterIdKind)) { - // We are looking at `type identifier` followed by - // `(',' | '=' | ';')`. - - // TODO(ahe): Generate type events and call - // parseVariablesDeclarationRest instead. - return parseVariablesDeclaration(beforeBegin); - } else if (OPEN_PAREN_TOKEN == afterIdKind) { - // We are looking at `type identifier '('`. - if (looksLikeFunctionBody(afterId.endGroup.next)) { - // We are looking at `type identifier '(' ... ')'` followed - // `( '{' | '=>' | 'async' | 'sync' )`. - - // Although it looks like there are no type variables here, they - // may get injected from a comment. - Token beforeFormals = parseTypeVariablesOpt(token); - - listener.beginLocalFunctionDeclaration(begin); - if (voidToken != null) { - listener.handleVoidKeyword(voidToken); - } else { - commitType(); - } - return parseNamedFunctionRest( - beforeToken, begin, beforeFormals, false); - } - } else if (identical(afterIdKind, LT_TOKEN)) { - // We are looking at `type identifier '<'`. - Token beforeFormals = afterId.endGroup; - if (beforeFormals?.next != null && - optional("(", beforeFormals.next)) { - if (looksLikeFunctionBody(beforeFormals.next.endGroup.next)) { - // We are looking at "type identifier '<' ... '>' '(' ... ')'" - // followed by '{', '=>', 'async', or 'sync'. - parseTypeVariablesOpt(token); - listener.beginLocalFunctionDeclaration(begin); - if (voidToken != null) { - listener.handleVoidKeyword(voidToken); - } else { - commitType(); - } - return parseNamedFunctionRest( - beforeToken, begin, beforeFormals, false); - } - } - } - // Fall-through to expression statement. - } else { - beforeToken = beforeBegin; - token = begin; - if (optional(':', token.next)) { - return parseLabeledStatement(beforeToken); - } else if (optional('(', token.next)) { - if (looksLikeFunctionBody(token.next.endGroup.next)) { - // We are looking at `identifier '(' ... ')'` followed by `'{'`, - // `'=>'`, `'async'`, or `'sync'`. - - // Although it looks like there are no type variables here, they - // may get injected from a comment. - Token formals = parseTypeVariablesOpt(token); - - listener.beginLocalFunctionDeclaration(token); - listener.handleNoType(token); - return parseNamedFunctionRest(beforeToken, begin, formals, false); - } - } else if (optional('<', token.next)) { - Token gt = token.next.endGroup; - if (gt?.next != null && optional("(", gt.next)) { - if (looksLikeFunctionBody(gt.next.endGroup.next)) { - // We are looking at `identifier '<' ... '>' '(' ... ')'` - // followed by `'{'`, `'=>'`, `'async'`, or `'sync'`. - parseTypeVariablesOpt(token); - listener.beginLocalFunctionDeclaration(token); - listener.handleNoType(token); - return parseNamedFunctionRest(beforeToken, begin, gt, false); - } - } - // Fall through to expression statement. - } - } - return parseExpressionStatement(beforeBegin); - case TypeContinuation.ExpressionStatementOrConstDeclaration: Token identifier; if (looksLikeType && token.isIdentifier) { @@ -3990,7 +3899,7 @@ class Parser { beforeName.next, fasta.messageNamedFunctionExpression); } listener.endFunctionName(begin, token); - token = parseFormalParametersOpt(formals, MemberKind.Local); + token = parseFormalParametersRequiredOpt(formals, MemberKind.Local); token = parseInitializersOpt(token); token = parseAsyncOptBody(token, isFunctionExpression, false); if (isFunctionExpression) { @@ -4278,10 +4187,14 @@ class Parser { } Token parseStatementX(Token token) { - final value = token.next.stringValue; if (identical(token.next.kind, IDENTIFIER_TOKEN)) { + if (optional(':', token.next.next)) { + return parseLabeledStatement(token); + } return parseExpressionStatementOrDeclaration(token); - } else if (identical(value, '{')) { + } + final value = token.next.stringValue; + if (identical(value, '{')) { return parseBlock(token); } else if (identical(value, 'return')) { return parseReturnStatement(token); @@ -4334,10 +4247,15 @@ class Parser { return parseExpressionStatementOrConstDeclaration(token); } else if (isModifier(token.next)) { return parseVariablesDeclaration(token); + } else if (!inPlainSync && identical(value, 'await')) { + return parseExpressionStatement(token); } else if (token.next.isIdentifier) { + if (optional(':', token.next.next)) { + return parseLabeledStatement(token); + } return parseExpressionStatementOrDeclaration(token); } else if (identical(value, '@')) { - return parseVariablesDeclaration(token); + return parseExpressionStatementOrDeclaration(token); } else { return parseExpressionStatement(token); } @@ -4386,10 +4304,6 @@ class Parser { return token; } - Token parseExpressionStatementOrDeclaration(Token token) { - return parseType(token, TypeContinuation.ExpressionStatementOrDeclaration); - } - Token parseExpressionStatementOrConstDeclaration(Token token) { Token next = token.next; assert(optional('const', next)); @@ -4424,9 +4338,7 @@ class Parser { /// ``` Token parseLabeledStatement(Token token) { Token next = token.next; - // TODO(brianwilkerson): Enable this assert. - // `parseType` is allowing `void` to be a label. -// assert(next.isIdentifier); + assert(next.isIdentifier); assert(optional(':', next.next)); int labelCount = 0; do { @@ -5487,10 +5399,46 @@ class Parser { return token; } - /// Parse the metadata, modifiers, and type of a local declaration and return - /// the last token consumed. If a local variable declaration or local - /// function declaration is not found, then return the starting token. - Token parseLocalDeclarationStartOpt(final Token start) { + /// Returns true if [token] could be the start of a function declaration + /// without a return type. + bool looksLikeLocalFunction(Token token) { + if (token.isIdentifier) { + token = token.next; + if (optional('<', token)) { + Token closeBrace = token.endGroup; + if (closeBrace == null) { + return false; + } + token = closeBrace.next; + } + if (optional('(', token)) { + token = token.endGroup.next; + return optional('{', token) || + optional('=>', token) || + optional('async', token) || + optional('sync', token); + } else if (optional('=>', token)) { + // Recovery: Looks like a local function that is missing parenthesis. + return true; + } + } + return false; + } + + /// This method has two modes based upon [onlyParseVariableDeclarationStart]. + /// + /// If [onlyParseVariableDeclarationStart] is `false` (the default) then this + /// method will parse a local variable declaration, a local function, + /// or an expression statement, and then return the last consumed token. + /// + /// If [onlyParseVariableDeclarationStart] is `true` then this method + /// will only parse the metadata, modifiers, and type of a local variable + /// declaration if it exists. It is the responsibility of the caller to + /// call [parseVariablesDeclarationRest] to finish parsing the local variable + /// declaration. If a local variable declaration is not found then this + /// method will return [start]. + Token parseExpressionStatementOrDeclaration(final Token start, + [bool onlyParseVariableDeclarationStart = false]) { Token token = start; Token next = token.next; if (optional('@', next)) { @@ -5526,31 +5474,54 @@ class Parser { token = typeInfo.skipType(beforeType); next = token.next; - if (next.isIdentifier) { - if (optional('<', next.next) || optional('(', next.next)) { - // Found an expression or local function declaration. - // TODO(danrubel): Process local function declarations. + if (onlyParseVariableDeclarationStart) { + if (token == start) { + // If no annotation, modifier, or type, then return without consuming + // any tokens because this is not a local variable declaration. return start; } + // Fall through to parse the start of a local variable declaration. + } else if (looksLikeLocalFunction(next) && + // TODO(danrubel): allow metadata before local function + !optional('@', start.next)) { + // Parse a local function declaration. + if (varFinalOrConst != null) { + reportRecoverableErrorWithToken( + varFinalOrConst, fasta.templateExtraneousModifier); + } + Token beforeFormals = parseTypeVariablesOpt(next); + listener.beginLocalFunctionDeclaration(start.next); + token = typeInfo.parseType(beforeType, this); + next = token.next; + return parseNamedFunctionRest(token, start.next, beforeFormals, false); } if (token == start) { // If no annotation, modifier, or type, and this is not a local function - // then return without consuming any tokens because this is not - // a local declaration. - return start; - } - - if (!optional('@', start.next)) { - listener.beginMetadataStar(start.next); - listener.endMetadataStar(0); + // then this must be an expression statement. + return parseExpressionStatement(start); + } + if (next.type.isBuiltIn && + beforeType == start && + typeInfo != voidTypeInfo) { + // Detect situations such as identifier `as` identifier + // and treat those as expressions. + int kind = next.next.kind; + if (EQ_TOKEN != kind && SEMICOLON_TOKEN != kind && COMMA_TOKEN != kind) { + if (onlyParseVariableDeclarationStart) { + if (!optional('in', next.next)) { + return start; + } + } else { + return parseExpressionStatement(start); + } + } } - token = typeInfo.parseType(beforeType, this); - next = token.next; - // If there is not an identifier, then allow ensureIdentifier to report an - // error and don't report errors here. if (next.isIdentifier) { + // Only report these errors if there is an identifier. If there is not an + // identifier, then allow ensureIdentifier to report an error + // and don't report errors here. if (varFinalOrConst == null) { if (typeInfo == noTypeInfo) { reportRecoverableError(next, fasta.messageMissingConstFinalVarOrType); @@ -5562,7 +5533,16 @@ class Parser { } } + if (!optional('@', start.next)) { + listener.beginMetadataStar(start.next); + listener.endMetadataStar(0); + } + token = typeInfo.parseType(beforeType, this); + next = token.next; listener.beginVariablesDeclaration(next, varFinalOrConst); + if (!onlyParseVariableDeclarationStart) { + token = parseVariablesDeclarationRest(token, true); + } return token; } @@ -5708,8 +5688,15 @@ class Parser { } token = leftParenthesis; - token = parseLocalDeclarationStartOpt(token); + // Pass `true` so that the [parseExpressionStatementOrDeclaration] only + // parses the metadata, modifiers, and type of a local variable + // declaration if it exists. This enables capturing [beforeIdentifier] + // for later error reporting. + token = parseExpressionStatementOrDeclaration(token, true); Token beforeIdentifier = token; + + // Parse the remainder of the local variable declaration + // or an expression if no local variable declaration was found. if (token != leftParenthesis) { token = parseVariablesDeclarationRest(token, false); } else if (optional(';', token.next)) { @@ -5721,6 +5708,7 @@ class Parser { Token next = token.next; if (!optional('in', next)) { if (optional(':', next)) { + // Recovery reportRecoverableError(next, fasta.messageColonInPlaceOfIn); // Fall through to process `for ( ... in ... )` } else { diff --git a/pkg/front_end/lib/src/fasta/parser/type_continuation.dart b/pkg/front_end/lib/src/fasta/parser/type_continuation.dart index 165353e66f17..4f80ee93b9ba 100644 --- a/pkg/front_end/lib/src/fasta/parser/type_continuation.dart +++ b/pkg/front_end/lib/src/fasta/parser/type_continuation.dart @@ -31,10 +31,6 @@ enum TypeContinuation { /// should parse the following as a type unless it is followed by `=`. Typedef, - /// Indicates that what follows is either a local declaration or an - /// expression. - ExpressionStatementOrDeclaration, - /// Indicates that the keyword `const` has just been seen, and what follows /// may be a local variable declaration or an expression. ExpressionStatementOrConstDeclaration,