Track the type as written in `BaseDecl` and `AdaptDecl`. #4564

zygoloid · 2024-11-21T22:06:15Z

Represent the type as an InstId rather than as a TypeId to preserve how it was written and better support tracking its value in a generic. Add accessors to Class to get the base and adapted type to reduce code duplication, and add TypeStore::GetObjectRepr to make it easier to map from a type to its possibly-adapted object representation type. In passing, also move GetIntTypeInfo and GetUnqualifiedType into TypeStore.

This fixes specifics of generic adapters to properly look at the specific adapted type, and also fixes importing of adapters.

…ed with integer tyye builtins.

jonmeow · 2024-11-22T16:36:18Z

toolchain/sem_ir/class.cpp

+auto Class::GetAdaptedType(const File& file, SpecificId specific_id) const
+    -> TypeId {
+  if (!adapt_id.is_valid()) {
+    return TypeId::Invalid;
+  }
+  if (base_id == SemIR::InstId::BuiltinErrorInst) {
+    return TypeId::Error;
+  }
+  return TypeId::ForTypeConstant(GetConstantValueInSpecific(
+      file, specific_id,
+      file.insts().GetAs<AdaptDecl>(adapt_id).adapted_type_inst_id));
+}
+
+auto Class::GetBaseType(const File& file, SpecificId specific_id) const
+    -> TypeId {
+  if (!base_id.is_valid()) {
+    return TypeId::Invalid;
+  }
+  if (base_id == SemIR::InstId::BuiltinErrorInst) {
+    return TypeId::Error;
+  }
+  CARBON_CHECK(base_id.index >= 0);
+  return TypeId::ForTypeConstant(GetConstantValueInSpecific(
+      file, specific_id,
+      file.insts().GetAs<BaseDecl>(base_id).base_type_inst_id));
+}


Looking at the rename of adapted_type_inst_id, had you considered renaming base_type_inst_id and adapted_type_inst_id to something like extended_type_inst_id? It looks like adding AnyExtendedDecl (or a template on AdaptDecl/BaseDecl) would allow code sharing here:

Suggested change

auto Class::GetAdaptedType(const File& file, SpecificId specific_id) const

-> TypeId {

if (!adapt_id.is_valid()) {

return TypeId::Invalid;

}

if (base_id == SemIR::InstId::BuiltinErrorInst) {

return TypeId::Error;

}

return TypeId::ForTypeConstant(GetConstantValueInSpecific(

file, specific_id,

file.insts().GetAs<AdaptDecl>(adapt_id).adapted_type_inst_id));

}

auto Class::GetBaseType(const File& file, SpecificId specific_id) const

-> TypeId {

if (!base_id.is_valid()) {

return TypeId::Invalid;

}

if (base_id == SemIR::InstId::BuiltinErrorInst) {

return TypeId::Error;

}

CARBON_CHECK(base_id.index >= 0);

return TypeId::ForTypeConstant(GetConstantValueInSpecific(

file, specific_id,

file.insts().GetAs<BaseDecl>(base_id).base_type_inst_id));

}

// Returns the extended type for an `adapt` or `base` declaration.

static auto GetExtendedType(const File& file, SpecificId specific_id, InstId extended_id) -> TypeId {

if (!extended_id.is_valid()) {

return TypeId::Invalid;

}

if (extended_id == SemIR::InstId::BuiltinErrorInst) {

return TypeId::Error;

}

return TypeId::ForTypeConstant(GetConstantValueInSpecific(

file, specific_id,

file.insts().GetAs<AnyExtendedDecl>(extended_id).extended_type_inst_id));

}

auto Class::GetAdaptedType(const File& file, SpecificId specific_id) const

-> TypeId {

return GetExtendedType(file, specific_id, adapt_id);

}

auto Class::GetBaseType(const File& file, SpecificId specific_id) const

-> TypeId {

return GetExtendedType(file, specific_id, base_id);

}

Seems like a nice idea. The name doesn't work for me, though -- an adapted type need not be extended (you can use adapt without extend), and I think there's a reasonable chance we'll decide at some point we want to allow base without extend too. Will take to #naming.

Done, went with "foundation declaration" for base type or adapted type.

toolchain/check/handle_class.cpp

jonmeow · 2024-11-22T16:45:46Z

toolchain/check/import_ref.cpp

+    auto base_type_const_id = GetLocalConstantId(
+        import_ir_.constant_values().Get(inst.base_type_inst_id));
    if (HasNewWork()) {
      return Retry();
    }

