Support builtin conversions of adapter classes #4655

danakj · 2024-12-09T18:28:06Z

When creating a tuple or struct type object/value, we will walk each of
the tuple's or struct's parts, respectively, and Convert() each of them.
This allows (T, T) to convert to (U, U) and so forth. It also performs
the conversion from value to initialization even for the same types,
such as converting from a value of (T, T) to an object of (T, T).

Classes need to define their own conversions but when the target and
source types are the same, there is no conversion of types taking place.
If the class adapts a tuple or struct then walk each of the tuple's or
struct's parts, respectively, and Convert() each of them in order to
initialize the parts of the target.

For copyable types, the conversion implies a copy in the initialization,
and for non-copyable types, an error is emitted.

This supports copying a class value to a class object when it is an
adapter of a tuple or struct.

danakj · 2024-12-09T18:29:28Z

toolchain/check/convert.cpp

+      auto converted_value_id =
+          PerformBuiltinConversion(context, loc_id, foundation_value_id,
+                                   {
+                                       .kind = target.kind,


The construction of this ConversionTarget is the most questionable to me. Should it use the same kind, init_id and init_block? If the init_block is used, should the AsCompatible be put into the block?

If we have an init_id and init_block here, the init_id should refer to the destination object that we are initializing. So I think we need to apply a conversion to init_id, and the logic ends up being:

(Always) Convert the source to the adapted type with an AsCompatible.

If there's an init_id, create an AsCompatible in the init_block to convert the destination to the adapted type.

Perform the builtin conversion.

Convert the resulting expression back to the adapter type with an AsCompatible.

It seems a bit weird that we need three AsCompatible conversions in some cases, but in SemIR we model an initializing expression as (sometimes) taking the location to be initialized and (always) producing an "initializing value" of the destination type, so weirdly we do end up with three things to convert.

It took me a while to really understand everything that was said here and why, but I think I got it, and the semir looks better now with this change.

toolchain/check/convert.cpp

zygoloid · 2024-12-10T01:54:44Z

toolchain/check/convert.cpp

+      auto converted_value_id =
+          PerformBuiltinConversion(context, loc_id, foundation_value_id,
+                                   {
+                                       .kind = target.kind,


If we have an init_id and init_block here, the init_id should refer to the destination object that we are initializing. So I think we need to apply a conversion to init_id, and the logic ends up being:

(Always) Convert the source to the adapted type with an AsCompatible.

If there's an init_id, create an AsCompatible in the init_block to convert the destination to the adapted type.

Perform the builtin conversion.

Convert the resulting expression back to the adapter type with an AsCompatible.

It seems a bit weird that we need three AsCompatible conversions in some cases, but in SemIR we model an initializing expression as (sometimes) taking the location to be initialized and (always) producing an "initializing value" of the destination type, so weirdly we do end up with three things to convert.

toolchain/check/convert.cpp

toolchain/check/testdata/class/adapter/adapt_copy.carbon

danakj · 2024-12-16T13:58:10Z

This is ready for review, in case it was missed

zygoloid

Thanks, some trivial things but this looks good. I'm happy for the note I suggested to be added as a separate PR (approving on that basis), or as part of this PR; your choice.

toolchain/check/convert.cpp

zygoloid · 2024-12-17T00:11:46Z

toolchain/check/convert.cpp

@@ -707,7 +707,7 @@ static auto CanUseValueOfInitializer(const SemIR::File& sem_ir,
 }

 // Returns the non-adapter type that is compatible with the specified type.
-static auto GetCompatibleBaseType(Context& context, SemIR::TypeId type_id)
+static auto GetCompatibleFoundationType(Context& context, SemIR::TypeId type_id)


I wonder if GetCompatibleNonAdapterType would actually work better here -- "compatible" is already supposed to imply that we're looking at adapters (an adapter and its adapted type are compatible with each other).

If you feel like its currently redundant, I would prefer GetFoundationType over "CompatibleNonAdapter". The negation there feels more ambiguous - many things may be non-adapters, though the compatible narrows it down. Maybe you're trying to highlight that it chases through the chain of adapters, though I would say foundation does at least as good a job of highlighting that. Maybe it's also that "adapter" and "adapted" are very close linguistically and make me pause and think about the relationship (kind of like how definition and declaration still do today...) in a way foundation does not.

zygoloid · 2024-12-17T00:21:27Z

toolchain/check/testdata/as/adapter_conversion.carbon

+}
+
+fn F(a: A) {
+  let a_value: A = (a as (i32, Noncopyable)) as A;


We usually put tests that are expected to pass in different file splits from ones that are expected to fail. That way we get extra assurance that we don't have regressions after autoupdate -- if an expected-to-pass test starts to fail, the test will fail even after an autoupdate run.

Ok, thanks. I will split off the fail_init_class.carbon above as well.

FWIW I tried making a non-fail_ test in this file fail, ran autoupdate, and then bazel test and it still fails that expected-to-pass test regardless of the presence of a fail_ test in the same file or not. Not sure if that's unexpected then.

toolchain/check/testdata/class/adapter/adapt_copy.carbon

danakj

PTAL

danakj · 2024-12-23T14:48:38Z

toolchain/check/convert.cpp

@@ -707,7 +707,7 @@ static auto CanUseValueOfInitializer(const SemIR::File& sem_ir,
 }

 // Returns the non-adapter type that is compatible with the specified type.
-static auto GetCompatibleBaseType(Context& context, SemIR::TypeId type_id)
+static auto GetCompatibleFoundationType(Context& context, SemIR::TypeId type_id)


If you feel like its currently redundant, I would prefer GetFoundationType over "CompatibleNonAdapter". The negation there feels more ambiguous - many things may be non-adapters, though the compatible narrows it down. Maybe you're trying to highlight that it chases through the chain of adapters, though I would say foundation does at least as good a job of highlighting that. Maybe it's also that "adapter" and "adapted" are very close linguistically and make me pause and think about the relationship (kind of like how definition and declaration still do today...) in a way foundation does not.

danakj · 2024-12-23T21:09:03Z

toolchain/check/testdata/as/adapter_conversion.carbon

+}
+
+fn F(a: A) {
+  let a_value: A = (a as (i32, Noncopyable)) as A;


Ok, thanks. I will split off the fail_init_class.carbon above as well.

FWIW I tried making a non-fail_ test in this file fail, ran autoupdate, and then bazel test and it still fails that expected-to-pass test regardless of the presence of a fail_ test in the same file or not. Not sure if that's unexpected then.

toolchain/check/testdata/class/adapter/adapt_copy.carbon

When creating a tuple or struct type object/value, we will walk each of the tuple's or struct's parts, respectively, and Convert() each of them. This allows (T, T) to convert to (U, U) and so forth. It also performs the conversion from value to initialization even for the same types, such as converting from a value of (T, T) to an object of (T, T). Classes need to define their own conversions but when the target and source types are the same, there is no conversion of types taking place. If the class adapts a tuple or struct then walk each of the tuple's or struct's parts, respectively, and Convert() each of them in order to initialize the parts of the target. For copyable types, the conversion implies a copy in the initialization, and for non-copyable types, an error is emitted. This supports copying a class value to a class object when it is an adapter of a tuple or struct.

Co-authored-by: Richard Smith <richard@metafoo.co.uk>

danakj requested a review from zygoloid December 9, 2024 18:28

github-actions bot added the toolchain label Dec 9, 2024

github-actions bot requested a review from geoffromer December 9, 2024 18:28

danakj commented Dec 9, 2024

View reviewed changes

danakj removed the request for review from geoffromer December 9, 2024 18:29

danakj force-pushed the tuple-adapt branch from e09de35 to ba81bed Compare December 9, 2024 18:33

danakj commented Dec 9, 2024

View reviewed changes

toolchain/check/convert.cpp Outdated Show resolved Hide resolved

danakj force-pushed the tuple-adapt branch from ba81bed to 64bb4c7 Compare December 9, 2024 18:38

zygoloid reviewed Dec 10, 2024

View reviewed changes

danakj commented Dec 12, 2024

View reviewed changes

toolchain/check/testdata/class/adapter/adapt_copy.carbon Show resolved Hide resolved

danakj requested a review from zygoloid December 12, 2024 22:12

danakj force-pushed the tuple-adapt branch 2 times, most recently from 103bdb3 to c2d0cb3 Compare December 12, 2024 22:21

zygoloid approved these changes Dec 17, 2024

View reviewed changes

danakj changed the title ~~Support builtin conversions of adaptor classes~~ Support builtin conversions of adapter classes Dec 23, 2024

danakj commented Dec 23, 2024

View reviewed changes

danakj and others added 13 commits December 23, 2024 16:48

rebase

81d3482

adds-conversion-to-init_block

7070bbb

only recurse on adapted tuple and struct

684aaa4

rename-back-to-simpler-intuple

7590124

cleanup

04379f2

comment

684d548

Merge remote-tracking branch 'origin/trunk' into tuple-adapt

0f7e5c3

Merge remote-tracking branch 'origin/trunk' into tuple-adapt

52f694b

GetFoundationType

184b061

rather

01e008d

Co-authored-by: Richard Smith <richard@metafoo.co.uk>

move-to-fail-file

b51e1ba

in-copy-note

db4b066

format

7802852

danakj force-pushed the tuple-adapt branch from f872ad2 to 7802852 Compare December 23, 2024 21:50

danakj requested a review from zygoloid December 23, 2024 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support builtin conversions of adapter classes #4655

Support builtin conversions of adapter classes #4655

danakj commented Dec 9, 2024 •

edited

Loading

danakj Dec 9, 2024

zygoloid Dec 10, 2024

danakj Dec 12, 2024

zygoloid Dec 10, 2024

danakj commented Dec 16, 2024

zygoloid left a comment

zygoloid Dec 17, 2024

danakj Dec 23, 2024 •

edited

Loading

zygoloid Dec 17, 2024

danakj Dec 23, 2024

danakj left a comment

danakj Dec 23, 2024 •

edited

Loading

danakj Dec 23, 2024

Support builtin conversions of adapter classes #4655

Are you sure you want to change the base?

Support builtin conversions of adapter classes #4655

Conversation

danakj commented Dec 9, 2024 • edited Loading

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

danakj commented Dec 16, 2024

zygoloid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danakj Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danakj left a comment

Choose a reason for hiding this comment

danakj Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danakj commented Dec 9, 2024 •

edited

Loading

danakj Dec 23, 2024 •

edited

Loading

danakj Dec 23, 2024 •

edited

Loading