From 621f462a24ed6d8c0516cc6813ffe9cd3e545da8 Mon Sep 17 00:00:00 2001 From: David Plass Date: Thu, 12 Dec 2024 10:06:58 -0800 Subject: [PATCH] Add VerbatimNode to the ImplMember variant, so we can disable formatting for impl functions. Improved format disabler to keep track of which nodes it's already considered, to reduce infinite loops and hopefully other weirdness. Addresses most of https://github.com/google/xls/issues/1781 PiperOrigin-RevId: 705543728 --- xls/dslx/fmt/BUILD | 1 + xls/dslx/fmt/ast_fmt.cc | 1 + xls/dslx/fmt/ast_fmt_test.cc | 48 +++++++++++++++++++++++++++++++++ xls/dslx/fmt/format_disabler.cc | 17 ++++++++++++ xls/dslx/fmt/format_disabler.h | 2 ++ xls/dslx/frontend/ast.cc | 7 ++++- xls/dslx/frontend/ast.h | 2 +- xls/dslx/frontend/ast_cloner.cc | 8 +++--- 8 files changed, 81 insertions(+), 5 deletions(-) diff --git a/xls/dslx/fmt/BUILD b/xls/dslx/fmt/BUILD index d7215c8545..5ed8a6133b 100644 --- a/xls/dslx/fmt/BUILD +++ b/xls/dslx/fmt/BUILD @@ -166,6 +166,7 @@ cc_library( "//xls/dslx/frontend:comment_data", "//xls/dslx/frontend:module", "//xls/dslx/frontend:pos", + "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/log", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", diff --git a/xls/dslx/fmt/ast_fmt.cc b/xls/dslx/fmt/ast_fmt.cc index 1eebb49fb6..059bdbe3c4 100644 --- a/xls/dslx/fmt/ast_fmt.cc +++ b/xls/dslx/fmt/ast_fmt.cc @@ -2414,6 +2414,7 @@ DocRef Formatter::Format(const ImplMember& n) { return absl::visit(Visitor{ [&](const Function* n) { return Format(*n); }, [&](const ConstantDef* n) { return Format(*n); }, + [&](const VerbatimNode* n) { return Format(*n); }, }, n); } diff --git a/xls/dslx/fmt/ast_fmt_test.cc b/xls/dslx/fmt/ast_fmt_test.cc index 62ac8de30e..dd18731342 100644 --- a/xls/dslx/fmt/ast_fmt_test.cc +++ b/xls/dslx/fmt/ast_fmt_test.cc @@ -1603,6 +1603,54 @@ impl MyStruct { )"); } +TEST_F(ModuleFmtTest, ImplWithDisabledFmtFn) { + DoFmt( + R"(struct MyStruct {} + +impl MyStruct { + // dslx-fmt::off +fn my_function() -> u32 { +// Do something important. +u32:3 +} +// dslx-fmt::on + +} +)"); +} + +TEST_F(ModuleFmtTest, ImplWithDisabledFmtExpr) { + DoFmt( + R"(struct MyStruct {} + +impl MyStruct { + fn my_function() -> u32 { + // dslx-fmt::off +// Do something important. +u32:3 +// dslx-fmt::on + + } +} +)"); +} + +TEST_F(ModuleFmtTest, DISABLED_ImplWithDisabledFmtImpl) { + // This test is disabled because the impl becomes a VerbatimNode. Then the + // cloner tries to set it into the struct as its impl, but structs cannot + // take VerbatimNodes as their impl yet. + DoFmt( + R"(struct MyStruct {} + +// dslx-fmt::off +impl MyStruct { fn my_function() -> u32 { +// Do something important. +u32:3 +}} +// dslx-fmt::on +)"); +} + TEST_F(ModuleFmtTest, ImplMethodWithVars) { DoFmt(R"(struct MyStruct { x: u32 } diff --git a/xls/dslx/fmt/format_disabler.cc b/xls/dslx/fmt/format_disabler.cc index b9d2c35b0c..879cdfd3f5 100644 --- a/xls/dslx/fmt/format_disabler.cc +++ b/xls/dslx/fmt/format_disabler.cc @@ -55,33 +55,44 @@ bool IsInDesugaredFn(const AstNode *node) { absl::StatusOr> FormatDisabler::operator()( const AstNode *node) { + if (seen_nodes_.contains(node)) { + return std::nullopt; + } + seen_nodes_.insert(node); + if (node == nullptr || !node->GetSpan().has_value()) { // If there's no node, or no span, we can't know if it's in the unformatted // range, so just return nullopt to indicate it should be unchanged. return std::nullopt; } + VLOG(5) << "FormatDisabler looking at " << node->ToString() << " @ " + << (*node->GetSpan()).ToString(node->owner()->file_name()); // If this node is part of a desugared proc function, we skip it, because it // won't be formatted anyway. // TODO: https://github.com/google/xls/issues/1029 remove desugared proc // functions. if (IsInDesugaredFn(node)) { + VLOG(5) << "In desugared function, stopping"; return std::nullopt; } if (unformatted_end_.has_value()) { + VLOG(6) << "In format disabled mode"; // We are in "format disabled" mode. if (node->GetSpan()->start() < *unformatted_end_) { // This node is within the unformatted range; delete it by returning // an empty VerbatimNode. This is safe because its text has already been // extracted in a VerbatimNode. + VLOG(5) << "Setting previous node to " << node->ToString(); previous_node_ = node; return node->owner()->Make(*node->GetSpan()); } // We're past the end of the unformatted range; clear the setting. + VLOG(5) << "Past end of unformatted range"; unformatted_end_ = std::nullopt; // Note: continue and process this node normally now. @@ -91,6 +102,8 @@ absl::StatusOr> FormatDisabler::operator()( std::vector disable_comments = FindDisablesBetween(previous_node_, node); if (disable_comments.empty()) { + VLOG(5) << "No comments between previous node and this node"; + VLOG(5) << "Setting previous node to " << node->ToString(); previous_node_ = node; // Node should be unchanged. return std::nullopt; @@ -105,6 +118,7 @@ absl::StatusOr> FormatDisabler::operator()( absl::StrCat("Multiple dslx-fmt::off commands between ", previous_node_->ToString(), " and ", node->ToString())); } + VLOG(5) << "Found disable comment between previous node and this node."; const CommentData *disable_comment = disable_comments[0]; // If there's a disable comment between the previous node and this node: @@ -134,11 +148,14 @@ absl::StatusOr> FormatDisabler::operator()( // d. Set a field that indicates the ending position of the last node that // should be unformatted. + VLOG(6) << "Setting unformatted end to " << limit; unformatted_end_ = limit; previous_node_ = node; // e. Replace the current node with the verbatim node from step c + VLOG(5) << "Setting previous node to, and replacing " << node->ToString() + << " with VerbatimNode"; return verbatim_node; } diff --git a/xls/dslx/fmt/format_disabler.h b/xls/dslx/fmt/format_disabler.h index 900f8433b1..a1f90a7bf1 100644 --- a/xls/dslx/fmt/format_disabler.h +++ b/xls/dslx/fmt/format_disabler.h @@ -21,6 +21,7 @@ #include #include +#include "absl/container/flat_hash_set.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "xls/dslx/fmt/comments.h" @@ -87,6 +88,7 @@ class FormatDisabler { // Parsed lines of the module. std::vector lines_; + absl::flat_hash_set seen_nodes_; }; } // namespace xls::dslx diff --git a/xls/dslx/frontend/ast.cc b/xls/dslx/frontend/ast.cc index 2a9c715b9a..06184091a6 100644 --- a/xls/dslx/frontend/ast.cc +++ b/xls/dslx/frontend/ast.cc @@ -1538,7 +1538,12 @@ std::vector Impl::GetConstants() const { static std::string ImplMemberIdentifier(const ImplMember member) { return absl::visit( - Visitor{[](auto* n) { return n->name_def()->identifier(); }}, member); + Visitor{ + [](const ConstantDef* n) { return n->name_def()->identifier(); }, + [](const Function* n) { return n->name_def()->identifier(); }, + [](const VerbatimNode* n) { return std::string("Verbatim"); }, + }, + member); } std::optional Impl::GetMember(std::string_view name) const { diff --git a/xls/dslx/frontend/ast.h b/xls/dslx/frontend/ast.h index bb6c83fe15..1fdc12fefb 100644 --- a/xls/dslx/frontend/ast.h +++ b/xls/dslx/frontend/ast.h @@ -2611,7 +2611,7 @@ inline std::optional TypeDefinitionToStructDefBase( return result == nullptr ? std::nullopt : std::make_optional(result); } -using ImplMember = std::variant; +using ImplMember = std::variant; // Represents an impl for a struct. class Impl : public AstNode { diff --git a/xls/dslx/frontend/ast_cloner.cc b/xls/dslx/frontend/ast_cloner.cc index 4b6f3dce33..651891742e 100644 --- a/xls/dslx/frontend/ast_cloner.cc +++ b/xls/dslx/frontend/ast_cloner.cc @@ -785,10 +785,12 @@ class AstCloner : public AstNodeVisitor { XLS_RETURN_IF_ERROR(ReplaceOrVisit(member_node)); } AstNode* new_node = old_to_new_.at(member_node); - if (std::holds_alternative(member)) { - new_members.push_back(down_cast(new_node)); + if (auto* new_constant_def = dynamic_cast(new_node)) { + new_members.push_back(new_constant_def); + } else if (auto* new_function = dynamic_cast(new_node)) { + new_members.push_back(new_function); } else { - new_members.push_back(down_cast(new_node)); + new_members.push_back(down_cast(new_node)); } } new_impl->set_members(new_members);