-    // Import the instruction in order to update contained base_type_id and
-    // track the import location.
+    auto base_type_inst_id = AddLoadedImportRef(
+        context_, {.ir_id = import_ir_id_, .inst_id = inst.base_type_inst_id},
+        SemIR::TypeId::TypeType, base_type_const_id);


Is it worth trying to share logic with AdaptDecl, maybe with a template? It looks like this section mirrors.

Of course, maybe the answer is just "that's not how we do it elsewhere" or "think about it as part of the other refactoring"

Let's think about it as part of the other refactoring :-)

jonmeow · 2024-11-22T16:47:59Z

toolchain/check/member_access.cpp

+    if (auto base_type_id = self_class_info.GetBaseType(
+            context.sem_ir(), self_class_type->specific_id);
+        base_type_id.is_valid()) {
+      if (context.types().GetConstantId(base_type_id) == name_scope_const_id) {
        return SemIR::AccessKind::Protected;
      }
-    } else if (auto adapt_decl =
-                   context.insts().TryGetAsIfValid<SemIR::AdaptDecl>(
-                       self_class_info.adapt_id)) {
-      if (adapt_decl->adapted_type_id == class_info.self_type_id) {
+      // TODO: Also check whether this base class has a base class of its own.
+    } else if (auto adapt_type_id = self_class_info.GetAdaptedType(
+                   context.sem_ir(), self_class_type->specific_id);
+               adapt_type_id.is_valid()) {


You've written similar in a couple spots (e.g. context.cpp), where the same logic is done on the extended type. Would it be worth having a function like GetExtendedType that returns base or adapt?

I plan to send a follow-up PR that removes the adapter handling here and in DiagnoseInvalidQualifiedNameAccess, because it doesn't match the design (see #4560), which I think would not leave much use of this facility.

jonmeow · 2024-11-22T16:49:49Z

toolchain/lower/constant.cpp

@@ -239,6 +209,24 @@ static auto EmitAsConstant(ConstantContext& /*context*/,
  CARBON_FATAL("TODO: Add support: {0}", inst);
 }

+template <typename InstT>


Maybe add a comment explaining this...

Though I'll note this strays a little from the original change (as does TypeStore). Some of this feels like it would've been nicer split out, but I'm basically done reviewing now. :-P

Yeah, sorry about that. (I started with the TypeStore refactoring and ended up cleaning up too much other stuff "in passing", ending up with this mostly being a change to base / adapt declarations instead.) :(

Comment added.

Co-authored-by: Jon Ross-Perkins <[email protected]>

chandlerc

Nice! Comments from previous review round seem addressed. One optional one below, but LG if you'd like to do that in a follow-up.

chandlerc · 2024-11-26T02:25:44Z

toolchain/sem_ir/type.cpp

+auto TypeStore::IsSignedInt(TypeId int_type_id) const -> bool {
+  auto object_repr_id = GetObjectRepr(int_type_id);
+  if (!object_repr_id.is_valid()) {
+    return false;
+  }
+  auto inst_id = GetInstId(int_type_id);
+  if (inst_id == InstId::BuiltinIntLiteralType) {
+    return true;
+  }
+  auto int_type = file_->insts().TryGetAs<IntType>(inst_id);
+  return int_type && int_type->int_kind.is_signed();
+}


Now that this GetIntTypeInfo is a method here, do you want to implement this in terms of it?

Happy for this to be a follow-up PR if that's easier.

I don't think this is completely straightforward, given that IsSignedInt returns false on non-integer types whereas GetIntTypeInfo CHECK-fails, so I'm going to look at this in a follow-up PR.

geoffromer · 2024-11-25T22:26:59Z

toolchain/sem_ir/typed_insts.h


  // No type_id; this is not a value.
-  TypeId adapted_type_id;
+  InstId adapted_type_inst_id;


Could this be foundation_type_inst_id? Given the amount of implicit magic in this header, especially with respect to Any types, I think it would be helpful to be as unambiguous as possible about the intended correspondence of the fields. I think it could also help readability, by acting as an illustration of the meaning of "foundation type".

Hm, I see your point. On the other hand, all producers and consumers of this field (and the field on BaseDecl) know that they're using this specific kind of declaration, and we use the term "base type" and "adapted type" elsewhere to refer to this value.

The pattern we use elsewhere for Any types is to have generic field names in the Any type and more specific field names in the corresponding specific Insts -- for example, AnyAggregateAccess::aggregate_id corresponds to StructAccess::struct_id, TupleAccess::tuple_id, ClassElementAccess::base_id (not a great name), and InterfaceWitnessAccess::witness_id. I think the usages of those fields would be made less clear by using aggregate_id in all cases.

Perhaps a comment linking the insts in a category back to any corresponding category would work better? In any case, I think this goes beyond this PR, so I'd suggest we discuss it separately. Will start a discussion in #toolchain.

Sure, resolving this separately sounds good.

toolchain/check/testdata/class/generic/adapt.carbon

geoffromer · 2024-11-27T20:20:27Z

toolchain/sem_ir/typed_insts.h


  // No type_id; this is not a value.
-  TypeId adapted_type_id;
+  InstId adapted_type_inst_id;


Sure, resolving this separately sounds good.

toolchain/check/testdata/class/generic/adapt.carbon

…uage#4564) Represent the type as an `InstId` rather than as a `TypeId` to preserve how it was written and better support tracking its value in a generic. Add accessors to `Class` to get the base and adapted type to reduce code duplication, and add `TypeStore::GetObjectRepr` to make it easier to map from a type to its possibly-adapted object representation type. In passing, also move `GetIntTypeInfo` and `GetUnqualifiedType` into `TypeStore`. This fixes specifics of generic adapters to properly look at the specific adapted type, and also fixes importing of adapters. --------- Co-authored-by: Jon Ross-Perkins <[email protected]>

zygoloid added 5 commits November 21, 2024 21:40

Allow classes whose object representation is an integer type to be us…

0296bb5

…ed with integer tyye builtins.

Support generic adapters, bases and import better.

cbe5eb7

Support for generic adapters.

a066c41

Fix importing of adapters

2c1c938

clang-format

ecd787e

github-actions bot requested a review from geoffromer November 21, 2024 22:06

github-actions bot added the toolchain label Nov 21, 2024

jonmeow reviewed Nov 22, 2024

View reviewed changes

zygoloid and others added 3 commits November 23, 2024 00:08

Improve comments.

79c0d6b

Simplify.

d2abf71

Co-authored-by: Jon Ross-Perkins <[email protected]>

Factor out shared handling of adapt and base declarations.

9bb62e9

chandlerc self-requested a review November 26, 2024 02:19

chandlerc approved these changes Nov 26, 2024

View reviewed changes

geoffromer reviewed Nov 26, 2024

View reviewed changes

zygoloid added 2 commits November 26, 2024 20:04

Fix test.

ce28fbf

Merge branch 'trunk' into toolchain-refactor-typestore

705f201

zygoloid requested a review from geoffromer November 27, 2024 01:27

geoffromer approved these changes Nov 27, 2024

View reviewed changes

Rename file and add TODO comment describing the intent.

8bdc0ea

zygoloid enabled auto-merge November 27, 2024 21:41

zygoloid added 2 commits November 27, 2024 22:13

Merge branch 'trunk' into toolchain-refactor-typestore

114f8fb

Autoupdate.

a838158

zygoloid added this pull request to the merge queue Nov 27, 2024

Merged via the queue into carbon-language:trunk with commit d6ec885 Nov 27, 2024
8 checks passed

zygoloid deleted the toolchain-refactor-typestore branch November 27, 2024 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track the type as written in `BaseDecl` and `AdaptDecl`. #4564

Track the type as written in `BaseDecl` and `AdaptDecl`. #4564

zygoloid commented Nov 21, 2024

jonmeow Nov 22, 2024

zygoloid Nov 22, 2024

zygoloid Nov 25, 2024

jonmeow Nov 22, 2024

zygoloid Nov 22, 2024

jonmeow Nov 22, 2024

zygoloid Nov 23, 2024

jonmeow Nov 22, 2024

zygoloid Nov 23, 2024

chandlerc left a comment

chandlerc Nov 26, 2024

zygoloid Nov 26, 2024

geoffromer Nov 25, 2024

zygoloid Nov 26, 2024

geoffromer Nov 27, 2024

geoffromer Nov 27, 2024

Track the type as written in BaseDecl and AdaptDecl. #4564

Track the type as written in BaseDecl and AdaptDecl. #4564

Conversation

zygoloid commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Track the type as written in `BaseDecl` and `AdaptDecl`. #4564

Track the type as written in `BaseDecl` and `AdaptDecl`. #4564