Skip to content

Commit e6872f9

Browse files
jonmeowjonmeow
authored
Change NodeIdOneOf and similar to use "requires" and explicit UnsafeMake (#5084)
This doesn't change functionality, but I was seeing better diagnostics in VS Code. This also changes the NodeId constructors for related types (also NodeCategory and NodeIdForKind) to use UnsafeMake for construction. That originated from avoiding ambiguity coming from `requires`, but the constructor mode is also one we should typically avoid (e.g., preferring `Parse::Tree::As`).
1 parent ebaf62e commit e6872f9

File tree

8 files changed

+65
-37
lines changed

8 files changed

+65
-37
lines changed

‎toolchain/check/check_unit.cpp‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -378,10 +378,11 @@ auto CheckUnit::ProcessNodeIds() -> bool {
378378
bool result;
379379
auto parse_kind = context_.parse_tree().node_kind(node_id);
380380
switch (parse_kind) {
381-
#define CARBON_PARSE_NODE_KIND(Name) \
382-
case Parse::NodeKind::Name: { \
383-
result = HandleParseNode(context_, Parse::Name##Id(node_id)); \
384-
break; \
381+
#define CARBON_PARSE_NODE_KIND(Name) \
382+
case Parse::NodeKind::Name: { \
383+
result = HandleParseNode( \
384+
context_, context_.parse_tree().As<Parse::Name##Id>(node_id)); \
385+
break; \
385386
}
386387
#include "toolchain/parse/node_kind.def"
387388
}

‎toolchain/check/import.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,9 @@ auto AddImportNamespace(Context& context, SemIR::TypeId namespace_type_id,
133133
? MakeImportedLocIdAndInst(context, import_loc_id.import_ir_inst_id(),
134134
namespace_inst)
135135
// TODO: Check that this actually is an `AnyNamespaceId`.
136-
: SemIR::LocIdAndInst(Parse::AnyNamespaceId(import_loc_id.node_id()),
137-
namespace_inst);
136+
: SemIR::LocIdAndInst(
137+
Parse::AnyNamespaceId::UnsafeMake(import_loc_id.node_id()),
138+
namespace_inst);
138139
auto namespace_id =
139140
AddPlaceholderInstInNoBlock(context, namespace_inst_and_loc);
140141
context.import_ref_ids().push_back(namespace_id);

‎toolchain/check/node_stack.h‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,8 @@ class NodeStack {
140140
auto PopForSoloNodeId() -> Parse::NodeIdForKind<RequiredParseKind> {
141141
Entry back = PopEntry<SemIR::InstId>();
142142
RequireIdKind(RequiredParseKind, Id::Kind::None);
143-
RequireParseKind<RequiredParseKind>(back.node_id);
144-
return Parse::NodeIdForKind<RequiredParseKind>(back.node_id);
143+
return parse_tree_->As<Parse::NodeIdForKind<RequiredParseKind>>(
144+
back.node_id);
145145
}
146146

147147
// Pops the top of the stack if it is the given kind, and returns the
@@ -192,7 +192,7 @@ class NodeStack {
192192
template <const Parse::NodeKind& RequiredParseKind>
193193
auto PopWithNodeId() -> auto {
194194
auto id = Peek<RequiredParseKind>();
195-
Parse::NodeIdForKind<RequiredParseKind> node_id(
195+
auto node_id = parse_tree_->As<Parse::NodeIdForKind<RequiredParseKind>>(
196196
stack_.pop_back_val().node_id);
197197
return std::make_pair(node_id, id);
198198
}
@@ -201,8 +201,9 @@ class NodeStack {
201201
template <Parse::NodeCategory::RawEnumType RequiredParseCategory>
202202
auto PopWithNodeId() -> auto {
203203
auto id = Peek<RequiredParseCategory>();
204-
Parse::NodeIdInCategory<RequiredParseCategory> node_id(
205-
stack_.pop_back_val().node_id);
204+
auto node_id =
205+
parse_tree_->As<Parse::NodeIdInCategory<RequiredParseCategory>>(
206+
stack_.pop_back_val().node_id);
206207
return std::make_pair(node_id, id);
207208
}
208209

@@ -302,7 +303,9 @@ class NodeStack {
302303
template <const Parse::NodeKind& RequiredParseKind>
303304
auto Peek() const -> auto {
304305
Entry back = stack_.back();
305-
RequireParseKind<RequiredParseKind>(back.node_id);
306+
CARBON_CHECK(RequiredParseKind == parse_tree_->node_kind(back.node_id),
307+
"Expected {0}, found {1}", RequiredParseKind,
308+
parse_tree_->node_kind(back.node_id));
306309
constexpr Id::Kind RequiredIdKind = NodeKindToIdKind(RequiredParseKind);
307310
return Peek<RequiredIdKind>();
308311
}
@@ -589,14 +592,6 @@ class NodeStack {
589592
SemIR::IdKind(NodeKindToIdKind(parse_kind)));
590593
}
591594

592-
// Require an entry to have the given Parse::NodeKind.
593-
template <const Parse::NodeKind& RequiredParseKind>
594-
auto RequireParseKind(Parse::NodeId node_id) const -> void {
595-
auto actual_kind = parse_tree_->node_kind(node_id);
596-
CARBON_CHECK(RequiredParseKind == actual_kind, "Expected {0}, found {1}",
597-
RequiredParseKind, actual_kind);
598-
}
599-
600595
// Require an entry to have the given Parse::NodeCategory.
601596
template <Parse::NodeCategory::RawEnumType RequiredParseCategory>
602597
auto RequireParseCategory(Parse::NodeId node_id) const -> void {

‎toolchain/parse/context.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -449,8 +449,8 @@ auto Context::AddFunctionDefinitionStart(Lex::TokenIndex token, bool has_error)
449449
-> void {
450450
if (ParsingInDeferredDefinitionScope(*this)) {
451451
deferred_definition_stack_.push_back(tree_->deferred_definitions_.Add(
452-
{.start_id =
453-
FunctionDefinitionStartId(NodeId(tree_->node_impls_.size()))}));
452+
{.start_id = FunctionDefinitionStartId::UnsafeMake(
453+
NodeId(tree_->node_impls_.size()))}));
454454
}
455455

456456
AddNode(NodeKind::FunctionDefinitionStart, token, has_error);
@@ -462,7 +462,7 @@ auto Context::AddFunctionDefinition(Lex::TokenIndex token, bool has_error)
462462
auto definition_index = deferred_definition_stack_.pop_back_val();
463463
auto& definition = tree_->deferred_definitions_.Get(definition_index);
464464
definition.definition_id =
465-
FunctionDefinitionId(NodeId(tree_->node_impls_.size()));
465+
FunctionDefinitionId::UnsafeMake(NodeId(tree_->node_impls_.size()));
466466
definition.next_definition_index =
467467
DeferredDefinitionIndex(tree_->deferred_definitions().size());
468468
}

‎toolchain/parse/extract.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ struct Extractable<NodeIdForKind<Kind>> {
155155
static auto Extract(NodeExtractor& extractor)
156156
-> std::optional<NodeIdForKind<Kind>> {
157157
if (extractor.MatchesNodeIdForKind(Kind)) {
158-
return NodeIdForKind<Kind>(extractor.ExtractNode());
158+
return NodeIdForKind<Kind>::UnsafeMake(extractor.ExtractNode());
159159
} else {
160160
return std::nullopt;
161161
}
@@ -182,7 +182,7 @@ struct Extractable<NodeIdInCategory<Category>> {
182182
static auto Extract(NodeExtractor& extractor)
183183
-> std::optional<NodeIdInCategory<Category>> {
184184
if (extractor.MatchesNodeIdInCategory(Category)) {
185-
return NodeIdInCategory<Category>(extractor.ExtractNode());
185+
return NodeIdInCategory<Category>::UnsafeMake(extractor.ExtractNode());
186186
} else {
187187
return std::nullopt;
188188
}
@@ -227,7 +227,7 @@ struct Extractable<NodeIdOneOf<T...>> {
227227
static auto Extract(NodeExtractor& extractor)
228228
-> std::optional<NodeIdOneOf<T...>> {
229229
if (extractor.MatchesNodeIdOneOf({T::Kind...})) {
230-
return NodeIdOneOf<T...>(extractor.ExtractNode());
230+
return NodeIdOneOf<T...>::UnsafeMake(extractor.ExtractNode());
231231
} else {
232232
return std::nullopt;
233233
}

‎toolchain/parse/handle_import_and_package.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ static auto HandleDeclContent(Context& context, Context::StateStackEntry state,
4040
llvm::function_ref<auto()->void> on_parse_error)
4141
-> void {
4242
Tree::PackagingNames names{
43-
.node_id = ImportDeclId(NodeId(state.subtree_start)),
43+
.node_id = ImportDeclId::UnsafeMake(NodeId(state.subtree_start)),
4444
.is_export = is_export};
4545

4646
// Parse the package name.

‎toolchain/parse/node_ids.h‎

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,20 @@ template <const NodeKind& K>
4040
struct NodeIdForKind : public NodeId {
4141
// NOLINTNEXTLINE(readability-identifier-naming)
4242
static const NodeKind& Kind;
43-
constexpr explicit NodeIdForKind(NodeId node_id) : NodeId(node_id) {}
43+
44+
// Provide a factory function for construction from `NodeId`. This doesn't
45+
// validate the type, so it's unsafe.
46+
static constexpr auto UnsafeMake(NodeId node_id) -> NodeIdForKind {
47+
return NodeIdForKind(node_id);
48+
}
49+
4450
// NOLINTNEXTLINE(google-explicit-constructor)
4551
constexpr NodeIdForKind(NoneNodeId /*none*/) : NodeId(NoneIndex) {}
52+
53+
private:
54+
// Private to prevent accidental explicit construction from an untyped
55+
// NodeId.
56+
explicit constexpr NodeIdForKind(NodeId node_id) : NodeId(node_id) {}
4657
};
4758
template <const NodeKind& K>
4859
const NodeKind& NodeIdForKind<K>::Kind = K;
@@ -54,6 +65,12 @@ const NodeKind& NodeIdForKind<K>::Kind = K;
5465
// NodeId that matches any NodeKind whose `category()` overlaps with `Category`.
5566
template <NodeCategory::RawEnumType Category>
5667
struct NodeIdInCategory : public NodeId {
68+
// Provide a factory function for construction from `NodeId`. This doesn't
69+
// validate the type, so it's unsafe.
70+
static constexpr auto UnsafeMake(NodeId node_id) -> NodeIdInCategory {
71+
return NodeIdInCategory(node_id);
72+
}
73+
5774
// Support conversion from `NodeIdForKind<Kind>` if Kind's category
5875
// overlaps with `Category`.
5976
template <const NodeKind& Kind>
@@ -62,9 +79,13 @@ struct NodeIdInCategory : public NodeId {
6279
CARBON_CHECK(Kind.category().HasAnyOf(Category));
6380
}
6481

65-
constexpr explicit NodeIdInCategory(NodeId node_id) : NodeId(node_id) {}
6682
// NOLINTNEXTLINE(google-explicit-constructor)
6783
constexpr NodeIdInCategory(NoneNodeId /*none*/) : NodeId(NoneIndex) {}
84+
85+
private:
86+
// Private to prevent accidental explicit construction from an untyped
87+
// NodeId.
88+
explicit constexpr NodeIdInCategory(NodeId node_id) : NodeId(node_id) {}
6889
};
6990

7091
// Aliases for `NodeIdInCategory` to describe particular categories of nodes.
@@ -84,16 +105,26 @@ using AnyPackageNameId = NodeIdInCategory<NodeCategory::PackageName>;
84105

85106
// NodeId with kind that matches one of the `T::Kind`s.
86107
template <typename... T>
108+
requires(sizeof...(T) >= 2)
87109
struct NodeIdOneOf : public NodeId {
88-
static_assert(sizeof...(T) >= 2, "Expected at least two types.");
89-
constexpr explicit NodeIdOneOf(NodeId node_id) : NodeId(node_id) {}
110+
// Provide a factory function for construction from `NodeId`. This doesn't
111+
// validate the type, so it's unsafe.
112+
static constexpr auto UnsafeMake(NodeId node_id) -> NodeIdOneOf {
113+
return NodeIdOneOf(node_id);
114+
}
115+
90116
template <const NodeKind& Kind>
117+
requires((T::Kind == Kind) || ...)
91118
// NOLINTNEXTLINE(google-explicit-constructor)
92-
NodeIdOneOf(NodeIdForKind<Kind> node_id) : NodeId(node_id) {
93-
static_assert(((T::Kind == Kind) || ...));
94-
}
119+
NodeIdOneOf(NodeIdForKind<Kind> node_id) : NodeId(node_id) {}
120+
95121
// NOLINTNEXTLINE(google-explicit-constructor)
96122
constexpr NodeIdOneOf(NoneNodeId /*none*/) : NodeId(NoneIndex) {}
123+
124+
private:
125+
// Private to prevent accidental explicit construction from an untyped
126+
// NodeId.
127+
explicit constexpr NodeIdOneOf(NodeId node_id) : NodeId(node_id) {}
97128
};
98129

99130
using AnyClassDeclId =

‎toolchain/parse/tree.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ class Tree : public Printable<Tree> {
146146
auto TryAs(NodeId n) const -> std::optional<T> {
147147
CARBON_DCHECK(n.has_value());
148148
if (ConvertTo<T>::AllowedFor(node_kind(n))) {
149-
return T(n);
149+
return T::UnsafeMake(n);
150150
} else {
151151
return std::nullopt;
152152
}
@@ -157,8 +157,8 @@ class Tree : public Printable<Tree> {
157157
template <typename T>
158158
auto As(NodeId n) const -> T {
159159
CARBON_DCHECK(n.has_value());
160-
CARBON_CHECK(ConvertTo<T>::AllowedFor(node_kind(n)));
161-
return T(n);
160+
CARBON_DCHECK(ConvertTo<T>::AllowedFor(node_kind(n)));
161+
return T::UnsafeMake(n);
162162
}
163163

164164
auto packaging_decl() const -> const std::optional<PackagingDecl>& {

0 commit comments

Comments
 (0)