Make IdTag typesafe by danakj · Pull Request #6574 · carbon-language/carbon-lang

danakj · 2026-01-09T16:14:02Z

The IdTag knows the type of the Id its tagging and the type of the Id being used as the tag. This prevents mixing up tagged and untagged ids, and avoids having to work with untyped integers.

Adds an Untagged marker struct that's used as the tag type in IdTag when no tag is desired.

The complexity of ConstantIds and TypeIds became a bit visible: TypeIds are concrete ConstantIds. And ConstantIds have two different tagging schemes, one for concrete and one for symbolic ids. And ConstantIds are actually re-cast InstIds with the same index. The LoweredTypeStore needs to work with tagged TypeIds, but the tags actually come from an InstId store in ConstantValueStore. Now this is expressed in the type system by getting the tags for TypeIds from the ConstantValueStore.

ValueStores without an TagId type parameter are now visibly untagged.

IdTag is now only default constructible when it does not have a tag, which means ValueStore is only default constructible when the TagId is untagged. This forces tagged value stores to be constructed correctly with a tag at compile time, and untagged ones to be constructed without.

FixedSizeValueStore has overloads for dealing with tagged and untagged Ids, since it can't default-construct ValueStore for tagged ids, and no longer requires passing in default-constructed tags when there is no tag in the ids.

chandlerc

I like the direction here, but currently it feels unfortunate how much duplication there is. Specifically:

Duplicating the id type: FooStore<InstId, ..., IdTag<InstId, ...>>
Writing CheckIRId everywhere...

I wonder if we could do something a bit more terse... My current idea is:

Use template aliases for Untagged<IdT> and TagId<IdT> to select between Unchecked vs. CheckIRId for the second template parameter.
- If we do have other checked tags, create relevant aliases for them such as IRTagId, SymbolicTagId, ...
Use a template-template argument to the stores so we can just pass Untagged or TagId, and have it apply the IdT to it.

But not sure if that will work -- template template arguments are ... fussy. Let me know if not and if you don't see other easy ways to achieve this.

