Skip to content

Fix formatting of forward declared generics #5530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 29, 2025
+23 −13
Merged

Fix formatting of forward declared generics #5530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions toolchain/sem_ir/formatter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,23 @@ auto Formatter::ShouldIncludeInstByIR(InstId inst_id) -> bool {
return include_ir_in_dumps_[import_ir->check_ir_id().index];
}

auto Formatter::ShouldFormatEntity(InstId decl_id, bool is_definition_start)
-> bool {
// Returns true for a `DefinitionStart` node.
static auto IsDefinitionStart(Parse::NodeKind node_kind) -> bool {
switch (node_kind) {
case Parse::NodeKind::BuiltinFunctionDefinitionStart:
case Parse::NodeKind::ChoiceDefinitionStart:
case Parse::NodeKind::ClassDefinitionStart:
case Parse::NodeKind::FunctionDefinitionStart:
case Parse::NodeKind::ImplDefinitionStart:
case Parse::NodeKind::InterfaceDefinitionStart:
case Parse::NodeKind::NamedConstraintDefinitionStart:
return true;
default:
return false;
}
}
Comment on lines +172 to +185
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to move this into parse somewhere in a way that makes it clear that it needs to be defined as true/false when adding new kinds of Nodes, so that Check can then just query it off the NodeKind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think nodes have support like that, and this seems pretty minor to add new support, but maybe I'm missing something; can you give a little more detail about what you're suggesting?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I was thinking something like a bool is_definition_start() on the Parse::NodeKind struct here:

carbon-lang/toolchain/parse/node_kind.h

Line 34 in 3946cac

class NodeKind : public CARBON_ENUM_BASE(NodeKind) {

So it could be added to NodeKind::DefinitionArgs with a default value of false but overridden to true in typed_nodes.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My perspective is that wouldn't need to be defined when adding new kinds, it just could be when people remember. We could do this by adding a NodeCategory, but I feel odd doing that when there's only one use, and any definition starts seem like they'll remain rare (and with handling specific to the formatter).

We also already have IsStartOfDeferredDefinitionScope and IsEndOfDeferredDefinitionScope in node_id_traversal.cpp, which feel similar; this keeps the check and logic close to the code using it.

How strongly do you feel here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't think it's a win, then that's fine go with what you think is best. I thought that by having it in Parse, when adding something new I would at least have a hope of seeing all the things I need to set and think about which it is - and maybe copy/paste from a similar one too. Whereas over here, it's unlikely I will remember to set this correctly if I added a new node that should be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably better. FWIW, my thought is it might be easier to notice here when adding printing of a new entity. If the entity doesn't need formatting, then it also doesn't need handling here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think I agree with Jon here, at least at this stage. The criteria I would use to sink this into Parse is there being more clients, especially if those are spread out somewhat.


auto Formatter::ShouldFormatEntity(InstId decl_id) -> bool {
if (!decl_id.has_value()) {
return true;
}
Expand Down Expand Up @@ -201,7 +216,7 @@ auto Formatter::ShouldFormatEntity(InstId decl_id, bool is_definition_start)
// to find the `Definition`, giving a range that includes the definition's
// body.
auto end_node_id = loc_id.node_id();
if (is_definition_start) {
if (IsDefinitionStart(sem_ir_->parse_tree().node_kind(end_node_id))) {
end_node_id = node_parents_[end_node_id.index];
}

Expand All @@ -212,8 +227,7 @@ auto Formatter::ShouldFormatEntity(InstId decl_id, bool is_definition_start)
}

auto Formatter::ShouldFormatEntity(const EntityWithParamsBase& entity) -> bool {
return ShouldFormatEntity(entity.latest_decl_id(),
entity.definition_id.has_value());
return ShouldFormatEntity(entity.latest_decl_id());
}

auto Formatter::ShouldFormatInst(InstId inst_id) -> bool {
Expand Down Expand Up @@ -379,8 +393,7 @@ auto Formatter::FormatInterface(InterfaceId id) -> void {
auto Formatter::FormatAssociatedConstant(AssociatedConstantId id) -> void {
const AssociatedConstant& assoc_const =
sem_ir_->associated_constants().Get(id);
if (!ShouldFormatEntity(assoc_const.decl_id,
/*is_definition_start=*/false)) {
if (!ShouldFormatEntity(assoc_const.decl_id)) {
return;
}

Expand Down Expand Up @@ -538,9 +551,7 @@ auto Formatter::FormatSpecificRegion(const Generic& generic,
auto Formatter::FormatSpecific(SpecificId id) -> void {
const auto& specific = sem_ir_->specifics().Get(id);
const auto& generic = sem_ir_->generics().Get(specific.generic_id);
if (!ShouldFormatEntity(
generic.decl_id,
/*is_definition_start=*/generic.definition_block_id.has_value())) {
if (!ShouldFormatEntity(generic.decl_id)) {
// Omit specifics if we also omitted the generic.
return;
}
Expand Down
5 changes: 2 additions & 3 deletions toolchain/sem_ir/formatter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,8 @@ class Formatter {
auto ShouldIncludeInstByIR(InstId inst_id) -> bool;

// Determines whether the specified entity should be included in the formatted
// output. `is_definition_start` should indicate whether, if `decl_id`'s
// `LocId` is a `NodeId`, it is expected to be a `DefinitionStart` kind.
auto ShouldFormatEntity(InstId decl_id, bool is_definition_start) -> bool;
// output.
auto ShouldFormatEntity(InstId decl_id) -> bool;

auto ShouldFormatEntity(const EntityWithParamsBase& entity) -> bool;

Expand Down
Loading