Skip to content

Commit

Permalink
[DSLX:fmt] Address new reflow example.
Browse files Browse the repository at this point in the history 
This moves us to (hzeller@ suggested) policy of having the "opening" character
on the line with the leader, to indicate to the reader that the construct
continues on the subsequent line.

This change introduces a distinction of the notion of "blocked expression" on
AST nodes -- there are blocked expressions that have "leader characters" (e.g.
an invocation has a callee as leading characters), and those that do not (e.g.
a block just has an open curl so constant and small amount of characters to
open the construct).

This uncovered a case where the semicolon doesn't fit in the line -- I'll
address this in a followon CL, I think to retain the greedy properties we'll
reduce the page width by one when emitting the statement so that we know we
always have space to put the semicolon inline afterwards, so we can avoid a
semicolon on its own line.

PiperOrigin-RevId: 584145148
  • Loading branch information
@cdleary @copybara-github
cdleary authored and copybara-github committed Nov 20, 2023
1 parent c52230a commit c1a5d1d
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 76 deletions.
80 changes: 53 additions & 27 deletions xls/dslx/fmt/ast_fmt.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -714,8 +714,7 @@ DocRef Fmt(const For& n, const Comments& comments, DocArena& arena) {
arena,
{arena.oparen(), Fmt(*n.init(), comments, arena), arena.cparen()}));

return arena.MakeConcat(ConcatNGroup(arena, pieces),
ConcatN(arena, body_pieces));
return arena.MakeConcat(ConcatN(arena, pieces), ConcatN(arena, body_pieces));
}

