Support builtin conversions of adapter classes #4655
Conversation
danakj
force-pushed
the
tuple-adapt
branch
2 times, most recently
from
103bdb3 to
c2d0cb3
Compare
|
This is ready for review, in case it was missed |
toolchain/check/convert.cpp
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think GetFoundationType is also unclear -- we're using "foundation declaration" to mean "base or adapt declaration", so I'd expect GetFoundationType to return the immediate base class or adapted type, whereas this transitively looks through adapters and does not look through bases. Conversely, we use "compatible" to mean "transitively related through adapters".
If you want to avoid the negation, maybe GetCanonicalCompatibleType or GetTransitiveAdaptedType or GetIndirectlyAdaptedType?
| } | ||
|
|
||
| fn F(a: A) { | ||
| let a_value: A = (a as (i32, Noncopyable)) as A; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Trying to clarify: you can have fail_ and non-fail_ splits in the same on-disk file. The fail_ split will fail (even after autoupdate) if it doesn't produce errors, and conversely the non-fail_ split will fail (even after autoupdate) if it does produce errors. It doesn't matter what is in the other splits in the same on-disk file or whether they're fail_ / non-fail_.
What we try to avoid is putting expected-to-pass tests in a fail_ split, because if they start producing errors we might not notice. (It's also desirable for the same reason to have only one error per fail_ split, so that the test won't still pass if one of the errors goes away.)
danakj
changed the title
toolchain/check/convert.cpp
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| fn F(a: A) { | ||
| let a_value: A = (a as (i32, Noncopyable)) as A; |
There was a problem hiding this comment.
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.
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.
| // the type (or some part of it) is not copyable. | ||
| DiagnosticAnnotationScope annotate_diagnostics( | ||
| &context.emitter(), [&](auto& builder) { | ||
| CARBON_DIAGNOSTIC(InCopy, Note, "in copy of `{0}`", TypeOfInstId); |
There was a problem hiding this comment.
| CARBON_DIAGNOSTIC(InCopy, Note, "in copy of `{0}`", TypeOfInstId); | |
| CARBON_DIAGNOSTIC(InCopy, Note, "in copy of {0}", TypeOfInstId); |
The diagnostic machinery adds the `s for you.
| } | ||
|
|
||
| fn F(a: A) { | ||
| let a_value: A = (a as (i32, Noncopyable)) as A; |
There was a problem hiding this comment.
Trying to clarify: you can have fail_ and non-fail_ splits in the same on-disk file. The fail_ split will fail (even after autoupdate) if it doesn't produce errors, and conversely the non-fail_ split will fail (even after autoupdate) if it does produce errors. It doesn't matter what is in the other splits in the same on-disk file or whether they're fail_ / non-fail_.
What we try to avoid is putting expected-to-pass tests in a fail_ split, because if they start producing errors we might not notice. (It's also desirable for the same reason to have only one error per fail_ split, so that the test won't still pass if one of the errors goes away.)
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.