Handle identifier value category properly. (#986)

executable_semantics/ast/BUILD

@@ -80,6 +80,7 @@ cc_library(
         ":source_location",
         ":statement",
         ":static_scope",
+        ":value_category",
         "//common:ostream",
         "//executable_semantics/common:nonnull",
         "@llvm-project//llvm:Support",
@@ -94,6 +95,7 @@ cc_library(
         ":ast_node",
         ":paren_contents",
         ":static_scope",
+        ":value_category",
         "//common:indirect_value",
         "//common:ostream",
         "//executable_semantics/common:arena",
@@ -146,6 +148,7 @@ cc_library(
         ":expression",
         ":source_location",
         ":static_scope",
+        ":value_category",
         "//common:ostream",
         "//executable_semantics/common:arena",
         "//executable_semantics/common:error",
@@ -171,6 +174,7 @@ cc_library(
     deps = [
         ":ast_node",
         ":source_location",
+        ":value_category",
         "//executable_semantics/common:arena",
         "//executable_semantics/common:error",
     ],
@@ -194,9 +198,15 @@ cc_library(
         ":expression",
         ":pattern",
         ":source_location",
+        ":value_category",
         "//common:check",
         "//common:ostream",
         "//executable_semantics/common:arena",
         "@llvm-project//llvm:Support",
     ],
 )
+
+cc_library(
+    name = "value_category",
+    hdrs = ["value_category.h"],
+)

executable_semantics/ast/declaration.h

@@ -15,6 +15,7 @@
 #include "executable_semantics/ast/source_location.h"
 #include "executable_semantics/ast/statement.h"
 #include "executable_semantics/ast/static_scope.h"
+#include "executable_semantics/ast/value_category.h"
 #include "executable_semantics/common/nonnull.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Compiler.h"
@@ -106,6 +107,8 @@ class GenericBinding : public AstNode {
   // and after typechecking it's guaranteed to be true.
   auto has_static_type() const -> bool { return static_type_.has_value(); }
 
+  auto value_category() const -> ValueCategory { return ValueCategory::Let; }
+
  private:
   std::string name_;
   Nonnull<Expression*> type_;
@@ -228,6 +231,8 @@ class FunctionDeclaration : public Declaration {
   auto body() const -> std::optional<Nonnull<const Block*>> { return body_; }
   auto body() -> std::optional<Nonnull<Block*>> { return body_; }
 
+  auto value_category() const -> ValueCategory { return ValueCategory::Let; }
+
  private:
   std::string name_;
   std::vector<Nonnull<GenericBinding*>> deduced_parameters_;
@@ -253,6 +258,8 @@ class ClassDeclaration : public Declaration {
   auto name() const -> const std::string& { return name_; }
   auto members() const -> llvm::ArrayRef<Nonnull<Member*>> { return members_; }
 
+  auto value_category() const -> ValueCategory { return ValueCategory::Let; }
+
  private:
   std::string name_;
   std::vector<Nonnull<Member*>> members_;
@@ -304,6 +311,8 @@ class ChoiceDeclaration : public Declaration {
     return alternatives_;
   }
 
+  auto value_category() const -> ValueCategory { return ValueCategory::Let; }
+
  private:
   std::string name_;
   std::vector<Nonnull<AlternativeSignature*>> alternatives_;

executable_semantics/ast/expression.h

@@ -15,6 +15,7 @@
 #include "executable_semantics/ast/paren_contents.h"
 #include "executable_semantics/ast/source_location.h"
 #include "executable_semantics/ast/static_scope.h"
+#include "executable_semantics/ast/value_category.h"
 #include "executable_semantics/common/arena.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Compiler.h"
@@ -25,16 +26,6 @@ class Value;
 
 class Expression : public AstNode {
  public:
-  // The value category of a Carbon expression indicates whether it evaluates
-  // to a variable or a value. A variable can be mutated, and can have its
-  // address taken, whereas a value cannot.
-  enum class ValueCategory {
-    // A variable. This roughly corresponds to a C/C++ lvalue.
-    Var,
-    // A value. This roughly corresponds to a C/C++ rvalue.
-    Let,
-  };
-
   ~Expression() override = 0;
 
   void Print(llvm::raw_ostream& out) const override;

executable_semantics/ast/pattern.h

@@ -118,6 +118,8 @@ class BindingPattern : public Pattern {
   auto type() const -> const Pattern& { return *type_; }
   auto type() -> Pattern& { return *type_; }
 
+  auto value_category() const -> ValueCategory { return ValueCategory::Var; }
+
  private:
   std::optional<std::string> name_;
   Nonnull<Pattern*> type_;

executable_semantics/ast/statement.h

@@ -13,6 +13,7 @@
 #include "executable_semantics/ast/pattern.h"
 #include "executable_semantics/ast/source_location.h"
 #include "executable_semantics/ast/static_scope.h"
+#include "executable_semantics/ast/value_category.h"
 #include "executable_semantics/common/arena.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Compiler.h"
@@ -344,6 +345,8 @@ class Continuation : public Statement {
   // and after typechecking it's guaranteed to be true.
   auto has_static_type() const -> bool { return static_type_.has_value(); }
 
+  auto value_category() const -> ValueCategory { return ValueCategory::Var; }
+
  private:
   std::string continuation_variable_;
   Nonnull<Block*> body_;

executable_semantics/ast/static_scope.h

@@ -13,6 +13,7 @@
 
 #include "executable_semantics/ast/ast_node.h"
 #include "executable_semantics/ast/source_location.h"
+#include "executable_semantics/ast/value_category.h"
 #include "executable_semantics/common/nonnull.h"
 
 namespace Carbon {
@@ -20,12 +21,19 @@ namespace Carbon {
 class Value;
 
 // True if NodeType::ImplementsCarbonNamedEntity is valid and names a type,
-// indicating that NodeType implements the NamedEntity interface. This imposes
-// the following requirements on NodeType, where `node` is a const instance of
-// NodeType:
+// indicating that NodeType implements the NamedEntity interface, which means
+// it must define the following methods, with contracts as documented.
+#if 0
+  // Returns the static type of an IdentifierExpression that names *this.
+  auto static_type() const -> const Value&;
+
+  // Returns the value category of an IdentifierExpression that names *this.
+  auto value_category() const -> ValueCategory;
+#endif
+// NodeType must be derived from AstNode.
 //
-// - NodeType is derived from AstNode.
-// - node.static_type() is well-formed and has type const Value&.
+// TODO: consider turning the above documentation into real code, as sketched
+// at https://godbolt.org/z/186oEozhc
 template <typename T, typename = void>
 static constexpr bool ImplementsNamedEntity = false;
 
@@ -43,9 +51,13 @@ class NamedEntityView {
   NamedEntityView(Nonnull<const NodeType*> node)
       // Type-erase NodeType, retaining a pointer to the base class AstNode
       // and using std::function to encapsulate the ability to call
-      // the derived class's static_type.
-      : base_(node), static_type_([](const AstNode& base) -> const Value& {
+      // the derived class's methods.
+      : base_(node),
+        static_type_([](const AstNode& base) -> const Value& {
           return llvm::cast<NodeType>(base).static_type();
+        }),
+        value_category_([](const AstNode& base) -> ValueCategory {
+          return llvm::cast<NodeType>(base).value_category();
         }) {}
 
   NamedEntityView(const NamedEntityView&) = default;
@@ -59,6 +71,11 @@ class NamedEntityView {
   // Returns node->static_type()
   auto static_type() const -> const Value& { return static_type_(*base_); }
 
+  // Returns node->value_category()
+  auto value_category() const -> ValueCategory {
+    return value_category_(*base_);
+  }
+
   friend auto operator==(const NamedEntityView& lhs,
                          const NamedEntityView& rhs) {
     return lhs.base_ == rhs.base_;
@@ -72,6 +89,7 @@ class NamedEntityView {
  private:
   Nonnull<const AstNode*> base_;
   std::function<const Value&(const AstNode&)> static_type_;
+  std::function<ValueCategory(const AstNode&)> value_category_;
 };
 
 // Maps the names visible in a given scope to the entities they name.

executable_semantics/ast/value_category.h

@@ -0,0 +1,22 @@
+// 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
+
+#ifndef EXECUTABLE_SEMANTICS_AST_VALUE_CATEGORY_H_
+#define EXECUTABLE_SEMANTICS_AST_VALUE_CATEGORY_H_
+
+namespace Carbon {
+
+// The value category of a Carbon expression indicates whether it evaluates
+// to a variable or a value. A variable can be mutated, and can have its
+// address taken, whereas a value cannot.
+enum class ValueCategory {
+  // A variable. This roughly corresponds to a C/C++ lvalue.
+  Var,
+  // A value. This roughly corresponds to a C/C++ rvalue.
+  Let,
+};
+
+}  // namespace Carbon
+
+#endif  // EXECUTABLE_SEMANTICS_AST_VALUE_CATEGORY_H_

executable_semantics/interpreter/type_checker.cpp

@@ -426,7 +426,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
         arg_types.push_back(&arg->static_type());
       }
       SetStaticType(e, arena_->New<TupleValue>(std::move(arg_types)));
-      e->set_value_category(Expression::ValueCategory::Let);
+      e->set_value_category(ValueCategory::Let);
       return;
     }
     case ExpressionKind::StructLiteral: {
@@ -436,7 +436,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
         arg_types.push_back({arg.name(), &arg.expression().static_type()});
       }
       SetStaticType(e, arena_->New<StructType>(std::move(arg_types)));
-      e->set_value_category(Expression::ValueCategory::Let);
+      e->set_value_category(ValueCategory::Let);
       return;
     }
     case ExpressionKind::StructTypeLiteral: {
@@ -455,7 +455,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
       } else {
         SetStaticType(&struct_type, arena_->New<TypeType>());
       }
-      e->set_value_category(Expression::ValueCategory::Let);
+      e->set_value_category(ValueCategory::Let);
       return;
     }
     case ExpressionKind::FieldAccessExpression: {
@@ -490,7 +490,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
           for (auto& method : t_class.methods()) {
             if (access.field() == method.name) {
               SetStaticType(&access, method.value);
-              access.set_value_category(Expression::ValueCategory::Let);
+              access.set_value_category(ValueCategory::Let);
               return;
             }
           }
@@ -512,7 +512,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
                         arena_->New<FunctionType>(
                             std::vector<Nonnull<const GenericBinding*>>(),
                             *parameter_types, &aggregate_type));
-          access.set_value_category(Expression::ValueCategory::Let);
+          access.set_value_category(ValueCategory::Let);
           return;
         }
         default:
@@ -534,17 +534,15 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
         }
       }
       SetStaticType(&ident, &ident.named_entity().static_type());
-      // TODO: this should depend on what entity this name resolves to, but
-      //   we don't have access to that information yet.
-      ident.set_value_category(Expression::ValueCategory::Var);
+      ident.set_value_category(ident.named_entity().value_category());
       return;
     }
     case ExpressionKind::IntLiteral:
-      e->set_value_category(Expression::ValueCategory::Let);
+      e->set_value_category(ValueCategory::Let);
       SetStaticType(e, arena_->New<IntType>());
       return;
     case ExpressionKind::BoolLiteral:
-      e->set_value_category(Expression::ValueCategory::Let);
+      e->set_value_category(ValueCategory::Let);
       SetStaticType(e, arena_->New<BoolType>());
       return;
     case ExpressionKind::PrimitiveOperatorExpression: {
@@ -559,7 +557,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
           ExpectExactType(e->source_loc(), "negation", arena_->New<IntType>(),
                           ts[0]);
           SetStaticType(&op, arena_->New<IntType>());
-          op.set_value_category(Expression::ValueCategory::Let);
+          op.set_value_category(ValueCategory::Let);
           return;
         case Operator::Add:
           ExpectExactType(e->source_loc(), "addition(1)",
@@ -567,7 +565,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
           ExpectExactType(e->source_loc(), "addition(2)",
                           arena_->New<IntType>(), ts[1]);
           SetStaticType(&op, arena_->New<IntType>());
-          op.set_value_category(Expression::ValueCategory::Let);
+          op.set_value_category(ValueCategory::Let);
           return;
         case Operator::Sub:
           ExpectExactType(e->source_loc(), "subtraction(1)",
@@ -575,7 +573,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
           ExpectExactType(e->source_loc(), "subtraction(2)",
                           arena_->New<IntType>(), ts[1]);
           SetStaticType(&op, arena_->New<IntType>());
-          op.set_value_category(Expression::ValueCategory::Let);
+          op.set_value_category(ValueCategory::Let);
           return;
         case Operator::Mul:
           ExpectExactType(e->source_loc(), "multiplication(1)",
@@ -583,7 +581,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
           ExpectExactType(e->source_loc(), "multiplication(2)",
                           arena_->New<IntType>(), ts[1]);
           SetStaticType(&op, arena_->New<IntType>());
-          op.set_value_category(Expression::ValueCategory::Let);
+          op.set_value_category(ValueCategory::Let);
           return;
         case Operator::And:
           ExpectExactType(e->source_loc(), "&&(1)", arena_->New<BoolType>(),
@@ -591,7 +589,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
           ExpectExactType(e->source_loc(), "&&(2)", arena_->New<BoolType>(),
                           ts[1]);
           SetStaticType(&op, arena_->New<BoolType>());
-          op.set_value_category(Expression::ValueCategory::Let);
+          op.set_value_category(ValueCategory::Let);
           return;
         case Operator::Or:
           ExpectExactType(e->source_loc(), "||(1)", arena_->New<BoolType>(),
@@ -599,27 +597,27 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
           ExpectExactType(e->source_loc(), "||(2)", arena_->New<BoolType>(),
                           ts[1]);
           SetStaticType(&op, arena_->New<BoolType>());
-          op.set_value_category(Expression::ValueCategory::Let);
+          op.set_value_category(ValueCategory::Let);
           return;
         case Operator::Not:
           ExpectExactType(e->source_loc(), "!", arena_->New<BoolType>(), ts[0]);
           SetStaticType(&op, arena_->New<BoolType>());
-          op.set_value_category(Expression::ValueCategory::Let);
+          op.set_value_category(ValueCategory::Let);
           return;
         case Operator::Eq:
           ExpectExactType(e->source_loc(), "==", ts[0], ts[1]);
           SetStaticType(&op, arena_->New<BoolType>());
-          op.set_value_category(Expression::ValueCategory::Let);
+          op.set_value_category(ValueCategory::Let);
           return;
         case Operator::Deref:
           ExpectPointerType(e->source_loc(), "*", ts[0]);
           SetStaticType(&op, &cast<PointerType>(*ts[0]).type());
-          op.set_value_category(Expression::ValueCategory::Var);
+          op.set_value_category(ValueCategory::Var);
           return;
         case Operator::Ptr:
           ExpectExactType(e->source_loc(), "*", arena_->New<TypeType>(), ts[0]);
           SetStaticType(&op, arena_->New<TypeType>());
-          op.set_value_category(Expression::ValueCategory::Let);
+          op.set_value_category(ValueCategory::Let);
           return;
       }
       break;
@@ -656,7 +654,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
                        &call.argument().static_type());
           }
           SetStaticType(&call, return_type);
-          call.set_value_category(Expression::ValueCategory::Let);
+          call.set_value_category(ValueCategory::Let);
           return;
         }
         default: {
@@ -674,12 +672,12 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
       ExpectIsConcreteType(fn.return_type().source_loc(),
                            interpreter_.InterpExp(values, &fn.return_type()));
       SetStaticType(&fn, arena_->New<TypeType>());
-      fn.set_value_category(Expression::ValueCategory::Let);
+      fn.set_value_category(ValueCategory::Let);
       return;
     }
     case ExpressionKind::StringLiteral:
       SetStaticType(e, arena_->New<StringType>());
-      e->set_value_category(Expression::ValueCategory::Let);
+      e->set_value_category(ValueCategory::Let);
       return;
     case ExpressionKind::IntrinsicExpression: {
       auto& intrinsic_exp = cast<IntrinsicExpression>(*e);
@@ -694,7 +692,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
                      arena_->New<StringType>(),
                      &intrinsic_exp.args().fields()[0]->static_type());
           SetStaticType(e, TupleValue::Empty());
-          e->set_value_category(Expression::ValueCategory::Let);
+          e->set_value_category(ValueCategory::Let);
           return;
       }
     }
@@ -703,7 +701,7 @@ void TypeChecker::TypeCheckExp(Nonnull<Expression*> e, Env values) {
     case ExpressionKind::StringTypeLiteral:
     case ExpressionKind::TypeTypeLiteral:
     case ExpressionKind::ContinuationTypeLiteral:
-      e->set_value_category(Expression::ValueCategory::Let);
+      e->set_value_category(ValueCategory::Let);
       SetStaticType(e, arena_->New<TypeType>());
       return;
     case ExpressionKind::UnimplementedExpression:
@@ -861,7 +859,7 @@ void TypeChecker::TypeCheckStmt(Nonnull<Statement*> s, Env values) {
       TypeCheckExp(&assign.lhs(), values);
       ExpectType(s->source_loc(), "assign", &assign.lhs().static_type(),
                  &assign.rhs().static_type());
-      if (assign.lhs().value_category() != Expression::ValueCategory::Var) {
+      if (assign.lhs().value_category() != ValueCategory::Var) {
         FATAL_COMPILATION_ERROR(assign.source_loc())
             << "Cannot assign to rvalue '" << assign.lhs() << "'";
       }

executable_semantics/testdata/basic_syntax/fail_assign_to_function.carbon

@@ -2,12 +2,12 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
-// RUN: %{executable_semantics} %s 2>&1 | \
+// RUN: %{not} %{executable_semantics} %s 2>&1 | \
 // RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
-// RUN: %{executable_semantics} --trace %s 2>&1 | \
+// RUN: %{not} %{executable_semantics} --trace %s 2>&1 | \
 // RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
 // AUTOUPDATE: %{executable_semantics} %s
-// CHECK: result: 0
+// CHECK: COMPILATION ERROR: {{.*}}/executable_semantics/testdata/basic_syntax/fail_assign_to_function.carbon:18: Cannot assign to rvalue 'F'
 
 package ExecutableSemanticsTest api;
 
@@ -15,8 +15,6 @@ fn F() {}
 fn G() {}
 
 fn Main() -> i32 {
-  // TODO: this test should not pass. See comments on the handling of
-  // IdentifierExpression in type_checker.cpp.
   F = G;
   return 0;
 }