DocRef Fmt(const FormatMacro& n, const Comments& comments, DocArena& arena) {
Expand Down Expand Up @@ -812,33 +811,55 @@ static DocRef FmtParametricArg(const ExprOrType& n, const Comments& comments,
}

DocRef Fmt(const Invocation& n, const Comments& comments, DocArena& arena) {
// Starts with the callee.
std::vector<DocRef> pieces = {
Fmt(*n.callee(), comments, arena),
};
DocRef callee_doc = Fmt(*n.callee(), comments, arena);

std::optional<DocRef> parametrics_doc;
if (!n.explicit_parametrics().empty()) {
// Group for the parametrics.
pieces.push_back(ConcatNGroup(
arena, {arena.break0(), arena.oangle(),
parametrics_doc = ConcatNGroup(
arena, {arena.oangle(), arena.break0(),
FmtJoin<ExprOrType>(
absl::MakeConstSpan(n.explicit_parametrics()),
Joiner::kCommaSpace, FmtParametricArg, comments, arena),
arena.cangle()}));
arena.cangle()});
}

DocRef args_doc =
DocRef args_doc_internal =
FmtJoin<const Expr*>(n.args(), Joiner::kCommaBreak1AsGroupNoTrailingComma,
FmtExprPtr, comments, arena);

// Group for the args tokens.
std::vector<DocRef> arg_pieces = {
arena.oparen(),
arena.MakeNestIfFlatFits(/*on_nested_flat_ref=*/args_doc,
/*on_other_ref=*/arena.MakeAlign(args_doc)),
arena.MakeNestIfFlatFits(
/*on_nested_flat_ref=*/args_doc_internal,
/*on_other_ref=*/arena.MakeAlign(args_doc_internal)),
arena.cparen()};
pieces.push_back(ConcatNGroup(arena, arg_pieces));
return ConcatNGroup(arena, pieces);
DocRef args_doc = ConcatNGroup(arena, arg_pieces);
DocRef args_doc_nested = arena.MakeNest(args_doc);

// This is the flat version -- it simply concats the pieces together.
DocRef flat = parametrics_doc.has_value()
? ConcatN(arena, {callee_doc, parametrics_doc.value(),
arena.oparen(), args_doc})
: ConcatN(arena, {callee_doc, arena.oparen(), args_doc});

// This doc ref is for the "I can emit *the leader* flat" case; i.e. the
// callee (or the callee with parametric args).
//
// The parametrics have a break at the start (after the oangle) that can be
// triggered, and the arguments have a break at the start (after the oparen)
// that can be triggered.
DocRef leader_flat =
parametrics_doc.has_value()
? ConcatN(arena, {callee_doc, arena.MakeNest(parametrics_doc.value()),
arena.oparen(), arena.break0(), args_doc_nested})
: ConcatN(arena, {callee_doc, arena.oparen(), arena.break0(),
args_doc_nested});

DocRef result = arena.MakeGroup(
arena.MakeFlatChoice(/*on_flat=*/flat, /*on_break=*/leader_flat));

return result;
}

static DocRef FmtNameDefTreePtr(const NameDefTree* n, const Comments& comments,
Expand Down Expand Up @@ -874,7 +895,7 @@ static DocRef FmtMatchArm(const MatchArm& n, const Comments& comments,
pieces.push_back(arena.space());
// If the RHS is a blocked expression, e.g. a struct instance, we don't
// align it to the fat arrow indicated column.
if (n.expr()->IsBlockedExpr()) {
if (n.expr()->IsBlockedExprAnyLeader()) {
pieces.push_back(arena.MakeGroup(rhs_doc));
} else {
pieces.push_back(arena.MakeAlign(arena.MakeGroup(rhs_doc)));
Expand Down Expand Up @@ -1309,36 +1330,41 @@ DocRef FmtLetWithSemi(const Let& n, const Comments& comments, DocArena& arena,
leader_pieces.push_back(arena.equals());

const DocRef rhs_doc_internal = Fmt(*n.rhs(), comments, arena);

// In the (rare) case where the semicolon is what pushes us over the line
// length, we want to put the semicolon on the subsequent line.
DocRef break0_semi =
arena.MakeGroup(arena.MakeConcat(arena.break0(), arena.semi()));
const DocRef rhs_doc = trailing_semi
? arena.MakeConcat(rhs_doc_internal, arena.semi())
? arena.MakeConcat(rhs_doc_internal, break0_semi)
: rhs_doc_internal;
DocRef body;
if (n.rhs()->IsBlockedExpr() || n.rhs()->kind() == AstNodeKind::kArray ||
n.rhs()->kind() == AstNodeKind::kInvocation) {
if (n.rhs()->IsBlockedExprAnyLeader()) {
// For blocked expressions we don't align them to the equals in the let,
// because it'd shove constructs like `let really_long_identifier = for ...`
// too far to the right hand side.
//
// Similarly for array literals, as they can have lots of elements which
// effectively makes them like blocks.
//
// Note that if you do e.g. a binary operation on blocked constructs as the
// Note: if you do e.g. a binary operation on blocked constructs as the
// RHS it /will/ align because we don't look for blocked constructs
// transitively -- seems reasonable given that's going to look funky no
// matter what.
//
// Note: if it's an expression that acts as a block of contents, but has
// leading chars, e.g. an invocation, so we don't know with reasonable
// confidence it'll fit on the current line.
body = arena.MakeNestIfFlatFits(
/*on_nesated_flat=*/rhs_doc,
/*on_other=*/arena.MakeConcat(arena.space(), rhs_doc));
/*on_nested_flat=*/rhs_doc,
/*on_other=*/ConcatN(arena, {arena.space(), rhs_doc}));
} else {
// Same as above but with an aligned RHS.
DocRef aligned_rhs = arena.MakeAlign(rhs_doc);
DocRef aligned_rhs = arena.MakeAlign(arena.MakeGroup(rhs_doc));
body = arena.MakeNestIfFlatFits(
/*on_nested_flat=*/rhs_doc,
/*on_other=*/arena.MakeConcat(arena.space(), aligned_rhs));
}

DocRef leader = ConcatN(arena, leader_pieces);
return arena.MakeConcat(leader, body);
return ConcatNGroup(arena, {leader, body});
}

/* static */ Comments Comments::Create(absl::Span<const CommentData> comments) {
Expand Down
27 changes: 23 additions & 4 deletions xls/dslx/fmt/ast_fmt_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -541,8 +541,9 @@ TEST_F(FunctionFmtTest, MatchWithCommentOnNonBlockExpressionSecondaryLine) {
TEST_F(FunctionFmtTest, MatchWithOverLongRhs) {
const std::string_view original = R"(fn f(yyyyyyyyyyyyyy: u32) {
match yyyyyyyyyyyyyy {
_ => fffffffffffffffffffffffffffffffff
<u32:7, AAAAAAAAAAAAAAAAAAAAA, BBBBBBBBBBB, CCCCCCCCCC>(yyyyyyyyyyyyyy),
_ => fffffffffffffffffffffffffffffffff<
u32:7, AAAAAAAAAAAAAAAAAAAAA, BBBBBBBBBBB, CCCCCCCCCC>(
yyyyyyyyyyyyyy),
}
})";
XLS_ASSERT_OK_AND_ASSIGN(
Expand Down Expand Up @@ -668,8 +669,9 @@ TEST_F(FunctionFmtTest, FunctionWithParagraphStyleChunksOfStatements) {
TEST_F(FunctionFmtTest, FunctionWithStmtThatNeedsReflow) {
const std::string_view original =
R"(fn f() {
assert_eq(aaaaaaaaaaa,
ffffffffffffffff(xxxxxxxxxxx, yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy, zzzzzzz));
assert_eq(
aaaaaaaaaaa,
ffffffffffffffff(xxxxxxxxxxx, yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy, zzzzzzz));
})";
XLS_ASSERT_OK_AND_ASSIGN(
std::string got,
Expand Down Expand Up @@ -1828,5 +1830,22 @@ fn foo(some_value_that_is_pretty_long: u32, some_other_value_that_is_also_not_to
EXPECT_EQ(got, kProgram);
}

TEST(ModuleFmtTest, LongLetRhs) {
const std::string_view kProgram = R"(import std
fn foo(some_value_that_is_pretty_long: u32, some_other_value_that_is_also_not_too_short: u32) {
type SomeTypeNameThatIsNotTooShort = sN[u32:96];
let very_somewhat_long_variable_name: SomeTypeNameThatIsNotTooShort = std::to_signed(
some_value_that_is_pretty_long ++ some_other_value_that_is_also_not_too_short ++
some_value_that_is_pretty_long);
}
)";
std::vector<CommentData> comments;
XLS_ASSERT_OK_AND_ASSIGN(std::unique_ptr<Module> m,
ParseModule(kProgram, "fake.x", "fake", &comments));
std::string got = AutoFmt(*m, Comments::Create(comments));
EXPECT_EQ(got, kProgram);
}

} // namespace
} // namespace xls::dslx
Loading

0 comments on commit c1a5d1d

Please sign in to comment.