When deducing arguments for generic parameters of an `impl`, the
deduction calls `Convert` on the input arguments. Often, the input
argument is a facet, and needs to be converted to a type via
FacetAccessType in order to produce a different facet. These
instructions end up being added to the semir, but only their constant
values are needed for the resulting specific returned from Deduce.
In the best case, these extra instructions are just noise in the semir,
or they just cause instruction names to get differentiated with larger
suffixes.
In the worst case, these extra instructions contain references to
instructions from a generic context, and leak them out of that generic
context and into another. In particular, when importing a
LookupImplWitness instruction, the re-evaluation of it can do deduce
(when the lookup is against a generic `impl`). The instructions created
in Deduce are not part of the import, and end up referring to imported
instructions from the local context, which leads to confusion in the
toolchain, and can crash.
The `import_self_specific.carbon` test demonstrates this. It causes the
`I.F` function to be imported from the `I` interface when building the
witness table for the `impl`. Doing so imports the specific of `C` which
includes a LookupImplWitness for `Self.Accoc` in `I`. The `Self` is a
BindSymbolicName with generic binding index 0, in `I`. When Convert
creates instructions in the generic `impl forall D`, however, they end
up referencing and including this BindSymbolicName into its eval block.
But the generic binding 0 in the `impl` is a very different thing (a
value of type `E`). This confusion leads to crashes.
The SymbolicBindingType refers to the type value that will be
substituted in for the BindSymbolicName, but holds onto the EntityNameId
from the BindSymbolicName instead of (or in addition to, for now) the
instruction.
The EntityNameId will be used to look in the ScopeStack to find the
witnesses either from the BindSymbolicName instruction, or other
instructions that specify `impls` constraints against the EntityName.
This will allow us to have the `T` in `I(T)` resolve to a `.Self`
reference in the type so that we get type equality with the binding's
type: `T:! I(.Self)`.
After this change, we correctly increment the offset by the next switch
case type.
Before this change, we accidentally incremented the offset by
`functions()` size instead of `cpp_overload_sets()` size and vice versa.
Also sorted the switch cases according to the order of the enum, for
consistency. This might help prevent a future similar incident.
This fix prevents crashing in the newly introduced test
`multiple_too_few_args_calls`.
This also has the side effect of showing `null name` for
`cpp_overload_set_type` and `cpp_overload_set_value`, instead of having
an arbitrary name.
Examples that demonstrate the old name is arbitrary can easily be seen
in tests like `cpp_namespace.carbon` and `decayed_param.carbon`, but
careful review would show that all old names are arbitrary, though often
luckily almost make sense.
We might want to have a proper name for these, but it's beyond the scope
of this crash fixing change.
See #6156.
Part of #5915.
This also does a little restructuring in the same direction, following
#6124.
Leads want `Destroy` to work similarly now for all types. As a
consequence, there doesn't seem to be as much benefit to splitting off
aggregate destruction. In this PR, the `type.destroy` function can now
be expected to destroy anything that's destructible; that means it'll be
usable for the `final fn` once that support is available.
Similarly, this gets rid of the impls other than the single blanket
impl, now using `type.can_destroy`. Since they all need to use the same
function, there's no benefit to splitting approaches. Also, now it can
just be a `final impl` since there should be no need for people to
create specializations -- if this blanket impl applies, it means the
`final fn` is the same.
This also slips in `partial` support since there's no reason to have it
diverge anymore. Also `abstract`, which I'm not sure is broadly testable
since most cases it'd come up, the `abstract` keyword is explicitly
detected/rejected.
Note though that this doesn't make any really big changes. It's just
realigning on the leads decision. I'm going this way to try to reduce
name-related churn for other changes.
This turns out to be quite important, as several important standard
library types (such as `std::string`) have mixed-access overload sets
for their constructors as an implementation detail. The overall approach
here is:
- Use the most permissive access to determine the access of the overload
set itself. This affects whether name lookup finds the member name at
all.
- After overload resolution, re-check the access of the selected member,
if it's protected or private.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
As proposed in [Carbon: C++ interop for overloaded functions and
function
templates](https://docs.google.com/document/d/1KUxumZtNe3mY3TsjW2s_ZADOlAaFlrtsLKHVILtqIaM/edit?tab=t.0),
Clang is used to perform the overload resolution using C++ rules, when
an overloaded C++ set is called from Carbon. Once a function is
selected, it's converted into a Carbon function and called using the
Carbon rules including argument conversions.
A single non-templated function is treated the same way as an overload
set and the same rules apply for its call.
Template functions are not supported yet.
Demo:
a) Non-templated function calls:
```c++
// --- overloads.h
auto foo(int a, short b) -> void;
auto foo(double a) -> void;
auto foo(int a) -> void;
```
```c++
// overloads.cpp
#include "overloads.h"
#include <cstdio>
auto foo(int a, short b) -> void {
printf("hello from foo_int_short(%d, %d) \n", a, b);
}
auto foo(double a) -> void { printf("hello from foo_double(%f) \n", a); }
auto foo(int a) -> void { printf("hello from foo_int(%d) \n", a); }
```
```c++
library "Main";
import Cpp library "overloads.h";
fn Run() -> i32 {
Cpp.foo(1.1 as f64);
return 0;
}
```
```
$ clang -c overloads.cpp
$ bazel-bin/toolchain/carbon compile main.carbon
$ bazel-bin/toolchain/carbon link overloads.o main.o --output=demo
$ ./demo
hello from foo_double(1.100000)
```
b) Constructors:
```c++
// --- constructor_overloads.h
class C {
public:
C();
C(int a, int b);
};
```
```c++
// constructor_overloads.cpp
#include "constructor_overloads.h"
#include <cstdio>
C::C() { printf("hello from C() \n"); }
C::C(int a, int b) { printf("hello from C(%d, %d) \n", a, b); }
```
```c++
library "Main";
import Cpp library "constructor_overloads.h";
fn Run() -> i32 {
let c1: Cpp.C = Cpp.C.C();
let c2: Cpp.C = Cpp.C.C(1, 2);
return 0;
}
```
```
$ clang -c constructor_overloads.cpp
$ bazel-bin/toolchain/carbon compile main.carbon
$ bazel-bin/toolchain/carbon link constructor_overloads.o main.o \--output=demo
$ ./demo
hello from C()
hello from C(1, 2)
```
Follow-ups:
- `Cpp.foo({})` - proper handling of struct literals as call args.
- Fix access for overloaded sets.
- Fix tests:
- Method calls: `error: missing object argument in method call
[MissingObjectInMethodCall]` in tests.
- Fix `toolchain/check/testdata/interop/cpp/import.carbon` test.
- Fix `enums` support.
- Fix `str` -> `std::string_view` mapping.
Part of #5915
Instead of taking the complete text of the Clang diagnostic and using it
as the message portion of a Carbon diagnostic, generate the individual
pieces separately and pass them into the Carbon diagnostic
infrastructure.
* Clang's context lines are generated by running a custom "diagnostic
renderer" and tracking which lines it wants to print as context for a
given source location. When mapping from a C++ source location back to a
Carbon location, the Carbon `Loc` structure is now fully populated,
including filling in the context line and the column number.
* Clang's snippet is generated by running a custom diagnostic renderer
that is a cut-down version of the full text diagnostic renderer that
only prints a snippet. This is then attached to the Carbon diagnostic
manually as an override for the snippet we'd usually create.
We no longer repeat the file location twice on each diagnostic, and no
longer produce a bogus "in import" line for all locations coming from
clang that point arbitrarily to the first C++ import in the Carbon file.
The `[diagnostic kind]` marker is now displayed at the end of the
diagnostic message, not on a line of its own after the snippet.
Dana Jansens
Boaz Brickner
Ivana Ivanovska
Jon Ross-Perkins
Richard SmithandDana Jansens