Skip to content

Commit

Permalink
Insert comments before, between and after items in a struct literal.
Browse files Browse the repository at this point in the history 
Fixes google#1719

PiperOrigin-RevId: 708015178
  • Loading branch information
@dplassgit @copybara-github
dplassgit authored and copybara-github committed Dec 19, 2024
1 parent 9a28850 commit 222a1a3
Show file tree
Hide file tree
Showing 2 changed files with 209 additions and 50 deletions.
141 changes: 91 additions & 50 deletions xls/dslx/fmt/ast_fmt.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1390,52 +1390,85 @@ DocRef FmtStructMembersFlat(
}

DocRef FmtStructMembersBreak(
absl::Span<const std::pair<std::string, Expr*>> members, Comments& comments,
DocArena& arena) {
return FmtJoin<std::pair<std::string, Expr*>>(
members, Joiner::kCommaHardlineTrailingCommaAlways,
[](const auto& member, Comments& comments, DocArena& arena) {
const auto& [field_name, expr] = member;
// If the expression is an identifier that matches its corresponding
// struct member name, we canonically use the shorthand notation of just
// providing the identifier and leaving the member name implicitly as
// the same symbol.
if (const NameRef* name_ref = dynamic_cast<const NameRef*>(expr);
name_ref != nullptr && name_ref->identifier() == field_name) {
return arena.MakeText(field_name);
}
Span struct_span, absl::Span<const std::pair<std::string, Expr*>> members,
Comments& comments, DocArena& arena) {
std::vector<DocRef> pieces;
Pos previous_item_limit = struct_span.start();
for (size_t i = 0; i < members.size(); ++i) {
const std::pair<std::string, Expr*>& member = members[i];
const auto& [field_name, expr] = member;

DocRef field_expr = Fmt(*expr, comments, arena);

// This is the document we want to emit both when we:
// - Know it fits in flat mode
// - Know the start of the document (i.e. the leader on the RHS
// expression) can be emitted in flat mode
//
// That's why it has a `break1` in it (instead of a space) and a
// reassessment of whether to enter break mode for the field
// expression.
DocRef on_flat =
ConcatN(arena, {arena.MakeText(field_name), arena.colon(),
arena.break1(), arena.MakeGroup(field_expr)});
DocRef nest_field_expr =
ConcatN(arena, {arena.MakeText(field_name), arena.colon(),
arena.hard_line(), arena.MakeNest(field_expr)});

DocRef on_other;
if (expr->IsBlockedExprWithLeader()) {
DocRef leader = ConcatN(
arena, {arena.MakeText(field_name), arena.colon(), arena.space(),
FmtBlockedExprLeader(*expr, comments, arena)});
on_other = arena.MakeModeSelect(leader, /*on_flat=*/on_flat,
/*on_break=*/nest_field_expr);
} else {
on_other = arena.MakeFlatChoice(on_flat, nest_field_expr);
}
// If there are comments between the last item and here, insert them.
Span comment_span(previous_item_limit, expr->span().start());
if (std::optional<DocRef> previous_comments =
EmitCommentsBetween(comment_span.start(), comment_span.limit(),
comments, arena, nullptr)) {
pieces.push_back(previous_comments.value());
pieces.push_back(arena.hard_line());
}
previous_item_limit = expr->span().limit();

// If the expression is an identifier that matches its corresponding
// struct member name, we canonically use the shorthand notation of just
// providing the identifier and leaving the member name implicitly as
// the same symbol.
DocRef member_doc;
if (const NameRef* name_ref = dynamic_cast<const NameRef*>(expr);
name_ref != nullptr && name_ref->identifier() == field_name) {
member_doc = arena.MakeText(field_name);
} else {
// First we format the member into a doc and then decide the best way to
// put it into the sequence.
DocRef field_expr = Fmt(*expr, comments, arena);

// This is the document we want to emit both when we:
// - Know it fits in flat mode
// - Know the start of the document (i.e. the leader on the RHS
// expression) can be emitted in flat mode
//
// That's why it has a `break1` in it (instead of a space) and a
// reassessment of whether to enter break mode for the field
// expression.
DocRef on_flat =
ConcatN(arena, {arena.MakeText(field_name), arena.colon(),
arena.break1(), arena.MakeGroup(field_expr)});
DocRef nest_field_expr =
ConcatN(arena, {arena.MakeText(field_name), arena.colon(),
arena.hard_line(), arena.MakeNest(field_expr)});

DocRef on_other;
if (expr->IsBlockedExprWithLeader()) {
DocRef leader = ConcatN(
arena, {arena.MakeText(field_name), arena.colon(), arena.space(),
FmtBlockedExprLeader(*expr, comments, arena)});
on_other = arena.MakeModeSelect(leader, /*on_flat=*/on_flat,
/*on_break=*/nest_field_expr);
} else {
on_other = arena.MakeFlatChoice(on_flat, nest_field_expr);
}

return arena.MakeNestIfFlatFits(on_flat, on_other);
},
comments, arena);
member_doc = arena.MakeNestIfFlatFits(on_flat, on_other);
}

pieces.push_back(member_doc);
pieces.push_back(arena.comma());
// TODO: https://github.com/google/xls/issues/1719 - if there is a comment
// on the same line as this item, insert the comment first.
if (i + 1 != members.size()) {
// Not the last item, add a hardline
pieces.push_back(arena.hard_line());
}
}

// Insert comments between the last item and the end of the struct.
Span comment_span(previous_item_limit, struct_span.limit());
if (std::optional<DocRef> previous_comments =
EmitCommentsBetween(comment_span.start(), comment_span.limit(),
comments, arena, nullptr)) {
pieces.push_back(arena.hard_line());
pieces.push_back(previous_comments.value());
}
return ConcatNGroup(arena, pieces);
}

