Update precedence rules to match design. (carbon-language#3081)

- Only allow assignment at the top level in an expression statement. - Allow both negation and complement as subexpressions of both bitwise and numeric operators. - Remove parsing support for postincrement and postdecrement. - Add parsing support for `as` operator. - Use the same ambient precedence for types and non-type expressions. --------- Co-authored-by: Chandler Carruth <[email protected]>
zygoloid · Aug 10, 2023 · fc5a954 · fc5a954
1 parent 3cca175
commit fc5a954
Show file tree

Hide file tree

Showing 18 changed files with 634 additions and 99 deletions.
diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def
@@ -63,6 +63,7 @@ CARBON_DIAGNOSTIC_KIND(ExpectedStructLiteralField)
 CARBON_DIAGNOSTIC_KIND(ExpectedVariableDeclaration)
 CARBON_DIAGNOSTIC_KIND(ExpectedVariableName)
 CARBON_DIAGNOSTIC_KIND(OperatorRequiresParentheses)
+CARBON_DIAGNOSTIC_KIND(StatementOperatorAsSubexpression)
 CARBON_DIAGNOSTIC_KIND(UnaryOperatorRequiresParentheses)
 CARBON_DIAGNOSTIC_KIND(UnaryOperatorHasWhitespace)
 CARBON_DIAGNOSTIC_KIND(UnaryOperatorRequiresWhitespace)

diff --git a/toolchain/parser/parser_handle_expression.cpp b/toolchain/parser/parser_handle_expression.cpp
@@ -6,6 +6,15 @@
 
 namespace Carbon {
 
+static auto DiagnoseStatementOperatorAsSubexpression(ParserContext& context)
+    -> void {
+  CARBON_DIAGNOSTIC(StatementOperatorAsSubexpression, Error,
+                    "Operator `{0}` can only be used as a complete statement.",
+                    TokenKind);
+  context.emitter().Emit(*context.position(), StatementOperatorAsSubexpression,
+                         context.PositionKind());
+}
+
 auto ParserHandleExpression(ParserContext& context) -> void {
   auto state = context.PopState();
 
@@ -17,13 +26,20 @@ auto ParserHandleExpression(ParserContext& context) -> void {
         OperatorPriority::RightFirst) {
       // The precedence rules don't permit this prefix operator in this
       // context. Diagnose this, but carry on and parse it anyway.
-      CARBON_DIAGNOSTIC(
-          UnaryOperatorRequiresParentheses, Error,
-          "Parentheses are required around this unary `{0}` operator.",
-          TokenKind);
-      context.emitter().Emit(*context.position(),
-                             UnaryOperatorRequiresParentheses,
-                             context.PositionKind());
+      if (PrecedenceGroup::GetPriority(PrecedenceGroup::ForTopLevelExpression(),
+                                       *operator_precedence) ==
+          OperatorPriority::RightFirst) {
+        CARBON_DIAGNOSTIC(
+            UnaryOperatorRequiresParentheses, Error,
+            "Parentheses are required around this unary `{0}` operator.",
+            TokenKind);
+        context.emitter().Emit(*context.position(),
+                               UnaryOperatorRequiresParentheses,
+                               context.PositionKind());
+      } else {
+        // This operator wouldn't be allowed even if parenthesized.
+        DiagnoseStatementOperatorAsSubexpression(context);
+      }
     } else {
       // Check that this operator follows the proper whitespace rules.
       context.DiagnoseOperatorFixity(ParserContext::OperatorFixity::Prefix);
@@ -188,10 +204,17 @@ auto ParserHandleExpressionLoop(ParserContext& context) -> void {
     // Either the LHS operator and this operator are ambiguous, or the
     // LHS operator is a unary operator that can't be nested within
     // this operator. Either way, parentheses are required.
-    CARBON_DIAGNOSTIC(
-        OperatorRequiresParentheses, Error,
-        "Parentheses are required to disambiguate operator precedence.");
-    context.emitter().Emit(*context.position(), OperatorRequiresParentheses);
+    if (PrecedenceGroup::GetPriority(PrecedenceGroup::ForTopLevelExpression(),
+                                     operator_precedence) ==
+        OperatorPriority::RightFirst) {
+      CARBON_DIAGNOSTIC(
+          OperatorRequiresParentheses, Error,
+          "Parentheses are required to disambiguate operator precedence.");
+      context.emitter().Emit(*context.position(), OperatorRequiresParentheses);
+    } else {
+      // This operator wouldn't be allowed even if parenthesized.
+      DiagnoseStatementOperatorAsSubexpression(context);
+    }
     state.has_error = true;
   } else {
     context.DiagnoseOperatorFixity(

diff --git a/toolchain/parser/parser_handle_statement.cpp b/toolchain/parser/parser_handle_statement.cpp
@@ -46,7 +46,7 @@ auto ParserHandleStatement(ParserContext& context) -> void {
     }
     default: {
       context.PushState(ParserState::ExpressionStatementFinish);
-      context.PushState(ParserState::Expression);
+      context.PushStateForExpression(PrecedenceGroup::ForExpressionStatement());
       break;
     }
   }

diff --git a/toolchain/parser/precedence.cpp b/toolchain/parser/precedence.cpp
@@ -18,7 +18,6 @@ enum PrecedenceLevel : int8_t {
   TermPrefix,
   // Numeric.
   NumericPrefix,
-  NumericPostfix,
   Modulo,
   Multiplicative,
   Additive,
@@ -31,8 +30,8 @@ enum PrecedenceLevel : int8_t {
   // Type formation.
   TypePrefix,
   TypePostfix,
-  // Sentinel representing a type context.
-  Type,
+  // Casts.
+  As,
   // Logical.
   LogicalPrefix,
   Relational,
@@ -41,8 +40,7 @@ enum PrecedenceLevel : int8_t {
   // Conditional.
   If,
   // Assignment.
-  SimpleAssignment,
-  CompoundAssignment,
+  Assignment,
   // Sentinel representing a context in which any operator can appear.
   Lowest,
 };
@@ -54,27 +52,24 @@ struct OperatorPriorityTable {
   constexpr OperatorPriorityTable() : table() {
     // Start with a list of <higher precedence>, <lower precedence>
     // relationships.
-    MarkHigherThan({Highest}, {TermPrefix});
-    MarkHigherThan({TermPrefix}, {NumericPrefix, BitwisePrefix, LogicalPrefix,
-                                  NumericPostfix});
-    MarkHigherThan({NumericPrefix, NumericPostfix},
-                   {Modulo, Multiplicative, BitShift});
+    MarkHigherThan({Highest}, {TermPrefix, LogicalPrefix});
+    MarkHigherThan({TermPrefix}, {NumericPrefix, BitwisePrefix});
+    MarkHigherThan({NumericPrefix, BitwisePrefix},
+                   {As, Multiplicative, Modulo, BitwiseAnd, BitwiseOr,
+                    BitwiseXor, BitShift});
     MarkHigherThan({Multiplicative}, {Additive});
-    MarkHigherThan({BitwisePrefix},
-                   {BitwiseAnd, BitwiseOr, BitwiseXor, BitShift});
     MarkHigherThan(
-        {Modulo, Additive, BitwiseAnd, BitwiseOr, BitwiseXor, BitShift},
+        {As, Additive, Modulo, BitwiseAnd, BitwiseOr, BitwiseXor, BitShift},
         {Relational});
     MarkHigherThan({Relational, LogicalPrefix}, {LogicalAnd, LogicalOr});
     MarkHigherThan({LogicalAnd, LogicalOr}, {If});
-    MarkHigherThan({If}, {SimpleAssignment, CompoundAssignment});
-    MarkHigherThan({SimpleAssignment, CompoundAssignment}, {Lowest});
+    MarkHigherThan({If}, {Assignment});
+    MarkHigherThan({Assignment}, {Lowest});
 
     // Types are mostly a separate precedence graph.
     MarkHigherThan({Highest}, {TypePrefix});
     MarkHigherThan({TypePrefix}, {TypePostfix});
-    MarkHigherThan({TypePostfix}, {Type});
-    MarkHigherThan({Type}, {If});
+    MarkHigherThan({TypePostfix}, {As});
 
     // 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`,
@@ -142,17 +137,13 @@ struct OperatorPriorityTable {
     // Associativity rules occupy the diagonal
 
     // For prefix operators, RightFirst would mean `@@x` is `@(@x)` and
-    // Ambiguous would mean it's an error. LeftFirst is meaningless. For now we
-    // allow all prefix operators other than `const` to be repeated.
-    //
-    // TODO: The design does not permit repeating most unary operators.
-    for (PrecedenceLevel prefix :
-         {TermPrefix, NumericPrefix, BitwisePrefix, LogicalPrefix, If}) {
+    // Ambiguous would mean it's an error. LeftFirst is meaningless.
+    for (PrecedenceLevel prefix : {TermPrefix, If}) {
       table[prefix][prefix] = OperatorPriority::RightFirst;
     }
 
     // Postfix operators are symmetric with prefix operators.
-    for (PrecedenceLevel postfix : {NumericPostfix, TypePostfix}) {
+    for (PrecedenceLevel postfix : {TypePostfix}) {
       table[postfix][postfix] = OperatorPriority::LeftFirst;
     }
 
@@ -164,12 +155,7 @@ struct OperatorPriorityTable {
       table[assoc][assoc] = OperatorPriority::LeftFirst;
     }
 
-    // Assignment is given right-to-left associativity in order to support
-    // chained assignment.
-    table[SimpleAssignment][SimpleAssignment] = OperatorPriority::RightFirst;
-
-    // For other operators, there isn't an obvious answer and we require
-    // explicit parentheses.
+    // For other operators, we require explicit parentheses.
   }
 
   constexpr void ConsistencyCheck() {
@@ -196,11 +182,15 @@ auto PrecedenceGroup::ForPostfixExpression() -> PrecedenceGroup {
 }
 
 auto PrecedenceGroup::ForTopLevelExpression() -> PrecedenceGroup {
+  return PrecedenceGroup(If);
+}
+
+auto PrecedenceGroup::ForExpressionStatement() -> PrecedenceGroup {
   return PrecedenceGroup(Lowest);
 }
 
 auto PrecedenceGroup::ForType() -> PrecedenceGroup {
-  return PrecedenceGroup(Type);
+  return ForTopLevelExpression();
 }
 
 auto PrecedenceGroup::ForLeading(TokenKind kind)
@@ -214,9 +204,11 @@ auto PrecedenceGroup::ForLeading(TokenKind kind)
       return PrecedenceGroup(LogicalPrefix);
 
     case TokenKind::Minus:
+      return PrecedenceGroup(NumericPrefix);
+
     case TokenKind::MinusMinus:
     case TokenKind::PlusPlus:
-      return PrecedenceGroup(NumericPrefix);
+      return PrecedenceGroup(Assignment);
 
     case TokenKind::Caret:
       return PrecedenceGroup(BitwisePrefix);
@@ -237,7 +229,6 @@ auto PrecedenceGroup::ForTrailing(TokenKind kind, bool infix)
   switch (kind) {
     // Assignment operators.
     case TokenKind::Equal:
-      return Trailing{.level = SimpleAssignment, .is_binary = true};
     case TokenKind::PlusEqual:
     case TokenKind::MinusEqual:
     case TokenKind::StarEqual:
@@ -248,7 +239,7 @@ auto PrecedenceGroup::ForTrailing(TokenKind kind, bool infix)
     case TokenKind::CaretEqual:
     case TokenKind::GreaterGreaterEqual:
     case TokenKind::LessLessEqual:
-      return Trailing{.level = CompoundAssignment, .is_binary = true};
+      return Trailing{.level = Assignment, .is_binary = true};
 
     // Logical operators.
     case TokenKind::And:
@@ -293,14 +284,15 @@ auto PrecedenceGroup::ForTrailing(TokenKind kind, bool infix)
       return infix ? Trailing{.level = Multiplicative, .is_binary = true}
                    : Trailing{.level = TypePostfix, .is_binary = false};
 
-    // Postfix operators.
-    case TokenKind::MinusMinus:
-    case TokenKind::PlusPlus:
-      return Trailing{.level = NumericPostfix, .is_binary = false};
+    // Cast operator.
+    case TokenKind::As:
+      return Trailing{.level = As, .is_binary = true};
 
     // Prefix-only operators.
-    case TokenKind::Not:
     case TokenKind::Const:
+    case TokenKind::MinusMinus:
+    case TokenKind::Not:
+    case TokenKind::PlusPlus:
       break;
 
     // Symbolic tokens that might be operators eventually.

diff --git a/toolchain/parser/precedence.h b/toolchain/parser/precedence.h
@@ -38,15 +38,20 @@ class PrecedenceGroup {
   PrecedenceGroup() = delete;
 
   // Get the sentinel precedence level for a postfix expression. All operators
-  // should have lower precedence than this.
+  // have lower precedence than this.
   static auto ForPostfixExpression() -> PrecedenceGroup;
 
-  // Get the sentinel precedence level for a top-level expression context. All
-  // operators should have higher precedence than this.
+  // Get the precedence level for a top-level or parenthesized expression. All
+  // expression operators have higher precedence than this.
   static auto ForTopLevelExpression() -> PrecedenceGroup;
 
+  // Get the sentinel precedence level for a statement context. All operators,
+  // including statement operators like `=` and `++`, have higher precedence
+  // than this.
+  static auto ForExpressionStatement() -> PrecedenceGroup;
+
   // Get the precedence level at which to parse a type expression. All type
-  // operators should have higher precedence than this.
+  // operators have higher precedence than this.
   static auto ForType() -> PrecedenceGroup;
 
   // Look up the operator information of the given prefix operator token, or

diff --git a/toolchain/parser/precedence_test.cpp b/toolchain/parser/precedence_test.cpp
@@ -33,29 +33,19 @@ TEST(PrecedenceTest, OperatorsAreRecognized) {
 
   EXPECT_TRUE(PrecedenceGroup::ForTrailing(TokenKind::Minus, true)->is_binary);
   EXPECT_FALSE(
-      PrecedenceGroup::ForTrailing(TokenKind::MinusMinus, false)->is_binary);
+      PrecedenceGroup::ForTrailing(TokenKind::MinusMinus, false).has_value());
 }
 
 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 always binary, even when written with whitespace that
+  // suggests it's postfix.
+  EXPECT_TRUE(PrecedenceGroup::ForTrailing(TokenKind::Minus, /*infix*/ false)
+                  ->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);
 
-  // Postfix `*` can appear in type contexts; infix `*` 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));
-
   // Infix `*` can appear in `+` contexts; postfix `*` cannot.
   EXPECT_THAT(PrecedenceGroup::GetPriority(
                   PrecedenceGroup::ForTrailing(TokenKind::Star, true)->level,
@@ -69,18 +59,14 @@ TEST(PrecedenceTest, InfixVsPostfix) {
 
 TEST(PrecedenceTest, Associativity) {
   EXPECT_THAT(PrecedenceGroup::ForLeading(TokenKind::Minus)->GetAssociativity(),
+              Eq(Associativity::None));
+  EXPECT_THAT(PrecedenceGroup::ForLeading(TokenKind::Star)->GetAssociativity(),
               Eq(Associativity::RightToLeft));
-  EXPECT_THAT(PrecedenceGroup::ForTrailing(TokenKind::PlusPlus, false)
-                  ->level.GetAssociativity(),
-              Eq(Associativity::LeftToRight));
   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, true)
-                  ->level.GetAssociativity(),
               Eq(Associativity::None));
 }
 
@@ -134,7 +120,7 @@ TEST(PrecedenceTest, IncomparableOperators) {
                   *PrecedenceGroup::ForLeading(TokenKind::Minus)),
               Eq(OperatorPriority::Ambiguous));
   EXPECT_THAT(PrecedenceGroup::GetPriority(
-                  *PrecedenceGroup::ForLeading(TokenKind::Minus),
+                  *PrecedenceGroup::ForLeading(TokenKind::Not),
                   PrecedenceGroup::ForTrailing(TokenKind::Amp, true)->level),
               Eq(OperatorPriority::Ambiguous));
   EXPECT_THAT(

diff --git a/toolchain/parser/testdata/if_expression/in_type.carbon b/toolchain/parser/testdata/if_expression/in_type.carbon
@@ -0,0 +1,43 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// AUTOUPDATE
+
+var n: i32;
+
+fn F() -> if true then i32 else i32* {
+  return if true then n else &n;
+}
+
+// CHECK:STDOUT: [
+// CHECK:STDOUT:   {kind: 'VariableIntroducer', text: 'var'},
+// CHECK:STDOUT:     {kind: 'Name', text: 'n'},
+// CHECK:STDOUT:     {kind: 'Literal', text: 'i32'},
+// CHECK:STDOUT:   {kind: 'PatternBinding', text: ':', subtree_size: 3},
+// CHECK:STDOUT: {kind: 'VariableDeclaration', text: ';', subtree_size: 5},
+// CHECK:STDOUT:     {kind: 'FunctionIntroducer', text: 'fn'},
+// CHECK:STDOUT:     {kind: 'Name', text: 'F'},
+// CHECK:STDOUT:       {kind: 'ParameterListStart', text: '('},
+// CHECK:STDOUT:     {kind: 'ParameterList', text: ')', subtree_size: 2},
+// CHECK:STDOUT:           {kind: 'Literal', text: 'true'},
+// CHECK:STDOUT:         {kind: 'IfExpressionIf', text: 'if', subtree_size: 2},
+// CHECK:STDOUT:           {kind: 'Literal', text: 'i32'},
+// CHECK:STDOUT:         {kind: 'IfExpressionThen', text: 'then', subtree_size: 2},
+// CHECK:STDOUT:           {kind: 'Literal', text: 'i32'},
+// CHECK:STDOUT:         {kind: 'PostfixOperator', text: '*', subtree_size: 2},
+// CHECK:STDOUT:       {kind: 'IfExpressionElse', text: 'else', subtree_size: 7},
+// CHECK:STDOUT:     {kind: 'ReturnType', text: '->', subtree_size: 8},
+// CHECK:STDOUT:   {kind: 'FunctionDefinitionStart', text: '{', subtree_size: 13},
+// CHECK:STDOUT:     {kind: 'ReturnStatementStart', text: 'return'},
+// CHECK:STDOUT:         {kind: 'Literal', text: 'true'},
+// CHECK:STDOUT:       {kind: 'IfExpressionIf', text: 'if', subtree_size: 2},
+// CHECK:STDOUT:         {kind: 'NameExpression', text: 'n'},
+// CHECK:STDOUT:       {kind: 'IfExpressionThen', text: 'then', subtree_size: 2},
+// CHECK:STDOUT:         {kind: 'NameExpression', text: 'n'},
+// CHECK:STDOUT:       {kind: 'PrefixOperator', text: '&', subtree_size: 2},
+// CHECK:STDOUT:     {kind: 'IfExpressionElse', text: 'else', subtree_size: 7},
+// CHECK:STDOUT:   {kind: 'ReturnStatement', text: ';', subtree_size: 9},
+// CHECK:STDOUT: {kind: 'FunctionDefinition', text: '}', subtree_size: 23},
+// CHECK:STDOUT: {kind: 'FileEnd', text: ''},
+// CHECK:STDOUT: ]