chandlerc · 2026-01-09T22:30:28Z

    CARBON_DCHECK(index >= 0, "{0}", index);
    if (index < initial_reserved_ids_) {
-      return index;
+      return IdT{index};


We generally use ()s to construct objects unless that doesn't compile or we're building some kind of container/tuple/struct of elements? This doesn't really feel like a container to me...

Maybe the Abseil tip is a useful heuristic here? https://abseil.io/tips/88

(Or let me know if I'm wrong and this actually is a case that doesn't compile w/o {}s...)

This choice here (and for TagIdT) was around the fact the type could be an integer and we like to use {} for those cuz we get compile-time errors for narrowing. But at this point I think there are no integers, so probably not gaining anything. The initialization store inside a template is a little unfortunate (or in general but yeah).

chandlerc · 2026-01-09T22:45:38Z

+// IdTags for ConstantIds are slightly complex, and you need to know if the
+// constant is concrete or symbolic to know its tag:
+// - Concrete ConstantIds use the tag of the store of InstIds.
+// - Symbolic ConstantIds are tagged by the internal SymbolicId.


Are they tagged by the SymbolicId, or by the CheckIRId associated with the SymbolicId?

Right, fixed, it's the tag from the SymbolicId store

danakj · 2026-01-12T18:39:02Z

I like the direction here, but currently it feels unfortunate how much duplication there is. Specifically:

Duplicating the id type: FooStore<InstId, ..., IdTag<InstId, ...>>

Writing CheckIRId everywhere...

I wonder if we could do something a bit more terse... My current idea is:

Use template aliases for Untagged<IdT> and TagId<IdT> to select between Unchecked vs. CheckIRId for the second template parameter.

If we do have other checked tags, create relevant aliases for them such as IRTagId, SymbolicTagId, ...

Use a template-template argument to the stores so we can just pass Untagged or TagId, and have it apply the IdT to it.

But not sure if that will work -- template template arguments are ... fussy. Let me know if not and if you don't see other easy ways to achieve this.

I tried this out and it got complex, but then realized we don't need anything fancy template-wise. The use of IdTag as the store parameter was based on the idea that you might need ValueStore<T, ..., IdTag<U, ...>> but that Does Not Work, which is what led to the IdTag type-conversion methods in ConstantValueStore. So we can just use the TagId as the template parameter to ValueStore and similar, and construct the (somewhat redundant) IdTag type internally, and we get the same type safety without syntactic redundancy.

danakj

PTAL

danakj · 2026-01-12T18:40:22Z

    CARBON_DCHECK(index >= 0, "{0}", index);
    if (index < initial_reserved_ids_) {
-      return index;
+      return IdT{index};


This choice here (and for TagIdT) was around the fact the type could be an integer and we like to use {} for those cuz we get compile-time errors for narrowing. But at this point I think there are no integers, so probably not gaining anything. The initialization store inside a template is a little unfortunate (or in general but yeah).

danakj · 2026-01-12T18:43:55Z

+// IdTags for ConstantIds are slightly complex, and you need to know if the
+// constant is concrete or symbolic to know its tag:
+// - Concrete ConstantIds use the tag of the store of InstIds.
+// - Symbolic ConstantIds are tagged by the internal SymbolicId.


Right, fixed, it's the tag from the SymbolicId store

chandlerc

LG

There are a few things that might be worth thinking about for follow-ups, but I'm not sure how to do them or if they're worth it, and they definitely don't seem like blockers...

One is it's a little hard on the reader to see ValueStore<FooId, ..., BarId> and remember that this is a store of FooIds, but tagged by BarIds. I almost want it to be CheckIRId::AsTag, and when constructing check_ir_id_.as_tag(). But not sure that would work, and it adds complexity that I'm not sure it pays for...

The other thing is that the Untagged default seems unfortunate for ValueStore... I think we want most of them to be tagged. But as you pointed out, we can't default to SemIR::CheckIRId in //toolchain/base. This makes me wonder whether we should have a ValueStore wrapper in SemIR that provides the default, and maybe not have a default in the //toolchain/base one? But very uncertain about this, just figured I'd mention it in case it sparks a good idea.

danakj · 2026-01-13T17:16:34Z

LG

There are a few things that might be worth thinking about for follow-ups, but I'm not sure how to do them or if they're worth it, and they definitely don't seem like blockers...

One is it's a little hard on the reader to see ValueStore<FooId, ..., BarId> and remember that this is a store of FooIds, but tagged by BarIds. I almost want it to be CheckIRId::AsTag, and when constructing check_ir_id_.as_tag(). But not sure that would work, and it adds complexity that I'm not sure it pays for...

We could make the tag parameter either Untagged or Tag<TagIdT>, so then you have to write Tag<CheckIRId> instead. I have done that here now, PTAL and see what you think?

The other thing is that the Untagged default seems unfortunate for ValueStore... I think we want most of them to be tagged. But as you pointed out, we can't default to SemIR::CheckIRId in //toolchain/base. This makes me wonder whether we should have a ValueStore wrapper in SemIR that provides the default, and maybe not have a default in the //toolchain/base one? But very uncertain about this, just figured I'd mention it in case it sparks a good idea.

On top of the base issue, we want most of the ValueStores in semir and check to be tagged, But there are also value stores all over parse which don't use tagging. I am not super keen on a wrapper/alias in semir, as I feel like it would be obfuscating the types more than helping.

chandlerc

LG!

id-tag-typesafe

6e17056

danakj requested a review from a team as a code owner January 9, 2026 16:14

danakj requested review from chandlerc and removed request for a team January 9, 2026 16:14

github-actions Bot added the toolchain label Jan 9, 2026

chandlerc reviewed Jan 9, 2026

View reviewed changes

danakj added 2 commits January 12, 2026 13:47

simpler-templates

a3b9d9d

review

45738db

danakj commented Jan 12, 2026

View reviewed changes

danakj requested a review from chandlerc January 12, 2026 18:51

merge trunk

2347c37

chandlerc approved these changes Jan 12, 2026

View reviewed changes

Comment thread toolchain/base/block_value_store.h

Comment thread toolchain/base/id_tag.h Outdated

danakj added 4 commits January 13, 2026 11:49

missed-braces

d4337a0

require-not-untagged

601c676

tag-template

23e6676

build

dd4fe4c

danakj requested a review from chandlerc January 13, 2026 17:16

merge trunk

1662a90

danakj enabled auto-merge January 13, 2026 21:57

chandlerc approved these changes Jan 13, 2026

View reviewed changes

danakj added this pull request to the merge queue Jan 13, 2026

Merged via the queue into carbon-language:trunk with commit c64117d Jan 13, 2026
8 checks passed

danakj deleted the id-tag-typesafe branch January 13, 2026 23:54

Conversation

danakj commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chandlerc left a comment

Choose a reason for hiding this comment

Uh oh!

chandlerc Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

chandlerc Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

danakj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chandlerc Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

danakj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danakj commented Jan 12, 2026

Uh oh!

danakj left a comment

Choose a reason for hiding this comment

Uh oh!

danakj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danakj Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chandlerc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danakj commented Jan 13, 2026

Uh oh!

chandlerc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danakj commented Jan 9, 2026 •

edited

Loading