[DSLX:cleanup] Make NameDef member on Import be a ref to document non…

…-nullness. This is one of those cleanup categories that's hard to switch over "all at once", so taking the scout approach "leave it better than you found it" mentality can be useful. This was better than documenting the "must not be null" property that corresponded to the XLS_CHECK in the implementation while it was topical. PiperOrigin-RevId: 604838672
google · Feb 7, 2024 · ea279d9 · ea279d9
1 parent c0b133c
commit ea279d9
Show file tree

Hide file tree

Showing 9 changed files with 42 additions and 18 deletions.
diff --git a/xls/dslx/frontend/BUILD b/xls/dslx/frontend/BUILD
@@ -40,6 +40,7 @@ cc_library(
         "//xls/common:visitor",
         "//xls/common/status:ret_check",
         "//xls/common/status:status_macros",
+        "//xls/ir:format_strings",
     ],
 )
 
@@ -297,6 +298,7 @@ cc_library(
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/strings:str_format",
+        "@com_google_absl//absl/types:variant",
         "//xls/common:visitor",
         "//xls/common/logging",
         "//xls/common/status:status_macros",

diff --git a/xls/dslx/frontend/ast.cc b/xls/dslx/frontend/ast.cc
@@ -751,14 +751,13 @@ TypeRef::~TypeRef() = default;
 // -- class Import
 
 Import::Import(Module* owner, Span span, std::vector<std::string> subject,
-               NameDef* name_def, std::optional<std::string> alias)
+               NameDef& name_def, std::optional<std::string> alias)
     : AstNode(owner),
       span_(std::move(span)),
       subject_(std::move(subject)),
       name_def_(name_def),
       alias_(std::move(alias)) {
   XLS_CHECK(!subject_.empty());
-  XLS_CHECK(name_def != nullptr);
 }
 
 Import::~Import() = default;

diff --git a/xls/dslx/frontend/ast.h b/xls/dslx/frontend/ast.h
@@ -1165,11 +1165,22 @@ class TypeRef : public AstNode {
 };
 
 // Represents an import statement; e.g.
-//  import std as my_std
+//  import std;
+// Or:
+//  import foo.bar.baz;
+// Or:
+//  import foo.bar.baz as my_alias;
+//
+// Attributes:
+//  span: Span of the overall import statement in the text.
+//  subject: The imported name; e.g. `foo.bar.baz`.
+//  name_def: The name definition node for the binding this import creates.
+//  alias: Alias that the imported name is bound to (if an `as` keyword was
+//    present).
 class Import : public AstNode {
  public:
   Import(Module* owner, Span span, std::vector<std::string> subject,
-         NameDef* name_def, std::optional<std::string> alias);
+         NameDef& name_def, std::optional<std::string> alias);
 
   ~Import() override;
 
@@ -1179,29 +1190,24 @@ class Import : public AstNode {
     return v->HandleImport(this);
   }
   std::string_view GetNodeTypeName() const override { return "Import"; }
-  const std::string& identifier() const { return name_def_->identifier(); }
+  const std::string& identifier() const { return name_def_.identifier(); }
 
   std::string ToString() const override;
 
   std::vector<AstNode*> GetChildren(bool want_types) const override {
-    return {name_def_};
+    return {&name_def_};
   }
 
   const std::vector<std::string>& subject() const { return subject_; }
-  NameDef* name_def() const { return name_def_; }
+  NameDef& name_def() const { return name_def_; }
   const Span& span() const { return span_; }
   std::optional<Span> GetSpan() const override { return span_; }
   const std::optional<std::string>& alias() const { return alias_; }
 
  private:
-  // Span of the import in the text.
   Span span_;
-  // Name of the module being imported ("original" name before aliasing); e.g.
-  // "std".
   std::vector<std::string> subject_;
-  // The name definition we bind the import to.
-  NameDef* name_def_;
-  // The identifier text we bind the import to.
+  NameDef& name_def_;
   std::optional<std::string> alias_;
 };
 

diff --git a/xls/dslx/frontend/ast_cloner.cc b/xls/dslx/frontend/ast_cloner.cc
@@ -35,6 +35,7 @@
 #include "xls/dslx/frontend/ast_utils.h"
 #include "xls/dslx/frontend/module.h"
 #include "xls/dslx/frontend/proc.h"
