From 1b122924e8d3e519476934dee43bd0e232ad9e17 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Mon, 14 Jun 2021 16:12:14 -0700 Subject: [PATCH] [toolchain] Parse postfix operator `*` as a type operator. (#576) --- toolchain/diagnostics/diagnostic_emitter.h | 2 +- toolchain/driver/driver_test.cpp | 4 + toolchain/lexer/token_registry.def | 2 + toolchain/lexer/tokenized_buffer.cpp | 40 ++++- toolchain/lexer/tokenized_buffer.h | 12 +- toolchain/lexer/tokenized_buffer_test.cpp | 49 +++++- toolchain/parser/parse_tree_test.cpp | 107 +++++++++++++ toolchain/parser/parser_impl.cpp | 171 ++++++++++++++++++++- toolchain/parser/parser_impl.h | 14 ++ toolchain/parser/precedence.cpp | 32 ++-- toolchain/parser/precedence.h | 7 +- toolchain/parser/precedence_test.cpp | 121 ++++++++++----- 12 files changed, 488 insertions(+), 73 deletions(-) diff --git a/toolchain/diagnostics/diagnostic_emitter.h b/toolchain/diagnostics/diagnostic_emitter.h index e5ac9cb40bc6..1edd78d5cccb 100644 --- a/toolchain/diagnostics/diagnostic_emitter.h +++ b/toolchain/diagnostics/diagnostic_emitter.h @@ -153,7 +153,7 @@ struct SimpleDiagnostic { // produced. class ErrorTrackingDiagnosticConsumer : public DiagnosticConsumer { public: - ErrorTrackingDiagnosticConsumer(DiagnosticConsumer& next_consumer) + explicit ErrorTrackingDiagnosticConsumer(DiagnosticConsumer& next_consumer) : next_consumer(&next_consumer) {} auto HandleDiagnostic(const Diagnostic& diagnostic) -> void override { diff --git a/toolchain/driver/driver_test.cpp b/toolchain/driver/driver_test.cpp index 46fefaac7ad4..2ddce35ac198 100644 --- a/toolchain/driver/driver_test.cpp +++ b/toolchain/driver/driver_test.cpp @@ -164,6 +164,8 @@ TEST(DriverTest, DumpTokens) { EXPECT_THAT(&*token_it, IsKeyValueScalars("spelling", "Hello")); ++token_it; EXPECT_THAT(&*token_it, IsKeyValueScalars("identifier", "0")); + ++token_it; + EXPECT_THAT(&*token_it, IsKeyValueScalars("has_trailing_space", "true")); EXPECT_THAT(++token_it, Eq(token_value_node->end())); ++mapping_it; @@ -189,6 +191,8 @@ TEST(DriverTest, DumpTokens) { EXPECT_THAT(&*token_it, IsKeyValueScalars("spelling", "World")); ++token_it; EXPECT_THAT(&*token_it, IsKeyValueScalars("identifier", "1")); + ++token_it; + EXPECT_THAT(&*token_it, IsKeyValueScalars("has_trailing_space", "true")); EXPECT_THAT(++token_it, Eq(token_value_node->end())); ++mapping_it; diff --git a/toolchain/lexer/token_registry.def b/toolchain/lexer/token_registry.def index 62698d1ae11b..7814d2b8e23a 100644 --- a/toolchain/lexer/token_registry.def +++ b/toolchain/lexer/token_registry.def @@ -92,6 +92,7 @@ CARBON_SYMBOL_TOKEN(Tilde, "~") // clang-format on CARBON_OPENING_GROUP_SYMBOL_TOKEN(OpenParen, "(", CloseParen) CARBON_OPENING_GROUP_SYMBOL_TOKEN(OpenCurlyBrace, "{", CloseCurlyBrace) +CARBON_OPENING_GROUP_SYMBOL_TOKEN(OpenSquareBracket, "[", CloseSquareBracket) // clang-format off #undef CARBON_OPENING_GROUP_SYMBOL_TOKEN @@ -102,6 +103,7 @@ CARBON_OPENING_GROUP_SYMBOL_TOKEN(OpenCurlyBrace, "{", CloseCurlyBrace) // clang-format on CARBON_CLOSING_GROUP_SYMBOL_TOKEN(CloseParen, ")", OpenParen) CARBON_CLOSING_GROUP_SYMBOL_TOKEN(CloseCurlyBrace, "}", OpenCurlyBrace) +CARBON_CLOSING_GROUP_SYMBOL_TOKEN(CloseSquareBracket, "]", OpenSquareBracket) // clang-format off #undef CARBON_CLOSING_GROUP_SYMBOL_TOKEN diff --git a/toolchain/lexer/tokenized_buffer.cpp b/toolchain/lexer/tokenized_buffer.cpp index 212026052e96..64f2fdb2e0cb 100644 --- a/toolchain/lexer/tokenized_buffer.cpp +++ b/toolchain/lexer/tokenized_buffer.cpp @@ -146,7 +146,15 @@ class TokenizedBuffer::Lexer { set_indent = false; } + auto NoteWhitespace() -> void { + if (!buffer.token_infos.empty()) { + buffer.token_infos.back().has_trailing_space = true; + } + } + auto SkipWhitespace(llvm::StringRef& source_text) -> bool { + const char* const whitespace_start = source_text.begin(); + while (!source_text.empty()) { // We only support line-oriented commenting and lex comments as-if they // were whitespace. @@ -174,6 +182,9 @@ class TokenizedBuffer::Lexer { // If we find a non-whitespace character without exhausting the // buffer, return true to continue lexing. assert(!IsSpace(source_text.front())); + if (whitespace_start != source_text.begin()) { + NoteWhitespace(); + } return true; case '\n': @@ -374,13 +385,17 @@ class TokenizedBuffer::Lexer { open_groups.pop_back(); token_emitter.EmitError(opening_token); + assert(!buffer.Tokens().empty() && "Must have a prior opening token!"); + Token prev_token = buffer.Tokens().end()[-1]; + // TODO: do a smarter backwards scan for where to put the closing // token. - Token closing_token = - buffer.AddToken({.kind = opening_kind.GetClosingSymbol(), - .is_recovery = true, - .token_line = current_line, - .column = current_column}); + Token closing_token = buffer.AddToken( + {.kind = opening_kind.GetClosingSymbol(), + .has_trailing_space = buffer.HasTrailingWhitespace(prev_token), + .is_recovery = true, + .token_line = current_line, + .column = current_column}); TokenInfo& opening_token_info = buffer.GetTokenInfo(opening_token); TokenInfo& closing_token_info = buffer.GetTokenInfo(closing_token); opening_token_info.closing_token = closing_token; @@ -502,6 +517,9 @@ auto TokenizedBuffer::Lex(SourceBuffer& source, DiagnosticConsumer& consumer) assert(result && "No token was lexed."); } + // The end-of-file token is always considered to be whitespace. + lexer.NoteWhitespace(); + lexer.CloseInvalidOpenGroups(TokenKind::Error()); lexer.AddEndOfFileToken(); @@ -627,6 +645,15 @@ auto TokenizedBuffer::GetMatchedOpeningToken(Token closing_token) const return closing_token_info.opening_token; } +auto TokenizedBuffer::HasLeadingWhitespace(Token token) const -> bool { + auto it = TokenIterator(token); + return it == Tokens().begin() || GetTokenInfo(*(it - 1)).has_trailing_space; +} + +auto TokenizedBuffer::HasTrailingWhitespace(Token token) const -> bool { + return GetTokenInfo(token).has_trailing_space; +} + auto TokenizedBuffer::IsRecoveryToken(Token token) const -> bool { return GetTokenInfo(token).is_recovery; } @@ -733,6 +760,9 @@ auto TokenizedBuffer::PrintToken(llvm::raw_ostream& output_stream, Token token, } // TODO: Include value for numeric literals. + if (token_info.has_trailing_space) { + output_stream << ", has_trailing_space: true"; + } if (token_info.is_recovery) { output_stream << ", recovery: true"; } diff --git a/toolchain/lexer/tokenized_buffer.h b/toolchain/lexer/tokenized_buffer.h index 9e0a785d7841..d5001eb5ba51 100644 --- a/toolchain/lexer/tokenized_buffer.h +++ b/toolchain/lexer/tokenized_buffer.h @@ -166,7 +166,7 @@ class TokenizedBuffer { // Random-access iterator over tokens within the buffer. class TokenIterator : public llvm::iterator_facade_base< - TokenIterator, std::random_access_iterator_tag, Token, int> { + TokenIterator, std::random_access_iterator_tag, const Token, int> { public: TokenIterator() = default; @@ -180,8 +180,8 @@ class TokenizedBuffer { } auto operator*() const -> const Token& { return token; } - auto operator*() -> Token& { return token; } + using iterator_facade_base::operator-; auto operator-(const TokenIterator& rhs) const -> int { return token.index - rhs.token.index; } @@ -303,6 +303,11 @@ class TokenizedBuffer { // The given token must be a closing token kind. [[nodiscard]] auto GetMatchedOpeningToken(Token closing_token) const -> Token; + // Returns whether the given token has leading whitespace. + [[nodiscard]] auto HasLeadingWhitespace(Token token) const -> bool; + // Returns whether the given token has trailing whitespace. + [[nodiscard]] auto HasTrailingWhitespace(Token token) const -> bool; + // Returns whether the token was created as part of an error recovery effort. // // For example, a closing paren inserted to match an unmatched paren. @@ -380,6 +385,9 @@ class TokenizedBuffer { struct TokenInfo { TokenKind kind; + // Whether the token has trailing whitespace. + bool has_trailing_space = false; + // Whether the token was injected artificially during error recovery. bool is_recovery = false; diff --git a/toolchain/lexer/tokenized_buffer_test.cpp b/toolchain/lexer/tokenized_buffer_test.cpp index 89b6987b6b57..c5b162875cd5 100644 --- a/toolchain/lexer/tokenized_buffer_test.cpp +++ b/toolchain/lexer/tokenized_buffer_test.cpp @@ -543,6 +543,40 @@ TEST_F(LexerTest, MismatchedGroups) { })); } +TEST_F(LexerTest, Whitespace) { + auto buffer = Lex("{( } {("); + + // Whether there should be whitespace before/after each token. + bool space[] = {true, + // { + false, + // ( + true, + // inserted ) + true, + // } + true, + // { + false, + // ( + true, + // inserted ) + true, + // inserted } + true, + // EOF + false}; + int pos = 0; + for (TokenizedBuffer::Token token : buffer.Tokens()) { + ASSERT_LT(pos, std::size(space)); + EXPECT_THAT(buffer.HasLeadingWhitespace(token), Eq(space[pos])); + ++pos; + ASSERT_LT(pos, std::size(space)); + EXPECT_THAT(buffer.HasTrailingWhitespace(token), Eq(space[pos])); + } + ASSERT_EQ(pos + 1, std::size(space)); +} + TEST_F(LexerTest, Keywords) { auto buffer = Lex(" fn"); EXPECT_FALSE(buffer.HasErrors()); @@ -861,7 +895,7 @@ TEST_F(LexerTest, Printing) { llvm::StringRef print = print_stream.str(); EXPECT_THAT(GetAndDropLine(print), StrEq("token: { index: 0, kind: 'Semi', line: 1, column: 1, " - "indent: 1, spelling: ';' }")); + "indent: 1, spelling: ';', has_trailing_space: true }")); EXPECT_THAT(GetAndDropLine(print), StrEq("token: { index: 1, kind: 'EndOfFile', line: 1, column: 2, " "indent: 1, spelling: '' }")); @@ -887,7 +921,8 @@ TEST_F(LexerTest, Printing) { "6, indent: 1, spelling: ';' }")); EXPECT_THAT(GetAndDropLine(print), StrEq("token: { index: 4, kind: 'CloseParen', line: 1, column: " - "7, indent: 1, spelling: ')', opening_token: 0 }")); + "7, indent: 1, spelling: ')', opening_token: 0, " + "has_trailing_space: true }")); EXPECT_THAT(GetAndDropLine(print), StrEq("token: { index: 5, kind: 'EndOfFile', line: 1, column: " "8, indent: 1, spelling: '' }")); @@ -902,7 +937,7 @@ TEST_F(LexerTest, Printing) { EXPECT_THAT( GetAndDropLine(print), StrEq("token: { index: 0, kind: 'Semi', line: 1, column: 1, " - "indent: 1, spelling: ';' }")); + "indent: 1, spelling: ';', has_trailing_space: true }")); EXPECT_THAT( GetAndDropLine(print), StrEq("token: { index: 1, kind: 'Semi', line: 11, column: 9, " @@ -910,7 +945,7 @@ TEST_F(LexerTest, Printing) { EXPECT_THAT( GetAndDropLine(print), StrEq("token: { index: 2, kind: 'Semi', line: 11, column: 10, " - "indent: 9, spelling: ';' }")); + "indent: 9, spelling: ';', has_trailing_space: true }")); EXPECT_THAT( GetAndDropLine(print), StrEq("token: { index: 3, kind: 'EndOfFile', line: 11, column: 11, " @@ -958,6 +993,8 @@ TEST_F(LexerTest, PrintingAsYaml) { EXPECT_THAT(&*token_it, IsKeyValueScalars("indent", "2")); ++token_it; EXPECT_THAT(&*token_it, IsKeyValueScalars("spelling", ";")); + ++token_it; + EXPECT_THAT(&*token_it, IsKeyValueScalars("has_trailing_space", "true")); EXPECT_THAT(++token_it, Eq(token_value_node->end())); ++mapping_it; @@ -981,6 +1018,8 @@ TEST_F(LexerTest, PrintingAsYaml) { EXPECT_THAT(&*token_it, IsKeyValueScalars("indent", "1")); ++token_it; EXPECT_THAT(&*token_it, IsKeyValueScalars("spelling", ";")); + ++token_it; + EXPECT_THAT(&*token_it, IsKeyValueScalars("has_trailing_space", "true")); EXPECT_THAT(++token_it, Eq(token_value_node->end())); ++mapping_it; @@ -1004,6 +1043,8 @@ TEST_F(LexerTest, PrintingAsYaml) { EXPECT_THAT(&*token_it, IsKeyValueScalars("indent", "1")); ++token_it; EXPECT_THAT(&*token_it, IsKeyValueScalars("spelling", ";")); + ++token_it; + EXPECT_THAT(&*token_it, IsKeyValueScalars("has_trailing_space", "true")); EXPECT_THAT(++token_it, Eq(token_value_node->end())); ++mapping_it; diff --git a/toolchain/parser/parse_tree_test.cpp b/toolchain/parser/parse_tree_test.cpp index f232969bc777..081c271deda4 100644 --- a/toolchain/parser/parse_tree_test.cpp +++ b/toolchain/parser/parse_tree_test.cpp @@ -530,6 +530,113 @@ TEST_F(ParseTreeTest, Operators) { MatchFileEnd()})); } +TEST_F(ParseTreeTest, OperatorFixity) { + TokenizedBuffer tokens = GetTokenizedBuffer( + "fn F(p: Int*, n: Int) {\n" + " var q: Int* = p;\n" + " var t: Type = Int*;\n" + " t = t**;\n" + " n = n * n;\n" + " n = n*n;\n" + " G(Int*, n * n);\n" + "}"); + ParseTree tree = ParseTree::Parse(tokens, consumer); + EXPECT_FALSE(tree.HasErrors()); + + EXPECT_THAT( + tree, + MatchParseTreeNodes( + {MatchFunctionDeclaration( + MatchDeclaredName("F"), + MatchParameters( + MatchPatternBinding( + MatchDeclaredName("p"), + MatchPostfixOperator(MatchNameReference("Int"), "*")), + MatchParameterListComma(), + MatchPatternBinding(MatchDeclaredName("n"), + MatchNameReference("Int"))), + MatchCodeBlock( + MatchVariableDeclaration( + MatchPatternBinding(MatchDeclaredName("q"), + MatchPostfixOperator( + MatchNameReference("Int"), "*")), + MatchVariableInitializer(MatchNameReference("p")), + MatchDeclarationEnd()), + MatchVariableDeclaration( + MatchPatternBinding(MatchDeclaredName("t"), + MatchNameReference("Type")), + MatchVariableInitializer(MatchPostfixOperator( + MatchNameReference("Int"), "*")), + MatchDeclarationEnd()), + MatchExpressionStatement(MatchInfixOperator( + MatchNameReference("t"), "=", + MatchPostfixOperator( + MatchPostfixOperator(MatchNameReference("t"), "*"), + "*"))), + MatchExpressionStatement(MatchInfixOperator( + MatchNameReference("n"), "=", + MatchInfixOperator(MatchNameReference("n"), "*", + MatchNameReference("n")))), + MatchExpressionStatement(MatchInfixOperator( + MatchNameReference("n"), "=", + MatchInfixOperator(MatchNameReference("n"), "*", + MatchNameReference("n")))), + MatchExpressionStatement(MatchCallExpression( + MatchNameReference("G"), + MatchPostfixOperator(MatchNameReference("Int"), "*"), + MatchCallExpressionComma(), + MatchInfixOperator(MatchNameReference("n"), "*", + MatchNameReference("n")), + MatchCallExpressionEnd())), + MatchCodeBlockEnd())), + MatchFileEnd()})); +} + +TEST_F(ParseTreeTest, OperatorWhitespaceErrors) { + // Test dispositions: Recovered means we issued an error but recovered a + // proper parse tree; Failed means we didn't fully recover from the error. + enum Kind { Valid, Recovered, Failed }; + + struct Testcase { + const char* input; + Kind kind; + } testcases[] = { + {"var v: Type = Int*;", Valid}, + {"var v: Type = Int *;", Recovered}, + {"var v: Type = Int* ;", Valid}, + {"var v: Type = Int * ;", Recovered}, + {"var n: Int = n * n;", Valid}, + {"var n: Int = n*n;", Valid}, + {"var n: Int = (n)*3;", Valid}, + {"var n: Int = 3*(n);", Valid}, + {"var n: Int = n *n;", Recovered}, + // FIXME: We could figure out that this first Failed example is infix + // with one-token lookahead. + {"var n: Int = n* n;", Failed}, + {"var n: Int = n* -n;", Failed}, + {"var n: Int = n* *p;", Failed}, + // FIXME: We try to form (n*)*p and reject due to missing parentheses + // before we notice the missing whitespace around the second `*`. + // It'd be better to (somehow) form n*(*p) and reject due to the missing + // whitespace around the first `*`. + {"var n: Int = n**p;", Failed}, + {"var n: Int = -n;", Valid}, + {"var n: Int = - n;", Recovered}, + {"var n: Int =-n;", Valid}, + {"var n: Int =- n;", Recovered}, + {"var n: Int = F(Int *);", Recovered}, + {"var n: Int = F(Int *, 0);", Recovered}, + }; + + for (auto [input, kind] : testcases) { + TokenizedBuffer tokens = GetTokenizedBuffer(input); + ErrorTrackingDiagnosticConsumer error_tracker(consumer); + ParseTree tree = ParseTree::Parse(tokens, error_tracker); + EXPECT_THAT(tree.HasErrors(), Eq(kind == Failed)) << input; + EXPECT_THAT(error_tracker.SeenError(), Eq(kind != Valid)) << input; + } +} + TEST_F(ParseTreeTest, VariableDeclarations) { TokenizedBuffer tokens = GetTokenizedBuffer( "var v: Int = 0;\n" diff --git a/toolchain/parser/parser_impl.cpp b/toolchain/parser/parser_impl.cpp index b633c5f70e1b..5e6564cc8c0d 100644 --- a/toolchain/parser/parser_impl.cpp +++ b/toolchain/parser/parser_impl.cpp @@ -116,6 +116,54 @@ struct UnexpectedTokenAfterListElement static constexpr llvm::StringLiteral Message = "Expected `,` or `)`."; }; +struct BinaryOperatorRequiresWhitespace + : SimpleDiagnostic { + static constexpr llvm::StringLiteral ShortName = "syntax-error"; + static constexpr const char* Message = + "Whitespace missing {0} binary operator."; + + bool has_leading_space; + bool has_trailing_space; + + auto Format() -> std::string { + const char* where = "around"; + // clang-format off + if (has_leading_space) { + where = "after"; + } else if (has_trailing_space) { + where = "before"; + } + // clang-format on + return llvm::formatv(Message, where); + } +}; + +struct UnaryOperatorHasWhitespace + : SimpleDiagnostic { + static constexpr llvm::StringLiteral ShortName = "syntax-error"; + static constexpr const char* Message = + "Whitespace is not allowed {0} this unary operator."; + + bool prefix; + + auto Format() -> std::string { + return llvm::formatv(Message, prefix ? "after" : "before"); + } +}; + +struct UnaryOperatorRequiresWhitespace + : SimpleDiagnostic { + static constexpr llvm::StringLiteral ShortName = "syntax-error"; + static constexpr const char* Message = + "Whitespace is required {0} this unary operator."; + + bool prefix; + + auto Format() -> std::string { + return llvm::formatv(Message, prefix ? "before" : "after"); + } +}; + struct OperatorRequiresParentheses : SimpleDiagnostic { static constexpr llvm::StringLiteral ShortName = "syntax-error"; @@ -540,6 +588,7 @@ auto ParseTree::Parser::ParseVariableDeclaration() -> Node { auto semi = ConsumeAndAddLeafNodeIf(TokenKind::Semi(), ParseNodeKind::DeclarationEnd()); if (!semi) { + emitter.EmitError(*position); SkipPastLikelyEnd(var_token, [&](TokenizedBuffer::Token semi) { return AddLeafNode(ParseNodeKind::DeclarationEnd(), semi); }); @@ -685,6 +734,114 @@ auto ParseTree::Parser::ParsePostfixExpression() -> llvm::Optional { } } +// Determines whether the given token is considered to be the start of an +// operand according to the rules for infix operator parsing. +static auto IsAssumedStartOfOperand(TokenKind kind) -> bool { + return kind.IsOneOf({TokenKind::OpenParen(), TokenKind::Identifier(), + TokenKind::IntegerLiteral(), TokenKind::RealLiteral(), + TokenKind::StringLiteral()}); +} + +// Determines whether the given token is considered to be the end of an operand +// according to the rules for infix operator parsing. +static auto IsAssumedEndOfOperand(TokenKind kind) -> bool { + return kind.IsOneOf({TokenKind::CloseParen(), TokenKind::CloseCurlyBrace(), + TokenKind::CloseSquareBracket(), TokenKind::Identifier(), + TokenKind::IntegerLiteral(), TokenKind::RealLiteral(), + TokenKind::StringLiteral()}); +} + +// Determines whether the given token could possibly be the start of an operand. +// This is conservatively correct, and will never incorrectly return `false`, +// but can incorrectly return `true`. +static auto IsPossibleStartOfOperand(TokenKind kind) -> bool { + return !kind.IsOneOf({TokenKind::CloseParen(), TokenKind::CloseCurlyBrace(), + TokenKind::CloseSquareBracket(), TokenKind::Comma(), + TokenKind::Semi(), TokenKind::Colon()}); +} + +auto ParseTree::Parser::IsLexicallyValidInfixOperator() -> bool { + assert(!AtEndOfFile() && "Expected an operator token."); + + bool leading_space = tokens.HasLeadingWhitespace(*position); + bool trailing_space = tokens.HasTrailingWhitespace(*position); + + // If there's whitespace on both sides, it's an infix operator. + if (leading_space && trailing_space) { + return true; + } + + // If there's whitespace on exactly one side, it's not an infix operator. + if (leading_space || trailing_space) { + return false; + } + + // Otherwise, for an infix operator, the preceding token must be any close + // bracket, identifier, or literal and the next token must be an open paren, + // identifier, or literal. + if (position == tokens.Tokens().begin() || + !IsAssumedEndOfOperand(tokens.GetKind(*(position - 1))) || + !IsAssumedStartOfOperand(tokens.GetKind(*(position + 1)))) { + return false; + } + + return true; +} + +auto ParseTree::Parser::DiagnoseOperatorFixity(OperatorFixity fixity) -> void { + bool is_valid_as_infix = IsLexicallyValidInfixOperator(); + + if (fixity == OperatorFixity::Infix) { + // Infix operators must satisfy the infix operator rules. + if (!is_valid_as_infix) { + emitter.EmitError( + *position, + {.has_leading_space = tokens.HasLeadingWhitespace(*position), + .has_trailing_space = tokens.HasTrailingWhitespace(*position)}); + } + } else { + bool prefix = fixity == OperatorFixity::Prefix; + + // Whitespace is not permitted between a symbolic pre/postfix operator and + // its operand. + if (NextTokenKind().IsSymbol() && + (prefix ? tokens.HasTrailingWhitespace(*position) + : tokens.HasLeadingWhitespace(*position))) { + emitter.EmitError(*position, + {.prefix = prefix}); + } + // Pre/postfix operators must not satisfy the infix operator rules. + if (is_valid_as_infix) { + emitter.EmitError(*position, + {.prefix = prefix}); + } + } +} + +auto ParseTree::Parser::IsTrailingOperatorInfix() -> bool { + if (AtEndOfFile()) { + return false; + } + + // An operator that follows the infix operator rules is parsed as + // infix, unless the next token means that it can't possibly be. + if (IsLexicallyValidInfixOperator() && + IsPossibleStartOfOperand(tokens.GetKind(*(position + 1)))) { + return true; + } + + // A trailing operator with leading whitespace that's not valid as infix is + // not valid at all. If the next token looks like the start of an operand, + // then parse as infix, otherwise as postfix. Either way we'll produce a + // diagnostic later on. + if (tokens.HasLeadingWhitespace(*position) && + IsAssumedStartOfOperand(tokens.GetKind(*(position + 1)))) { + return true; + } + + return false; +} + auto ParseTree::Parser::ParseOperatorExpression( PrecedenceGroup ambient_precedence) -> llvm::Optional { auto start = GetSubtreeStartPosition(); @@ -703,6 +860,9 @@ auto ParseTree::Parser::ParseOperatorExpression( // The precedence rules don't permit this prefix operator in this // context. Diagnose this, but carry on and parse it anyway. emitter.EmitError(*position); + } else { + // Check that this operator follows the proper whitespace rules. + DiagnoseOperatorFixity(OperatorFixity::Prefix); } auto operator_token = Consume(NextTokenKind()); @@ -713,9 +873,13 @@ auto ParseTree::Parser::ParseOperatorExpression( } // Consume a sequence of infix and postfix operators. - while (auto trailing_operator = - PrecedenceGroup::ForTrailing(NextTokenKind())) { + while (auto trailing_operator = PrecedenceGroup::ForTrailing( + NextTokenKind(), IsTrailingOperatorInfix())) { auto [operator_precedence, is_binary] = *trailing_operator; + + // FIXME: If this operator is ambiguous with either the ambient precedence + // or the LHS precedence, and there's a variant with a different fixity + // that would work, use that one instead for error recovery. if (PrecedenceGroup::GetPriority(ambient_precedence, operator_precedence) != OperatorPriority::RightFirst) { // The precedence rules don't permit this operator in this context. Try @@ -730,6 +894,9 @@ auto ParseTree::Parser::ParseOperatorExpression( // this operator. Either way, parentheses are required. emitter.EmitError(*position); lhs = llvm::None; + } else { + DiagnoseOperatorFixity(is_binary ? OperatorFixity::Infix + : OperatorFixity::Postfix); } auto operator_token = Consume(NextTokenKind()); diff --git a/toolchain/parser/parser_impl.h b/toolchain/parser/parser_impl.h index f91ae4e4e5b0..cb3a676cfe87 100644 --- a/toolchain/parser/parser_impl.h +++ b/toolchain/parser/parser_impl.h @@ -191,6 +191,20 @@ class ParseTree::Parser { // - designators auto ParsePostfixExpression() -> llvm::Optional; + enum class OperatorFixity { Prefix, Infix, Postfix }; + + // Determines whether the current token satisfies the lexical validity rules + // for an infix operator. + auto IsLexicallyValidInfixOperator() -> bool; + + // Diagnoses if the current token is not written properly for the given + // fixity, for example because mandatory whitespace is missing. + auto DiagnoseOperatorFixity(OperatorFixity fixity) -> void; + + // Determines whether the current trailing operator should be treated as + // infix. + auto IsTrailingOperatorInfix() -> bool; + // Parses an expression involving operators, in a context with the given // precedence. auto ParseOperatorExpression(PrecedenceGroup precedence) diff --git a/toolchain/parser/precedence.cpp b/toolchain/parser/precedence.cpp index 5b60063ebf47..5c6c5ebd181a 100644 --- a/toolchain/parser/precedence.cpp +++ b/toolchain/parser/precedence.cpp @@ -24,6 +24,10 @@ enum PrecedenceLevel : int8_t { BitwiseOr, BitwiseXor, BitShift, + // Type formation. + TypePostfix, + // Sentinel representing a type context. + Type, // Logical. LogicalPrefix, Relational, @@ -32,8 +36,6 @@ enum PrecedenceLevel : int8_t { // Assignment. SimpleAssignment, CompoundAssignment, - // Sentinel representing a type context. - Type, // Sentinel representing a context in which any operator can appear. Lowest, }; @@ -46,30 +48,21 @@ struct OperatorPriorityTable { // Start with a list of , // relationships. MarkHigherThan({Highest}, {NumericPrefix, BitwisePrefix, LogicalPrefix, - NumericPostfix}); + NumericPostfix, TypePostfix}); MarkHigherThan({NumericPrefix, NumericPostfix}, {Modulo, Multiplicative, BitShift}); MarkHigherThan({Multiplicative}, {Additive}); MarkHigherThan({BitwisePrefix}, {BitwiseAnd, BitwiseOr, BitwiseXor, BitShift}); + MarkHigherThan({TypePostfix}, {Type}); MarkHigherThan( - {Modulo, Additive, BitwiseAnd, BitwiseOr, BitwiseXor, BitShift}, + {Modulo, Additive, BitwiseAnd, BitwiseOr, BitwiseXor, BitShift, Type}, {SimpleAssignment, CompoundAssignment, Relational}); MarkHigherThan({Relational, LogicalPrefix}, {LogicalAnd, LogicalOr}); MarkHigherThan( {SimpleAssignment, CompoundAssignment, LogicalAnd, LogicalOr}, {Lowest}); - // FIXME: Decide upon a precedence level to use for types. It's important - // that this is no higher than simple assignment, otherwise - // var x: T = y; - // would be parsed as - // var x: (T = y); - // For now, we have no type operators and no operator overloading, so we - // only parse primary expressions in types. - MarkHigherThan({Highest}, {Type}); - MarkHigherThan({Type}, {Lowest}); - // Compute the transitive closure of the above relationships: if we parse // `a $ b @ c` as `(a $ b) @ c` and parse `b @ c % d` as `(b @ c) % d`, // then we will parse `a $ b @ c % d` as `((a $ b) @ c) % d` and should @@ -144,7 +137,7 @@ struct OperatorPriorityTable { } // Postfix operators are symmetric with prefix operators. - for (PrecedenceLevel postfix : {NumericPostfix}) { + for (PrecedenceLevel postfix : {NumericPostfix, TypePostfix}) { table[postfix][postfix] = OperatorPriority::LeftFirst; } @@ -216,7 +209,8 @@ auto PrecedenceGroup::ForLeading(TokenKind kind) } } -auto PrecedenceGroup::ForTrailing(TokenKind kind) -> llvm::Optional { +auto PrecedenceGroup::ForTrailing(TokenKind kind, bool infix) + -> llvm::Optional { switch (kind) { // Assignment operators. case TokenKind::Equal(): @@ -265,12 +259,16 @@ auto PrecedenceGroup::ForTrailing(TokenKind kind) -> llvm::Optional { return Trailing{.level = Additive, .is_binary = true}; // Multiplicative operators. - case TokenKind::Star(): case TokenKind::Slash(): return Trailing{.level = Multiplicative, .is_binary = true}; case TokenKind::Percent(): return Trailing{.level = Modulo, .is_binary = true}; + // `*` could be multiplication or pointer type formation. + case TokenKind::Star(): + return infix ? Trailing{.level = Multiplicative, .is_binary = true} + : Trailing{.level = TypePostfix, .is_binary = false}; + // Postfix operators. case TokenKind::MinusMinus(): case TokenKind::PlusPlus(): diff --git a/toolchain/parser/precedence.h b/toolchain/parser/precedence.h index 71c25b5a4637..7f995b9bda50 100644 --- a/toolchain/parser/precedence.h +++ b/toolchain/parser/precedence.h @@ -57,8 +57,11 @@ class PrecedenceGroup { // Look up the operator information of the given infix or postfix operator // token, or return llvm::None if the given token is not an infix or postfix - // operator. - static auto ForTrailing(TokenKind kind) -> llvm::Optional; + // operator. `infix` indicates whether this is a valid infix operator, but is + // only considered if the same operator symbol is available as both infix and + // postfix. + static auto ForTrailing(TokenKind kind, bool infix) + -> llvm::Optional; friend auto operator==(PrecedenceGroup lhs, PrecedenceGroup rhs) -> bool { return lhs.level == rhs.level; diff --git a/toolchain/parser/precedence_test.cpp b/toolchain/parser/precedence_test.cpp index aee435d497f3..ad1a172ba7f1 100644 --- a/toolchain/parser/precedence_test.cpp +++ b/toolchain/parser/precedence_test.cpp @@ -20,71 +20,111 @@ TEST(PrecedenceTest, OperatorsAreRecognized) { EXPECT_FALSE(PrecedenceGroup::ForLeading(TokenKind::Slash()).hasValue()); EXPECT_FALSE(PrecedenceGroup::ForLeading(TokenKind::Identifier()).hasValue()); - EXPECT_TRUE(PrecedenceGroup::ForTrailing(TokenKind::Minus()).hasValue()); - EXPECT_FALSE(PrecedenceGroup::ForTrailing(TokenKind::Tilde()).hasValue()); - EXPECT_TRUE(PrecedenceGroup::ForTrailing(TokenKind::Slash()).hasValue()); + EXPECT_TRUE( + PrecedenceGroup::ForTrailing(TokenKind::Minus(), false).hasValue()); EXPECT_FALSE( - PrecedenceGroup::ForTrailing(TokenKind::Identifier()).hasValue()); + PrecedenceGroup::ForTrailing(TokenKind::Tilde(), false).hasValue()); + EXPECT_TRUE( + PrecedenceGroup::ForTrailing(TokenKind::Slash(), true).hasValue()); + EXPECT_FALSE( + PrecedenceGroup::ForTrailing(TokenKind::Identifier(), false).hasValue()); - EXPECT_TRUE(PrecedenceGroup::ForTrailing(TokenKind::Minus())->is_binary); + EXPECT_TRUE( + PrecedenceGroup::ForTrailing(TokenKind::Minus(), true)->is_binary); EXPECT_FALSE( - PrecedenceGroup::ForTrailing(TokenKind::MinusMinus())->is_binary); + PrecedenceGroup::ForTrailing(TokenKind::MinusMinus(), false)->is_binary); +} + +TEST(PrecedenceTest, InfixVsPostfix) { + // A trailing `-` is always infix; a trailing `--` is always postfix. + EXPECT_TRUE( + PrecedenceGroup::ForTrailing(TokenKind::Minus(), false)->is_binary); + EXPECT_FALSE( + PrecedenceGroup::ForTrailing(TokenKind::MinusMinus(), true)->is_binary); + + // A trailing `*` is interpreted based on context. + EXPECT_TRUE(PrecedenceGroup::ForTrailing(TokenKind::Star(), true)->is_binary); + EXPECT_FALSE( + PrecedenceGroup::ForTrailing(TokenKind::Star(), false)->is_binary); + + // Infix `*` can appear in type contexts; binary `*` cannot. + EXPECT_THAT(PrecedenceGroup::GetPriority( + PrecedenceGroup::ForTrailing(TokenKind::Star(), true)->level, + PrecedenceGroup::ForType()), + Eq(OperatorPriority::Ambiguous)); + EXPECT_THAT(PrecedenceGroup::GetPriority( + PrecedenceGroup::ForTrailing(TokenKind::Star(), false)->level, + PrecedenceGroup::ForType()), + Eq(OperatorPriority::LeftFirst)); + + // Binary `*` can appear in `+` contexts; binary `*` cannot. + EXPECT_THAT(PrecedenceGroup::GetPriority( + PrecedenceGroup::ForTrailing(TokenKind::Star(), true)->level, + PrecedenceGroup::ForTrailing(TokenKind::Plus(), true)->level), + Eq(OperatorPriority::LeftFirst)); + EXPECT_THAT(PrecedenceGroup::GetPriority( + PrecedenceGroup::ForTrailing(TokenKind::Star(), false)->level, + PrecedenceGroup::ForTrailing(TokenKind::Plus(), true)->level), + Eq(OperatorPriority::Ambiguous)); } TEST(PrecedenceTest, Associativity) { EXPECT_THAT( PrecedenceGroup::ForLeading(TokenKind::Minus())->GetAssociativity(), Eq(Associativity::RightToLeft)); - EXPECT_THAT(PrecedenceGroup::ForTrailing(TokenKind::PlusPlus()) + EXPECT_THAT(PrecedenceGroup::ForTrailing(TokenKind::PlusPlus(), false) ->level.GetAssociativity(), Eq(Associativity::LeftToRight)); - EXPECT_THAT( - PrecedenceGroup::ForTrailing(TokenKind::Plus())->level.GetAssociativity(), - Eq(Associativity::LeftToRight)); - EXPECT_THAT(PrecedenceGroup::ForTrailing(TokenKind::Equal()) + EXPECT_THAT(PrecedenceGroup::ForTrailing(TokenKind::Plus(), true) + ->level.GetAssociativity(), + Eq(Associativity::LeftToRight)); + EXPECT_THAT(PrecedenceGroup::ForTrailing(TokenKind::Equal(), true) ->level.GetAssociativity(), Eq(Associativity::RightToLeft)); - EXPECT_THAT(PrecedenceGroup::ForTrailing(TokenKind::PlusEqual()) + EXPECT_THAT(PrecedenceGroup::ForTrailing(TokenKind::PlusEqual(), true) ->level.GetAssociativity(), Eq(Associativity::None)); } TEST(PrecedenceTest, DirectRelations) { EXPECT_THAT(PrecedenceGroup::GetPriority( - PrecedenceGroup::ForTrailing(TokenKind::Star())->level, - PrecedenceGroup::ForTrailing(TokenKind::Plus())->level), + PrecedenceGroup::ForTrailing(TokenKind::Star(), true)->level, + PrecedenceGroup::ForTrailing(TokenKind::Plus(), true)->level), Eq(OperatorPriority::LeftFirst)); EXPECT_THAT(PrecedenceGroup::GetPriority( - PrecedenceGroup::ForTrailing(TokenKind::Plus())->level, - PrecedenceGroup::ForTrailing(TokenKind::Star())->level), + PrecedenceGroup::ForTrailing(TokenKind::Plus(), true)->level, + PrecedenceGroup::ForTrailing(TokenKind::Star(), true)->level), Eq(OperatorPriority::RightFirst)); EXPECT_THAT(PrecedenceGroup::GetPriority( - PrecedenceGroup::ForTrailing(TokenKind::Amp())->level, - PrecedenceGroup::ForTrailing(TokenKind::Less())->level), + PrecedenceGroup::ForTrailing(TokenKind::Amp(), true)->level, + PrecedenceGroup::ForTrailing(TokenKind::Less(), true)->level), Eq(OperatorPriority::LeftFirst)); EXPECT_THAT(PrecedenceGroup::GetPriority( - PrecedenceGroup::ForTrailing(TokenKind::Less())->level, - PrecedenceGroup::ForTrailing(TokenKind::Amp())->level), + PrecedenceGroup::ForTrailing(TokenKind::Less(), true)->level, + PrecedenceGroup::ForTrailing(TokenKind::Amp(), true)->level), Eq(OperatorPriority::RightFirst)); } TEST(PrecedenceTest, IndirectRelations) { - EXPECT_THAT(PrecedenceGroup::GetPriority( - PrecedenceGroup::ForTrailing(TokenKind::Star())->level, - PrecedenceGroup::ForTrailing(TokenKind::OrKeyword())->level), - Eq(OperatorPriority::LeftFirst)); - EXPECT_THAT(PrecedenceGroup::GetPriority( - PrecedenceGroup::ForTrailing(TokenKind::OrKeyword())->level, - PrecedenceGroup::ForTrailing(TokenKind::Star())->level), - Eq(OperatorPriority::RightFirst)); + EXPECT_THAT( + PrecedenceGroup::GetPriority( + PrecedenceGroup::ForTrailing(TokenKind::Star(), true)->level, + PrecedenceGroup::ForTrailing(TokenKind::OrKeyword(), true)->level), + Eq(OperatorPriority::LeftFirst)); + EXPECT_THAT( + PrecedenceGroup::GetPriority( + PrecedenceGroup::ForTrailing(TokenKind::OrKeyword(), true)->level, + PrecedenceGroup::ForTrailing(TokenKind::Star(), true)->level), + Eq(OperatorPriority::RightFirst)); + EXPECT_THAT( + PrecedenceGroup::GetPriority( + *PrecedenceGroup::ForLeading(TokenKind::Tilde()), + PrecedenceGroup::ForTrailing(TokenKind::Equal(), true)->level), + Eq(OperatorPriority::LeftFirst)); EXPECT_THAT(PrecedenceGroup::GetPriority( - *PrecedenceGroup::ForLeading(TokenKind::Tilde()), - PrecedenceGroup::ForTrailing(TokenKind::Equal())->level), - Eq(OperatorPriority::LeftFirst)); - EXPECT_THAT(PrecedenceGroup::GetPriority( - PrecedenceGroup::ForTrailing(TokenKind::Equal())->level, + PrecedenceGroup::ForTrailing(TokenKind::Equal(), true)->level, *PrecedenceGroup::ForLeading(TokenKind::Tilde())), Eq(OperatorPriority::RightFirst)); } @@ -100,15 +140,16 @@ TEST(PrecedenceTest, IncomparableOperators) { Eq(OperatorPriority::Ambiguous)); EXPECT_THAT(PrecedenceGroup::GetPriority( *PrecedenceGroup::ForLeading(TokenKind::Minus()), - PrecedenceGroup::ForTrailing(TokenKind::Amp())->level), + PrecedenceGroup::ForTrailing(TokenKind::Amp(), true)->level), Eq(OperatorPriority::Ambiguous)); + EXPECT_THAT( + PrecedenceGroup::GetPriority( + PrecedenceGroup::ForTrailing(TokenKind::Equal(), true)->level, + PrecedenceGroup::ForTrailing(TokenKind::PipeEqual(), true)->level), + Eq(OperatorPriority::Ambiguous)); EXPECT_THAT(PrecedenceGroup::GetPriority( - PrecedenceGroup::ForTrailing(TokenKind::Equal())->level, - PrecedenceGroup::ForTrailing(TokenKind::PipeEqual())->level), - Eq(OperatorPriority::Ambiguous)); - EXPECT_THAT(PrecedenceGroup::GetPriority( - PrecedenceGroup::ForTrailing(TokenKind::Plus())->level, - PrecedenceGroup::ForTrailing(TokenKind::Amp())->level), + PrecedenceGroup::ForTrailing(TokenKind::Plus(), true)->level, + PrecedenceGroup::ForTrailing(TokenKind::Amp(), true)->level), Eq(OperatorPriority::Ambiguous)); }