DocRef FmtFlatRest(const StructInstance& n, Comments& comments,
Expand All @@ -1448,10 +1481,11 @@ DocRef FmtFlatRest(const StructInstance& n, Comments& comments,

DocRef FmtBreakRest(const StructInstance& n, Comments& comments,
DocArena& arena) {
return ConcatN(arena, {arena.hard_line(),
arena.MakeNest(FmtStructMembersBreak(
n.GetUnorderedMembers(), comments, arena)),
arena.hard_line(), arena.ccurl()});
return ConcatN(arena,
{arena.hard_line(),
arena.MakeNest(FmtStructMembersBreak(
n.span(), n.GetUnorderedMembers(), comments, arena)),
arena.hard_line(), arena.ccurl()});
}

DocRef Fmt(const StructInstance& n, Comments& comments, DocArena& arena) {
Expand All @@ -1466,8 +1500,14 @@ DocRef Fmt(const StructInstance& n, Comments& comments, DocArena& arena) {
// definition may be defined an an imported file, and we have auto-formatting
// work purely at the single-file syntax level.

DocRef on_flat = FmtFlatRest(n, comments, arena);
// If there are comments within the span, we must go to break mode, because
// newlines.
DocRef on_break = FmtBreakRest(n, comments, arena);
if (CommentsWithin(n.span(), comments)) {
return arena.MakeConcat(leader, on_break);
}

DocRef on_flat = FmtFlatRest(n, comments, arena);
return arena.MakeConcat(
leader, arena.MakeGroup(arena.MakeFlatChoice(on_flat, on_break)));
}
Expand All @@ -1487,7 +1527,8 @@ DocRef Fmt(const SplatStructInstance& n, Comments& comments, DocArena& arena) {
DocRef on_break = ConcatN(
arena, {arena.hard_line(),
arena.MakeNest(ConcatN(
arena, {FmtStructMembersBreak(n.members(), comments, arena),
arena, {FmtStructMembersBreak(n.span(), n.members(), comments,
arena),
arena.hard_line(), arena.dot_dot(), splatted})),
arena.hard_line(), arena.ccurl()});
return arena.MakeConcat(
Expand Down
118 changes: 118 additions & 0 deletions xls/dslx/fmt/ast_fmt_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3203,5 +3203,123 @@ TEST_F(ModuleFmtTest, DisableFmtAroundTestFn) {
)");
}

TEST_F(ModuleFmtTest, CommentBeforeFirstItemStructLiteral) {
DoFmt(R"(struct Foo { a: u1, b: u1 }
fn test() {
let foo = Foo {
// Comment before
a: u1:0,
b: u1:0,
};
}
)");
}

TEST_F(ModuleFmtTest, CommentBetweenItemsInStructLiteral) {
DoFmt(R"(struct Foo { a: u1, b: u1 }
fn test() {
let foo = Foo {
a: u1:0,
// and between
b: u1:0,
};
}
)");
}

TEST_F(ModuleFmtTest, CommentAfterLastItemInStructLiteral) {
DoFmt(R"(struct Foo { a: u1, b: u1 }
fn test() {
let foo = Foo {
a: u1:0,
b: u1:0,
// and after.
};
}
)");
}

TEST_F(ModuleFmtTest, CommentThroughoutInStructLiteral) {
DoFmt(R"(struct Foo { a: u1, b: u1 }
fn test() {
let foo = Foo {
// Comment before
a: u1:0,
// and between
b: u1:0,
// and after.
};
}
)");
}

TEST_F(ModuleFmtTest, CommentSameLineAsItemInStructLiteral) {
// TODO: https://github.com/google/xls/issues/1719 - if there is a comment
// on the same line as this item, don't insert a hard-line.
DoFmt(R"(struct Foo { a: u1, b: u1 }
fn test() {
let foo = Foo {
a: u1:0, // and between
b: u1:0, // and after.
};
}
)",
R"(struct Foo { a: u1, b: u1 }
fn test() {
let foo = Foo {
a: u1:0,
// and between
b: u1:0,
// and after.
};
}
)");
}

TEST_F(ModuleFmtTest, CommentAfterStructLiteralSpan) {
DoFmt(R"(struct Foo { a: u1, b: u1 }
fn test() {
let foo = Foo { a: u1:0, b: u1:0 }; // after only
}
)");
}

TEST_F(ModuleFmtTest, StructLiteral) {
DoFmt(R"(struct Foo { a: u1, b: u1 }
fn test() { let foo = Foo { a: u1:0, b: u1:0 }; }
)");
}

TEST_F(ModuleFmtTest, StructLiteralShorthand) {
DoFmt(R"(struct Foo { a: u1, b: u1 }
fn test() {
let a = u1:0;
let foo = Foo {
a,
b: u1:0 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1,
};
}
)");
}
TEST_F(ModuleFmtTest, StructLiteralBreak) {
DoFmt(R"(struct Foo { a: u1, b: u1 }
fn test() {
let foo = Foo {
a: u1:0 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1,
b: u1:0 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1 + u1:1,
};
}
)");
}
} // namespace
} // namespace xls::dslx

0 comments on commit 222a1a3

Please sign in to comment.