+#include "xls/ir/format_strings.h"
 
 namespace xls::dslx {
 namespace {
@@ -341,7 +342,7 @@ class AstCloner : public AstNodeVisitor {
 
     old_to_new_[n] = module_->Make<Import>(
         n->span(), n->subject(),
-        down_cast<NameDef*>(old_to_new_.at(n->name_def())), n->alias());
+        *down_cast<NameDef*>(old_to_new_.at(&n->name_def())), n->alias());
     return absl::OkStatus();
   }
 

diff --git a/xls/dslx/frontend/ast_utils_test.cc b/xls/dslx/frontend/ast_utils_test.cc
@@ -17,6 +17,7 @@
 #include <memory>
 #include <optional>
 #include <string>
+#include <string_view>
 #include <utility>
 #include <vector>
 
@@ -140,7 +141,7 @@ TEST(ProcConfigIrConverterTest, ResolveProcColonRef) {
   Module module("test_module", /*fs_path=*/std::nullopt);
   NameDef* module_def =
       module.Make<NameDef>(Span::Fake(), "import_module", nullptr);
-  Import* import = module.Make<Import>(Span::Fake(), import_tokens, module_def,
+  Import* import = module.Make<Import>(Span::Fake(), import_tokens, *module_def,
                                        std::nullopt);
   module_def->set_definer(import);
   NameRef* module_ref =

diff --git a/xls/dslx/frontend/bindings.cc b/xls/dslx/frontend/bindings.cc
@@ -25,8 +25,12 @@
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/str_split.h"
+#include "absl/types/variant.h"
+#include "xls/common/logging/logging.h"
 #include "xls/common/status/status_macros.h"
 #include "xls/common/visitor.h"
+#include "xls/dslx/frontend/ast.h"
+#include "xls/dslx/frontend/pos.h"
 #include "re2/re2.h"
 
 namespace xls::dslx {
@@ -94,7 +98,7 @@ AnyNameDef BoundNodeToAnyNameDef(BoundNode bn) {
     return std::get<StructDef*>(bn)->name_def();
   }
   if (std::holds_alternative<Import*>(bn)) {
-    return std::get<Import*>(bn)->name_def();
+    return &std::get<Import*>(bn)->name_def();
   }
   XLS_LOG(FATAL) << "Unsupported BoundNode variant: "
                  << ToAstNode(bn)->ToString() << " "

diff --git a/xls/dslx/frontend/parser.cc b/xls/dslx/frontend/parser.cc
@@ -1405,7 +1405,7 @@ absl::StatusOr<Import*> Parser::ParseImport(Bindings& bindings) {
       DropTokenOrError(TokenKind::kSemi, /*start=*/&kw,
                        /*context=*/"Expect an ';' at end of import statement"));
 
-  auto* import = module_->Make<Import>(kw.span(), subject, name_def, alias);
+  auto* import = module_->Make<Import>(kw.span(), subject, *name_def, alias);
   name_def->set_definer(import);
   bindings.Add(name_def->identifier(), import);
   return import;

diff --git a/xls/dslx/lsp/BUILD b/xls/dslx/lsp/BUILD
@@ -49,8 +49,12 @@ cc_library(
         ":lsp_type_utils",
         "@com_google_absl//absl/container:flat_hash_map",
         "@com_google_absl//absl/status",
+        "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/time",
         "//xls/common:indent",
+        "//xls/common/logging",
+        "//xls/common/logging:vlog_is_on",
         "//xls/dslx:create_import_data",
         "//xls/dslx:extract_module_name",
         "//xls/dslx:import_data",
@@ -60,6 +64,7 @@ cc_library(
         "//xls/dslx/frontend:ast",
         "//xls/dslx/frontend:ast_utils",
         "//xls/dslx/frontend:bindings",
+        "//xls/dslx/frontend:pos",
         "@verible//common/lsp:lsp-file-utils",
         "@verible//common/lsp:lsp-protocol",
     ],

diff --git a/xls/dslx/lsp/language_server_adapter.cc b/xls/dslx/lsp/language_server_adapter.cc
@@ -24,15 +24,21 @@
 #include <vector>
 
 #include "absl/status/status.h"
+#include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
+#include "absl/time/clock.h"
+#include "absl/time/time.h"
 #include "external/verible/common/lsp/lsp-file-utils.h"
 #include "external/verible/common/lsp/lsp-protocol.h"
 #include "xls/common/indent.h"
+#include "xls/common/logging/logging.h"
+#include "xls/common/logging/vlog_is_on.h"
 #include "xls/dslx/create_import_data.h"
 #include "xls/dslx/extract_module_name.h"
 #include "xls/dslx/frontend/ast.h"
 #include "xls/dslx/frontend/ast_utils.h"
 #include "xls/dslx/frontend/bindings.h"
+#include "xls/dslx/frontend/pos.h"
 #include "xls/dslx/import_data.h"
 #include "xls/dslx/lsp/document_symbols.h"
 #include "xls/dslx/lsp/find_definition.h"
@@ -213,7 +219,7 @@ LanguageServerAdapter::ProvideImportLinks(std::string_view uri) const {
         continue;
       }
       verible::lsp::DocumentLink link = {
-          .range = ConvertSpanToLspRange(import_node->name_def()->span()),
+          .range = ConvertSpanToLspRange(import_node->name_def().span()),
           .target = verible::lsp::PathToLSPUri(info.value()->path().string()),
           .has_target = true,
       };