Make the names of declarations unusable before the point where their type is known. (#1352)

Following #875, diagnose any use of a name prior to the point where it is introduced and its type is known.

This works by performing name resolution on a top-level class, interface, or impl twice: the first pass performs name-resolution for everything other than nested function bodies, and the second pass performs name resolution on function bodies. At the moment, the second pass does a superset of the work done by the first pass, and as a consequence, some identifier expressions now have their target set twice to the same thing.

.gitignore

@@ -23,3 +23,6 @@ compile_commands.json
 # Emacs temporary files
 *~
 \#*\#
+
+# vim temporary files
+.*.swp

explorer/ast/expression.h

@@ -145,10 +145,10 @@ class IdentifierExpression : public Expression {
   // before name resolution.
   auto value_node() const -> const ValueNodeView& { return *value_node_; }
 
-  // Sets the value returned by value_node. Can be called only once,
-  // during name resolution.
+  // Sets the value returned by value_node. Can be called only during name
+  // resolution.
   void set_value_node(ValueNodeView value_node) {
-    CARBON_CHECK(!value_node_.has_value());
+    CARBON_CHECK(!value_node_.has_value() || value_node_ == value_node);
     value_node_ = std::move(value_node);
   }
 
@@ -180,9 +180,9 @@ class DotSelfExpression : public Expression {
   auto self_binding() const -> const GenericBinding& { return **self_binding_; }
   auto self_binding() -> GenericBinding& { return **self_binding_; }
 
-  // Sets the self binding. Called only once, during name resolution.
+  // Sets the self binding. Called only during name resolution.
   void set_self_binding(Nonnull<GenericBinding*> self_binding) {
-    CARBON_CHECK(!self_binding_.has_value());
+    CARBON_CHECK(!self_binding_.has_value() || self_binding_ == self_binding);
     self_binding_ = self_binding;
   }
 

explorer/ast/static_scope.cpp

@@ -21,6 +21,7 @@ auto StaticScope::Add(const std::string& name, ValueNodeView entity,
     CARBON_CHECK(usable || !it->second.usable)
         << entity.base().source_loc() << " attempting to mark a usable name `"
         << name << "` as unusable";
+    it->second.usable |= usable;
   }
   return Success();
 }

explorer/interpreter/resolve_names.cpp

@@ -19,16 +19,13 @@ using llvm::cast;
 namespace Carbon {
 
 // Adds the names exposed by the given AST node to enclosing_scope.
-static auto AddExposedNames(const Declaration& declaration,
-                            StaticScope& enclosing_scope) -> ErrorOr<Success>;
-
 static auto AddExposedNames(const Declaration& declaration,
                             StaticScope& enclosing_scope) -> ErrorOr<Success> {
   switch (declaration.kind()) {
     case DeclarationKind::InterfaceDeclaration: {
       auto& iface_decl = cast<InterfaceDeclaration>(declaration);
-      CARBON_RETURN_IF_ERROR(
-          enclosing_scope.Add(iface_decl.name(), &iface_decl));
+      CARBON_RETURN_IF_ERROR(enclosing_scope.Add(iface_decl.name(), &iface_decl,
+                                                 /*usable=*/false));
       break;
     }
     case DeclarationKind::ImplDeclaration: {
@@ -37,13 +34,14 @@ static auto AddExposedNames(const Declaration& declaration,
     }
     case DeclarationKind::FunctionDeclaration: {
       auto& func = cast<FunctionDeclaration>(declaration);
-      CARBON_RETURN_IF_ERROR(enclosing_scope.Add(func.name(), &func));
+      CARBON_RETURN_IF_ERROR(
+          enclosing_scope.Add(func.name(), &func, /*usable=*/false));
       break;
     }
     case DeclarationKind::ClassDeclaration: {
       auto& class_decl = cast<ClassDeclaration>(declaration);
-      CARBON_RETURN_IF_ERROR(
-          enclosing_scope.Add(class_decl.name(), &class_decl));
+      CARBON_RETURN_IF_ERROR(enclosing_scope.Add(class_decl.name(), &class_decl,
+                                                 /*usable=*/false));
       break;
     }
     case DeclarationKind::ChoiceDeclaration: {
@@ -55,8 +53,8 @@ static auto AddExposedNames(const Declaration& declaration,
     case DeclarationKind::VariableDeclaration: {
       auto& var = cast<VariableDeclaration>(declaration);
       if (var.binding().name() != AnonymousName) {
-        CARBON_RETURN_IF_ERROR(
-            enclosing_scope.Add(var.binding().name(), &var.binding()));
+        CARBON_RETURN_IF_ERROR(enclosing_scope.Add(
+            var.binding().name(), &var.binding(), /*usable=*/false));
       }
       break;
     }
@@ -75,16 +73,33 @@ static auto AddExposedNames(const Declaration& declaration,
   return Success();
 }
 
+namespace {
+enum class ResolveFunctionBodies {
+  // Do not resolve names in function bodies.
+  Skip,
+  // Resolve all names. When visiting a declaration with members, resolve
+  // names in member function bodies after resolving the names in all member
+  // declarations, as if the bodies appeared after all the declarations.
+  AfterDeclarations,
+  // Resolve names in function bodies immediately. This is appropriate when
+  // the declarations of all members of enclosing classes, interfaces, and
+  // similar have already been resolved.
+  Immediately,
+};
+}  // namespace
+
 // Traverses the sub-AST rooted at the given node, resolving all names within
 // it using enclosing_scope, and updating enclosing_scope to add names to
 // it as they become available. In scopes where names are only visible below
 // their point of declaration (such as block scopes in C++), this is implemented
 // as a single pass, recursively calling ResolveNames on the elements of the
 // scope in order. In scopes where names are also visible above their point of
-// declaration (such as class scopes in C++), this requires two passes: first
+// declaration (such as class scopes in C++), this requires three passes: first
 // calling AddExposedNames on each element of the scope to populate a
 // StaticScope, and then calling ResolveNames on each element, passing it the
-// already-populated StaticScope.
+// already-populated StaticScope but skipping member function bodies, and
+// finally calling ResolvedNames again on each element, and this time resolving
+// member function bodies.
 static auto ResolveNames(Expression& expression,
                          const StaticScope& enclosing_scope)
     -> ErrorOr<Success>;
@@ -95,8 +110,8 @@ static auto ResolveNames(Pattern& pattern, StaticScope& enclosing_scope)
     -> ErrorOr<Success>;
 static auto ResolveNames(Statement& statement, StaticScope& enclosing_scope)
     -> ErrorOr<Success>;
-static auto ResolveNames(Declaration& declaration, StaticScope& enclosing_scope)
-    -> ErrorOr<Success>;
+static auto ResolveNames(Declaration& declaration, StaticScope& enclosing_scope,
+                         ResolveFunctionBodies bodies) -> ErrorOr<Success>;
 
 static auto ResolveNames(Expression& expression,
                          const StaticScope& enclosing_scope)
@@ -389,8 +404,29 @@ static auto ResolveNames(Statement& statement, StaticScope& enclosing_scope)
   return Success();
 }
 
-static auto ResolveNames(Declaration& declaration, StaticScope& enclosing_scope)
+static auto ResolveMemberNames(llvm::ArrayRef<Nonnull<Declaration*>> members,
+                               StaticScope& scope, ResolveFunctionBodies bodies)
     -> ErrorOr<Success> {
+  for (Nonnull<Declaration*> member : members) {
+    CARBON_RETURN_IF_ERROR(AddExposedNames(*member, scope));
+  }
+  if (bodies != ResolveFunctionBodies::Immediately) {
+    for (Nonnull<Declaration*> member : members) {
+      CARBON_RETURN_IF_ERROR(
+          ResolveNames(*member, scope, ResolveFunctionBodies::Skip));
+    }
+  }
+  if (bodies != ResolveFunctionBodies::Skip) {
+    for (Nonnull<Declaration*> member : members) {
+      CARBON_RETURN_IF_ERROR(
+          ResolveNames(*member, scope, ResolveFunctionBodies::Immediately));
+    }
+  }
+  return Success();
+}
+
+static auto ResolveNames(Declaration& declaration, StaticScope& enclosing_scope,
+                         ResolveFunctionBodies bodies) -> ErrorOr<Success> {
   switch (declaration.kind()) {
     case DeclarationKind::InterfaceDeclaration: {
       auto& iface = cast<InterfaceDeclaration>(declaration);
@@ -399,13 +435,10 @@ static auto ResolveNames(Declaration& declaration, StaticScope& enclosing_scope)
       if (iface.params().has_value()) {
         CARBON_RETURN_IF_ERROR(ResolveNames(**iface.params(), iface_scope));
       }
+      enclosing_scope.MarkUsable(iface.name());
       CARBON_RETURN_IF_ERROR(iface_scope.Add("Self", iface.self()));
-      for (Nonnull<Declaration*> member : iface.members()) {
-        CARBON_RETURN_IF_ERROR(AddExposedNames(*member, iface_scope));
-      }
-      for (Nonnull<Declaration*> member : iface.members()) {
-        CARBON_RETURN_IF_ERROR(ResolveNames(*member, iface_scope));
-      }
+      CARBON_RETURN_IF_ERROR(
+          ResolveMemberNames(iface.members(), iface_scope, bodies));
       break;
     }
     case DeclarationKind::ImplDeclaration: {
@@ -426,12 +459,8 @@ static auto ResolveNames(Declaration& declaration, StaticScope& enclosing_scope)
         CARBON_RETURN_IF_ERROR(AddExposedNames(*impl.self(), impl_scope));
       }
       CARBON_RETURN_IF_ERROR(ResolveNames(impl.interface(), impl_scope));
-      for (Nonnull<Declaration*> member : impl.members()) {
-        CARBON_RETURN_IF_ERROR(AddExposedNames(*member, impl_scope));
-      }
-      for (Nonnull<Declaration*> member : impl.members()) {
-        CARBON_RETURN_IF_ERROR(ResolveNames(*member, impl_scope));
-      }
+      CARBON_RETURN_IF_ERROR(
+          ResolveMemberNames(impl.members(), impl_scope, bodies));
       break;
     }
     case DeclarationKind::FunctionDeclaration: {
@@ -451,7 +480,9 @@ static auto ResolveNames(Declaration& declaration, StaticScope& enclosing_scope)
         CARBON_RETURN_IF_ERROR(ResolveNames(
             **function.return_term().type_expression(), function_scope));
       }
-      if (function.body().has_value()) {
+      enclosing_scope.MarkUsable(function.name());
+      if (function.body().has_value() &&
+          bodies != ResolveFunctionBodies::Skip) {
         CARBON_RETURN_IF_ERROR(ResolveNames(**function.body(), function_scope));
       }
       break;
@@ -460,23 +491,14 @@ static auto ResolveNames(Declaration& declaration, StaticScope& enclosing_scope)
       auto& class_decl = cast<ClassDeclaration>(declaration);
       StaticScope class_scope;
       class_scope.AddParent(&enclosing_scope);
-      CARBON_RETURN_IF_ERROR(class_scope.Add(class_decl.name(), &class_decl));
-      CARBON_RETURN_IF_ERROR(AddExposedNames(*class_decl.self(), class_scope));
       if (class_decl.type_params().has_value()) {
         CARBON_RETURN_IF_ERROR(
             ResolveNames(**class_decl.type_params(), class_scope));
       }
-
-      // TODO: Disable unqualified access of members by other members for now.
-      // Put it back later, but in a way that turns unqualified accesses
-      // into qualified ones, so that generic classes and impls
-      // behave the in the right way. -Jeremy
-      // for (Nonnull<Declaration*> member : class_decl.members()) {
-      //   AddExposedNames(*member, class_scope);
-      // }
-      for (Nonnull<Declaration*> member : class_decl.members()) {
-        CARBON_RETURN_IF_ERROR(ResolveNames(*member, class_scope));
-      }
+      enclosing_scope.MarkUsable(class_decl.name());
+      CARBON_RETURN_IF_ERROR(AddExposedNames(*class_decl.self(), class_scope));
+      CARBON_RETURN_IF_ERROR(
+          ResolveMemberNames(class_decl.members(), class_scope, bodies));
       break;
     }
     case DeclarationKind::ChoiceDeclaration: {
@@ -527,7 +549,8 @@ auto ResolveNames(AST& ast) -> ErrorOr<Success> {
     CARBON_RETURN_IF_ERROR(AddExposedNames(*declaration, file_scope));
   }
   for (auto declaration : ast.declarations) {
-    CARBON_RETURN_IF_ERROR(ResolveNames(*declaration, file_scope));
+    CARBON_RETURN_IF_ERROR(ResolveNames(
+        *declaration, file_scope, ResolveFunctionBodies::AfterDeclarations));
   }
   return ResolveNames(**ast.main_call, file_scope);
 }

explorer/testdata/name_lookup/class_fn_body_reorder.carbon

@@ -0,0 +1,27 @@
+// 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
+//
+// RUN: %{explorer} %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
+// RUN: %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
+// AUTOUPDATE: %{explorer} %s
+// CHECK: result: 3
+
+package ExplorerTest api;
+
+// The bodies of member functions are processed after all immediately enclosing
+// classes, impls, and interfaces.
+class A {
+  fn F[me: Self]() -> i32 {
+    return G() + me.H();
+  }
+  fn G() -> i32 { return 1; }
+  fn H[me: Self]() -> i32 { return 2; }
+}
+
+fn Main() -> i32 {
+  var a: A = {};
+  return a.F();
+}

explorer/testdata/name_lookup/fail_class_fn_use_before_declaration.carbon

@@ -0,0 +1,35 @@
+// 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
+//
+// RUN: %{not} %{explorer} %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
+// RUN: %{not} %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
+// AUTOUPDATE: %{explorer} %s
+
+package ExplorerTest api;
+
+// The bodies of member functions are processed after all immediately enclosing
+// classes, impls, and interfaces.
+class A {
+  fn F() -> Type {
+    return i32;
+  }
+  // OK, resolves to `A.F`.
+  fn G() -> F() {
+    return 0;
+  }
+
+  // CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/name_lookup/fail_class_fn_use_before_declaration.carbon:[[@LINE+1]]: 'I' is not usable until after it has been completely declared
+  fn H() -> I() {
+    return 0;
+  }
+  fn I() -> Type {
+    return i32;
+  }
+}
+
+fn Main() -> i32 {
+  return 0;
+}

explorer/testdata/name_lookup/fail_class_use_in_params.carbon

@@ -0,0 +1,19 @@
+// 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
+//
+// RUN: %{not} %{explorer} %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
+// RUN: %{not} %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
+// AUTOUPDATE: %{explorer} %s
+
+package ExplorerTest api;
+
+// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/name_lookup/fail_class_use_in_params.carbon:[[@LINE+1]]: 'A' is not usable until after it has been completely declared
+class A(X:! A(i32)) {
+}
+
+fn Main() -> i32 {
+  return x;
+}

explorer/testdata/name_lookup/fail_fn_use_in_param.carbon

@@ -0,0 +1,20 @@
+// 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
+//
+// RUN: %{not} %{explorer} %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
+// RUN: %{not} %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
+// AUTOUPDATE: %{explorer} %s
+
+package ExplorerTest api;
+
+// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/name_lookup/fail_fn_use_in_param.carbon:[[@LINE+1]]: 'F' is not usable until after it has been completely declared
+fn F(x: F(0)) -> Type {
+  return i32;
+}
+
+fn Main() -> i32 {
+  return x;
+}

explorer/testdata/name_lookup/fail_fn_use_in_return_type.carbon

@@ -0,0 +1,20 @@
+// 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
+//
+// RUN: %{not} %{explorer} %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
+// RUN: %{not} %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
+// AUTOUPDATE: %{explorer} %s
+
+package ExplorerTest api;
+
+// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/name_lookup/fail_fn_use_in_return_type.carbon:[[@LINE+1]]: 'F' is not usable until after it has been completely declared
+fn F() -> F() {
+  return Type;
+}
+
+fn Main() -> i32 {
+  return x;
+}

explorer/testdata/global_variable/fail_init_order.carbon → explorer/testdata/name_lookup/fail_var_use_before_declaration.carbon

@@ -13,7 +13,7 @@ package ExplorerTest api;
 // Test that a global variable may not depend on a later global.
 // Error expected.
 
-// CHECK: RUNTIME ERROR: {{.*}}/explorer/testdata/global_variable/fail_init_order.carbon:[[@LINE+1]]: could not find `y: i32`
+// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/name_lookup/fail_var_use_before_declaration.carbon:[[@LINE+1]]: 'y' is not usable until after it has been completely declared
 var x: i32 = y;
 
 var y: i32 = 0;