Add name accessor to NamedEntityView (#994)

This required changing BindingPattern::name() to return `"_"` instead of nullopt when representing a `"_"` binding.

Semi-related drive-by fixes:
- Update BindingPlaceholderValue to expose a NamedEntity instead of a string name, and stop exposing its type.
- Drop the unused SourceLocation parameter of ValueEqual

executable_semantics/ast/member.h

@@ -50,7 +50,9 @@ class Member : public AstNode {
 class FieldMember : public Member {
  public:
   FieldMember(SourceLocation source_loc, Nonnull<BindingPattern*> binding)
-      : Member(AstNodeKind::FieldMember, source_loc), binding_(binding) {}
+      : Member(AstNodeKind::FieldMember, source_loc), binding_(binding) {
+    CHECK(binding->name() != AnonymousName);
+  }
 
   static auto classof(const AstNode* node) -> bool {
     return InheritsFromFieldMember(node->kind());
@@ -60,9 +62,6 @@ class FieldMember : public Member {
   auto binding() -> BindingPattern& { return *binding_; }
 
  private:
-  // TODO: split this into a non-optional name and a type, initialized by
-  // a constructor that takes a BindingPattern and handles errors like a
-  // missing name.
   Nonnull<BindingPattern*> binding_;
 };
 

executable_semantics/ast/pattern.cpp

@@ -26,12 +26,7 @@ void Pattern::Print(llvm::raw_ostream& out) const {
       break;
     case PatternKind::BindingPattern: {
       const auto& binding = cast<BindingPattern>(*this);
-      if (binding.name().has_value()) {
-        out << *binding.name();
-      } else {
-        out << "_";
-      }
-      out << ": " << binding.type();
+      out << binding.name() << ": " << binding.type();
       break;
     }
     case PatternKind::TuplePattern: {

executable_semantics/ast/pattern.h

@@ -101,7 +101,7 @@ class BindingPattern : public Pattern {
  public:
   using ImplementsCarbonNamedEntity = void;
 
-  BindingPattern(SourceLocation source_loc, std::optional<std::string> name,
+  BindingPattern(SourceLocation source_loc, std::string name,
                  Nonnull<Pattern*> type)
       : Pattern(AstNodeKind::BindingPattern, source_loc),
         name_(std::move(name)),
@@ -111,8 +111,10 @@ class BindingPattern : public Pattern {
     return InheritsFromBindingPattern(node->kind());
   }
 
-  // The name this pattern binds, if any.
-  auto name() const -> const std::optional<std::string>& { return name_; }
+  // The name this pattern binds, if any. If equal to AnonymousName, indicates
+  // that this BindingPattern does not bind a name, which in turn means it
+  // should not be used as a NamedEntity.
+  auto name() const -> const std::string& { return name_; }
 
   // The pattern specifying the type of values that this pattern matches.
   auto type() const -> const Pattern& { return *type_; }
@@ -121,7 +123,7 @@ class BindingPattern : public Pattern {
   auto value_category() const -> ValueCategory { return ValueCategory::Var; }
 
  private:
-  std::optional<std::string> name_;
+  std::string name_;
   Nonnull<Pattern*> type_;
 };
 

executable_semantics/ast/statement.cpp

@@ -100,7 +100,7 @@ void Statement::PrintDepth(int depth, llvm::raw_ostream& out) const {
     }
     case StatementKind::Continuation: {
       const auto& cont = cast<Continuation>(*this);
-      out << "continuation " << cont.continuation_variable() << " ";
+      out << "continuation " << cont.name() << " ";
       if (depth < 0 || depth > 1) {
         out << "\n";
       }

executable_semantics/ast/statement.h

@@ -314,19 +314,17 @@ class Continuation : public Statement {
  public:
   using ImplementsCarbonNamedEntity = void;
 
-  Continuation(SourceLocation source_loc, std::string continuation_variable,
+  Continuation(SourceLocation source_loc, std::string name,
                Nonnull<Block*> body)
       : Statement(AstNodeKind::Continuation, source_loc),
-        continuation_variable_(std::move(continuation_variable)),
+        name_(std::move(name)),
         body_(body) {}
 
   static auto classof(const AstNode* node) -> bool {
     return InheritsFromContinuation(node->kind());
   }
 
-  auto continuation_variable() const -> const std::string& {
-    return continuation_variable_;
-  }
+  auto name() const -> const std::string& { return name_; }
   auto body() const -> const Block& { return *body_; }
   auto body() -> Block& { return *body_; }
 
@@ -348,7 +346,7 @@ class Continuation : public Statement {
   auto value_category() const -> ValueCategory { return ValueCategory::Var; }
 
  private:
-  std::string continuation_variable_;
+  std::string name_;
   Nonnull<Block*> body_;
   std::optional<Nonnull<const Value*>> static_type_;
 };

executable_semantics/ast/static_scope.h

@@ -11,6 +11,7 @@
 #include <variant>
 #include <vector>
 
+#include "common/check.h"
 #include "executable_semantics/ast/ast_node.h"
 #include "executable_semantics/ast/source_location.h"
 #include "executable_semantics/ast/value_category.h"
@@ -20,6 +21,9 @@ namespace Carbon {
 
 class Value;
 
+// The placeholder name exposed by anonymous NamedEntities.
+static constexpr std::string_view AnonymousName = "_";
+
 // True if NodeType::ImplementsCarbonNamedEntity is valid and names a type,
 // indicating that NodeType implements the NamedEntity interface, which means
 // it must define the following methods, with contracts as documented.
@@ -29,6 +33,10 @@ class Value;
 
   // Returns the value category of an IdentifierExpression that names *this.
   auto value_category() const -> ValueCategory;
+
+  // Returns the name of an IdentifierExpression that names *this. If *this
+  // is anonymous, returns AnonymousName.
+  auto name() const -> std::string_view;
 #endif
 // NodeType must be derived from AstNode.
 //
@@ -45,6 +53,7 @@ static constexpr bool
 // NodeType implements the NamedEntity interface.
 class NamedEntityView {
  public:
+  // REQUIRES: node->name() != AnonymousName
   template <typename NodeType,
             typename = std::enable_if_t<ImplementsNamedEntity<NodeType>>>
   // NOLINTNEXTLINE(google-explicit-constructor)
@@ -53,12 +62,18 @@ class NamedEntityView {
       // and using std::function to encapsulate the ability to call
       // the derived class's methods.
       : base_(node),
+        name_([](const AstNode& base) -> std::string_view {
+          return llvm::cast<NodeType>(base).name();
+        }),
         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();
-        }) {}
+        }) {
+    CHECK(node->name() != AnonymousName)
+        << "Entity with no name used as NamedEntity: " << *node;
+  }
 
   NamedEntityView(const NamedEntityView&) = default;
   NamedEntityView(NamedEntityView&&) = default;
@@ -68,6 +83,9 @@ class NamedEntityView {
   // Returns `node` as an instance of the base class AstNode.
   auto base() const -> const AstNode& { return *base_; }
 
+  // Returns node->name()
+  auto name() const -> std::string_view { return name_(*base_); }
+
   // Returns node->static_type()
   auto static_type() const -> const Value& { return static_type_(*base_); }
 
@@ -76,18 +94,24 @@ class NamedEntityView {
     return value_category_(*base_);
   }
 
-  friend auto operator==(const NamedEntityView& lhs,
-                         const NamedEntityView& rhs) {
+  friend auto operator==(const NamedEntityView& lhs, const NamedEntityView& rhs)
+      -> bool {
     return lhs.base_ == rhs.base_;
   }
 
-  friend auto operator!=(const NamedEntityView& lhs,
-                         const NamedEntityView& rhs) {
+  friend auto operator!=(const NamedEntityView& lhs, const NamedEntityView& rhs)
+      -> bool {
     return lhs.base_ != rhs.base_;
   }
 
+  friend auto operator<(const NamedEntityView& lhs, const NamedEntityView& rhs)
+      -> bool {
+    return std::less<>()(lhs.base_, rhs.base_);
+  }
+
  private:
   Nonnull<const AstNode*> base_;
+  std::function<std::string_view(const AstNode&)> name_;
   std::function<const Value&(const AstNode&)> static_type_;
   std::function<ValueCategory(const AstNode&)> value_category_;
 };

executable_semantics/interpreter/heap.cpp

@@ -46,8 +46,8 @@ void Heap::Deallocate(AllocationId allocation) {
   if (alive_[allocation.index_]) {
     alive_[allocation.index_] = false;
   } else {
-    FATAL_RUNTIME_ERROR_NO_LINE() << "deallocating an already dead value: "
-                                  << *values_[allocation.index_];
+    FATAL() << "deallocating an already dead value: "
+            << *values_[allocation.index_];
   }
 }
 

executable_semantics/interpreter/interpreter.cpp

@@ -88,7 +88,7 @@ auto Interpreter::EvalPrim(Operator op,
       return arena_->New<BoolValue>(cast<BoolValue>(*args[0]).value() ||
                                     cast<BoolValue>(*args[1]).value());
     case Operator::Eq:
-      return arena_->New<BoolValue>(ValueEqual(args[0], args[1], source_loc));
+      return arena_->New<BoolValue>(ValueEqual(args[0], args[1]));
     case Operator::Ptr:
       return arena_->New<PointerType>(args[0]);
     case Operator::Deref:
@@ -125,7 +125,7 @@ void Interpreter::InitEnv(const Declaration& d, Env* env) {
             const Expression& type_expression =
                 cast<ExpressionPattern>(binding.type()).expression();
             auto type = InterpExp(Env(arena_), &type_expression);
-            fields.push_back({.name = *binding.name(), .value = type});
+            fields.push_back({.name = binding.name(), .value = type});
             break;
           }
         }
@@ -158,7 +158,7 @@ void Interpreter::InitEnv(const Declaration& d, Env* env) {
       Nonnull<const Value*> v =
           Convert(InterpExp(*env, &var.initializer()), &var.static_type());
       AllocationId a = heap_.AllocateValue(v);
-      env->Set(*var.binding().name(), a);
+      env->Set(var.binding().name(), a);
       break;
     }
   }
@@ -189,9 +189,9 @@ auto Interpreter::PatternMatch(Nonnull<const Value*> p, Nonnull<const Value*> v,
     case Value::Kind::BindingPlaceholderValue: {
       const auto& placeholder = cast<BindingPlaceholderValue>(*p);
       Env values(arena_);
-      if (placeholder.name().has_value()) {
+      if (placeholder.named_entity().has_value()) {
         AllocationId a = heap_.AllocateValue(v);
-        values.Set(*placeholder.name(), a);
+        values.Set(std::string(placeholder.named_entity()->name()), a);
       }
       return values;
     }
@@ -283,7 +283,7 @@ auto Interpreter::PatternMatch(Nonnull<const Value*> p, Nonnull<const Value*> v,
       // on the typechecker to ensure that `v` is a type.
       return Env(arena_);
     default:
-      if (ValueEqual(p, v, source_loc)) {
+      if (ValueEqual(p, v)) {
         return Env(arena_);
       } else {
         return std::nullopt;
@@ -680,11 +680,11 @@ void Interpreter::StepPattern() {
     }
     case PatternKind::BindingPattern: {
       const auto& binding = cast<BindingPattern>(pattern);
-      if (act.pos() == 0) {
-        return todo_.Spawn(std::make_unique<PatternAction>(&binding.type()));
+      if (binding.name() != AnonymousName) {
+        return todo_.FinishAction(
+            arena_->New<BindingPlaceholderValue>(&binding));
       } else {
-        return todo_.FinishAction(arena_->New<BindingPlaceholderValue>(
-            binding.name(), act.results()[0]));
+        return todo_.FinishAction(arena_->New<BindingPlaceholderValue>());
       }
     }
     case PatternKind::TuplePattern: {
@@ -926,9 +926,8 @@ void Interpreter::StepStmt() {
       AllocationId continuation_address =
           heap_.AllocateValue(arena_->New<ContinuationValue>(fragment));
       // Bind the continuation object to the continuation variable
-      todo_.CurrentScope().AddLocal(
-          cast<Continuation>(stmt).continuation_variable(),
-          continuation_address);
+      todo_.CurrentScope().AddLocal(cast<Continuation>(stmt).name(),
+                                    continuation_address);
       return todo_.FinishAction();
     }
     case StatementKind::Run: {

executable_semantics/interpreter/resolve_names.cpp

@@ -28,8 +28,8 @@ static void AddExposedNames(const Member& member,
   switch (member.kind()) {
     case MemberKind::FieldMember: {
       const auto& field = cast<FieldMember>(member);
-      if (field.binding().name().has_value()) {
-        enclosing_scope.Add(*field.binding().name(), &field.binding());
+      if (field.binding().name() != AnonymousName) {
+        enclosing_scope.Add(field.binding().name(), &field.binding());
       }
       break;
     }
@@ -56,8 +56,8 @@ static void AddExposedNames(const Declaration& declaration,
     }
     case DeclarationKind::VariableDeclaration:
       auto& var = cast<VariableDeclaration>(declaration);
-      if (var.binding().name().has_value()) {
-        enclosing_scope.Add(*(var.binding().name()), &var.binding());
+      if (var.binding().name() != AnonymousName) {
+        enclosing_scope.Add(var.binding().name(), &var.binding());
       }
       return;
   }
@@ -158,8 +158,8 @@ static void ResolveNames(Pattern& pattern, StaticScope& enclosing_scope) {
     case PatternKind::BindingPattern: {
       auto& binding = cast<BindingPattern>(pattern);
       ResolveNames(binding.type(), enclosing_scope);
-      if (binding.name().has_value()) {
-        enclosing_scope.Add(*binding.name(), &binding);
+      if (binding.name() != AnonymousName) {
+        enclosing_scope.Add(binding.name(), &binding);
       }
       break;
     }
@@ -241,7 +241,7 @@ static void ResolveNames(Statement& statement, StaticScope& enclosing_scope) {
     }
     case StatementKind::Continuation: {
       auto& continuation = cast<Continuation>(statement);
-      enclosing_scope.Add(continuation.continuation_variable(), &continuation);
+      enclosing_scope.Add(continuation.name(), &continuation);
       StaticScope continuation_scope;
       continuation_scope.AddParent(&enclosing_scope);
       ResolveNames(cast<Continuation>(statement).body(), continuation_scope);

executable_semantics/interpreter/type_checker.cpp

@@ -1051,13 +1051,13 @@ void TypeChecker::TypeCheckClassDeclaration(
     switch (m->kind()) {
       case MemberKind::FieldMember: {
         BindingPattern& binding = cast<FieldMember>(*m).binding();
-        if (!binding.name().has_value()) {
+        if (binding.name() == AnonymousName) {
           FATAL_COMPILATION_ERROR(binding.source_loc())
               << "Struct members must have names";
         }
         TypeCheckPattern(&binding, ct_top, std::nullopt);
         fields.push_back(
-            {.name = *binding.name(), .value = &binding.static_type()});
+            {.name = binding.name(), .value = &binding.static_type()});
         break;
       }
     }

executable_semantics/interpreter/value.cpp

@@ -139,12 +139,13 @@ void Value::Print(llvm::raw_ostream& out) const {
     }
     case Value::Kind::BindingPlaceholderValue: {
       const auto& placeholder = cast<BindingPlaceholderValue>(*this);
-      if (placeholder.name().has_value()) {
-        out << *placeholder.name();
+      out << "Placeholder<";
+      if (placeholder.named_entity().has_value()) {
+        out << (*placeholder.named_entity()).name();
       } else {
         out << "_";
       }
-      out << ": " << placeholder.type();
+      out << ">";
       break;
     }
     case Value::Kind::AlternativeValue: {
@@ -374,8 +375,7 @@ auto TypeEqual(Nonnull<const Value*> t1, Nonnull<const Value*> t2) -> bool {
 // Returns true if the two values are equal and returns false otherwise.
 //
 // This function implements the `==` operator of Carbon.
-auto ValueEqual(Nonnull<const Value*> v1, Nonnull<const Value*> v2,
-                SourceLocation source_loc) -> bool {
+auto ValueEqual(Nonnull<const Value*> v1, Nonnull<const Value*> v2) -> bool {
   if (v1->kind() != v2->kind()) {
     return false;
   }
@@ -401,7 +401,7 @@ auto ValueEqual(Nonnull<const Value*> v1, Nonnull<const Value*> v2,
         return false;
       }
       for (size_t i = 0; i < elements1.size(); ++i) {
-        if (!ValueEqual(elements1[i], elements2[i], source_loc)) {
+        if (!ValueEqual(elements1[i], elements2[i])) {
           return false;
         }
       }
@@ -414,7 +414,7 @@ auto ValueEqual(Nonnull<const Value*> v1, Nonnull<const Value*> v2,
       for (size_t i = 0; i < struct_v1.elements().size(); ++i) {
         CHECK(struct_v1.elements()[i].name == struct_v2.elements()[i].name);
         if (!ValueEqual(struct_v1.elements()[i].value,
-                        struct_v2.elements()[i].value, source_loc)) {
+                        struct_v2.elements()[i].value)) {
           return false;
         }
       }

executable_semantics/interpreter/value.h

@@ -285,23 +285,24 @@ class TupleValue : public Value {
 // A binding placeholder value.
 class BindingPlaceholderValue : public Value {
  public:
-  // nullopt represents the `_` placeholder.
-  BindingPlaceholderValue(std::optional<std::string> name,
-                          Nonnull<const Value*> type)
+  // Represents the `_` placeholder.
+  explicit BindingPlaceholderValue() : Value(Kind::BindingPlaceholderValue) {}
+
+  // Represents a named placeholder.
+  explicit BindingPlaceholderValue(NamedEntityView named_entity)
       : Value(Kind::BindingPlaceholderValue),
-        name_(std::move(name)),
-        type_(type) {}
+        named_entity_(std::move(named_entity)) {}
 
   static auto classof(const Value* value) -> bool {
     return value->kind() == Kind::BindingPlaceholderValue;
   }
 
-  auto name() const -> const std::optional<std::string>& { return name_; }
-  auto type() const -> const Value& { return *type_; }
+  auto named_entity() const -> const std::optional<NamedEntityView>& {
+    return named_entity_;
+  }
 
  private:
-  std::optional<std::string> name_;
-  Nonnull<const Value*> type_;
+  std::optional<NamedEntityView> named_entity_;
 };
 
 // The int type.
@@ -601,8 +602,7 @@ class TypeOfChoiceType : public Value {
 };
 
 auto TypeEqual(Nonnull<const Value*> t1, Nonnull<const Value*> t2) -> bool;
-auto ValueEqual(Nonnull<const Value*> v1, Nonnull<const Value*> v2,
-                SourceLocation source_loc) -> bool;
+auto ValueEqual(Nonnull<const Value*> v1, Nonnull<const Value*> v2) -> bool;
 
 }  // namespace Carbon
 

executable_semantics/syntax/parser.ypp

@@ -121,7 +121,7 @@
 %type <Nonnull<StructTypeLiteral*>> struct_type_literal
 %type <std::vector<FieldInitializer>> struct_type_literal_contents
 %type <Nonnull<TupleLiteral*>> tuple
-%type <std::optional<std::string>> binding_lhs
+%type <std::string> binding_lhs
 %type <Nonnull<BindingPattern*>> variable_declaration
 %type <Nonnull<Member*>> member
 %type <std::vector<Nonnull<Member*>>> member_list
@@ -476,7 +476,7 @@ non_expression_pattern:
 ;
 binding_lhs:
   identifier { $$ = $1; }
-| UNDERSCORE { $$ = std::nullopt; }
+| UNDERSCORE { $$ = AnonymousName; }
 ;
 paren_pattern: paren_pattern_base
     { $$ = PatternFromParenContents(arena, context.source_loc(), $1); }
@@ -536,7 +536,7 @@ clause:
 | DEFAULT DOUBLE_ARROW statement
     {
       $$ = Match::Clause(arena->New<BindingPattern>(
-                             context.source_loc(), std::nullopt,
+                             context.source_loc(), std::string(AnonymousName),
                              arena->New<AutoPattern>(context.source_loc())),
                          $3);
     }