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.
When attempting to import a definition of a class with virtual bases,
diagnose the point of use instead of the point of definition of the
class.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
When importing a class definition, ask Clang to complete it first. This
causes class template specializations to get instantiated as needed when
the type-checking of Carbon requires a C++ class to be complete. It also
allows Clang to implement things like modules-aware definition
visibility checking.
Don't reject importing a class with a virtual base if it's never
required to be complete. Instead, defer diagnosing until the definition
is required.
This also removes the recursion from `MapType`, as mapping a class type
no longer maps its definition.
In order to get diagnostics from instantiation failures, fix a bug that
caused any Clang diagnostics produced after the initial building of the
`ASTUnit` to get discarded. This exposed some duplicate diagnostic
issues in `ImportNameFromCpp` which are fixed here too.
Previously we would drop these diagnostics; now periodically flush them
to Carbon's diagnostics emitter. We flush them at the end of checking,
and also immediately before changing the set of diagnostic annotation
scopes, so that Clang diagnostics get properly annotated.
This exposes some double diagnostics being produced in situations where
Clang's name lookup logic would produce a diagnostic and we also
produced one. For access control issues, use the Carbon diagnostic, in
order to properly handle protected access. For ambiguity issues, use the
Clang diagnostic that produces helpful notes.
Also fix rendering of note diagnostics produced by Clang, by attaching
them to the prior error / warning diagnostic.
---------
Co-authored-by: Boaz Brickner <brickner@google.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Add a new type, `custom_layout_type`, representing a struct type whose
size, alignment, and field offsets can be manually controlled. Use this
as the object representation type for imported C++ class types (which
also includes struct and union types), allowing us to model C++ class
type layouts. In passing, also add support for incomplete C++ class
types, mapping them into incomplete Carbon class types.
Map C++ fields into Carbon field declarations, allowing direct access to
C++ fields from Carbon. So far, no support is added for base classes nor
anonymous struct or union declarations; those will be added in
subsequent PRs. Also, we don't map C++ access control into Carbon yet,
so all C++ fields are accessible regardless of their access control.
For now we still use a `struct_type` as the object representation for
empty C++ classes, in order to continue to support our existing tests
that convert `{}` to empty C++ class types. This is temporary and should
be removed once we support interop with C++ class initialization.
Expand support for `int` and `short` to cover all the other `intN_t` and
`uintN_t` types too. We achieve this by asking Clang what the `intN_t` /
`uintN_t` type that it would use for the given bitwidth is, and checking
if that's the type we're trying to map.
Change impls from `<interface>.impl` to `<self>.as.<interface>.impl`,
and *member* functions to `<parent scope>.<fn>` (non-member functions
exclude their parent scope). Stop special-casing builtin functions,
given the new naming scheme.
The purpose of this is to make it clearer when a member function is
being accessed and, if so, which member function. In particular, we
often access interface `Op` functions. The builtin function
special-casing was intended to help with that, but we still have lots of
`Op` functions. This particular approach should make the interactions
clearer.
This changes up queueing of block IDs a little because, in particular,
we need to process bodies of entities only after constants finish
processing. But, it should also result in less memory usage during
processing because it means we have less on the insts stack at any given
time, since we track a block rather than all instructions contained by
the block.
This switches from the `CollectNamesInBlock` approach for entities, to
instead traversing entities as they're encountered. For example, when
traversing constants, when a type is found, the entity will have its
block queued for processing.
This leads to a change in the traversal order, which affects
disambiguation done by numeric sequencing (since that's just showing the
traversal order).
This will allow for simpler "name based on name" logic. This is
something I plan to use for:
- impls: `<type>.as.<interface>.impl`
- functions: `<entity>.<member function>`
- Note an impl may be used as the entity for a bound function.
By naming the entities as they're encountered, I'll be able to rely on
the generated names rather than recalculating them.
To assist this, I'm also differentiating between the ambiguous and
disambiguated name. Otherwise, we could end up with things like
`<function>.<disambiguator>.<call>.<other disambiguator>`, where the
repeated disambiguator may not be necessary in order to get full
disambiguation. It's also a smaller delta from the current output.
Note, changing `Name` to a class felt appropriate given its shape. I was
also noticing that parts of its API were unused, and the class helps
detect unused private members.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Use `full.carbon` min-prelude for any tests using the full prelude. A
few tests were using it unnecessarily and are changed to a more minimal
one in the process.
Any test which does not include some min-prelude will now fail with an
error.
Before this change we crash in this case when trying to use the outer
type.
After this change we diagnose a TODO.
Will add the missing support for this in a separate PR.
Part of #5533.
For example, this would allow using `std::string::size`, which is
templated because it's a member of the class template
`std::basic_string`, but isn't itself a function template.
This is instead of having one test file that has different return types
and two test files for `i16` and `i32` param tests, which doesn't seem
consistent.
First commit does file renames for easier review.
Part of #5063.
Added test coverage to demonstrate name lookup following import of a
type of a function parameter.
SemIR changes show that the same decl isn't imported multiple times.
Part of #5533.
Create a `self` parameter when importing a C++ non-static member
function. That seems to be all we need to get basic method calls
working! For now, `const` methods have by-value self parameters, and
non-`const` methods get an `addr self: Self*` parameter.
Incidentally also supports import of pointers-to-pointers.
Imported const-qualified types aren't especially useful just yet,
because on the Carbon side we don't yet permit conversions from
non-const to const types, so most of the tests still fail, but for
different reasons now.
In `BuildUnaryOperator`, `GetOperatorOpFunction` is treated as
desugaring, but `PerformCompoundMemberAccess` and `PerformCall` are not.
This treats all of destruction as desugaring.
This leads to some instructions being elided, because of `GetOrAddInst`
behaviors:
> // If the instruction has a desugared location and a constant value,
returns
> // the constant value's instruction ID. Otherwise, same as AddInst.
This changes instructions that previously had a non-desugared location
to instead have a desugared location, so if they also have a constant
value then the constant value can be used directly.
Unify code paths for importing classes by name and importing them
indirectly when their type is referenced. Switch to using the general
type import machinery to import all type declarations, which allows
typedef declarations naming importable types to be used too.
Fix up handling of error cases to consistently only produce an Error
InstId after actually producing an error message, so that we can produce
exactly one diagnostic in failure cases.
Remove TODO error for unions that previously was only produced when
importing them indirectly, not when importing them by name. Import of
unions is exactly as complete / incomplete as import of other class
types, so treating them differently doesn't seem necessary.
Stop using the clang tooling library to build an ASTUnit; that library
is set up to process clang frontend arguments, assuming that something
has already built frontend arguments from the compiler arguments. It is
also too encapsulated and doesn't let us inspect and modify the compiler
invocation before it's executed.
Instead, build the AST unit directly in two phases:
* FIrst, take a list of clang driver arguments and convert them into a
list of compiler arguments, using `clang::createInvocation`. Internally,
this uses the clang driver to build a frontend invocation, including
building system-specific include paths as needed.
* Then, directly build an ASTUnit from this compiler invocation.
I've factored this so that we can split out the `createInvocation` step,
with the intention that we may want to move it out of check and into the
carbon driver with the rest of the driver-level argument handling, and
we may want to customize some of the clang options before we invoke the
clang frontend with that set of options.
In order to make the invocation reusable, it no longer depends on the
name of the carbon file importing the C++ code. In place of synthesizing
a header file name as `<foo.carbon>.generated.cpp_imports.h`, we now
insert line marker directives into the generated header so that errors
in that header cause Clang to point a diagnostic back at the Carbon
source file itself. This results in a minor improvement in the
diagnostic output: we no longer refer to a nonexistent generated file.
But the snippet still contains text that doesn't match the source code,
so it remains imperfect.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
When diagnosing a problem with C++ code imported from Carbon, include
the location of the specific `import Cpp` statement that imported the
C++ code as part of the backtrace, rather than providing a location in a
generated file that doesn't exist on disk.
Also fix handling in autoupdate of check lines that contain multiple
file name and line number pairs to use the matched file name for
remapping of locations rather than the first file name in the line.
When mapping parameter types, we assume that if the `clang::Decl` isn't
mapped, the name wasn't added, so this fixes a bug that triggers a crash
otherwise.
Part of #5533.
Currently if the first operand contains an error, we will return error,
even though the second operands contains a runtime, and it has a
stronger priority (the phase always goes up if possible).
Import is only allowed on instructions with compile-time values, so we
crash if we ever try to import a runtime value. Importable instructions
must diagnose unexpected runtime values and produce errors in the semir
from which they would be imported so that runtime values are never
imported by another semir.
If we had an instruction where you had an error value from the first
operand, and runtime from the second, and we imported it:
- Before https://github.com/carbon-language/carbon-lang/pull/5728 we
would crash in import, but only because we treated errors as runtime
- After https://github.com/carbon-language/carbon-lang/pull/5728 we
would import ErrorInst because we propagate errors. This is desirable
for cases with compile-time values and errors present only.
- After this PR, we would crash again, cuz you're importing a runtime
thing.
This change means that instructions containing an
`InstConstantKind::Never` instruction like`ValueParam` will consistently
evaluate to a runtime value, even if there are errors present. This is
visible in the `BindName` instructions changing in the semir, where they
became constant `ErrorInst` values previously but no longer do.
This changes `Destroy` to use an interface for its implementation.
Note that this change includes a lot of test updates. Even when
`Destroy` is a no-op, it still causes code generation as part of
determining that.
Originally I was trying to use ranges to cut down the scope of this, and
to a degree I think they have. But a flipside here is that cases where
no destructors should be generated -- particularly globals -- would be
needed to completely remove destructor calls. Even for ranges, the range
can often include the destructor placement. So I've shifted
frame-of-thought a little: accept a bunch of destructor churn, because
destructors are needed and will be prevalent. The verbosity is a feature
of the design to make desugaring apparent in IR, not a bug.
This doesn't support actually passing the value of the struct, which is
planned to be implemented using thunks.
`ClangDeclId` value is now `ClangDecl` which includes the mapped Carbon
instruction in addition to the Clang declaration. This allows finding
the Carbon instruction for a given Clang declaration, which is necessary
for mapping a Clang struct parameter type to the Carbon class without
doing name lookup. We don't take the instruction as part of the hash
key, as discussed in
[Discord](https://discord.com/channels/655572317891461132/768530752592805919/1380575881050718469).
To map the type, we also need to map namespaces. To avoid recursion for
inner namespaces, we use a vector.
Note that the first commit just changes the order of functions in the
file to make review easier.
C++ Interop Demo (that shows missing behavior):
```c++
// hello_world.h
struct S {
S(const S&) { x = 1; }
int x;
};
void hello_world(S s);
```
```c++
// hello_world.cpp
#include "hello_world.h"
#include <cstdio>
void hello_world2(S s) { printf("hello_world2: %d\n", s.x); }
void hello_world(S s) {
printf("hello_world: %d\n", s.x);
hello_world2(s);
}
```
```carbon
// main.carbon
library "Main";
import Cpp library "hello_world.h";
fn Run() -> i32 {
var s : Cpp.S;
Cpp.hello_world(s);
return 0;
}
```
```shell
$ clang -c hello_world.cpp
$ bazel-bin/toolchain/carbon compile main.carbon
$ bazel-bin/toolchain/carbon link hello_world.o main.o --output=demo
$ ./demo
hello_world: -1108224096
hello_world2: 1
```
Part of #5533.
The none.carbon min-prelude is not just an empty prelude, it also
prevents any prelude from being imported at all. So no import machinery
runs before the test, only the `package` statement from the prelude
would run.
Use the none.carbon min-prelude in a few tests that were specifying
`--no-prelude-import` to give it a trial run.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Follow up of #5594.
Trying to compromise SemIR size, having enough information and
complexity of tests, I've duplicated representative tests to a separate
test file with `--dump-sem-ir-ranges=if-present`.
Adds min-preludes more tests which were seen as slow and their
surrounding neighbours. This drops the file_test runtime on my machine
from about 7s to about 6s.
For a few files that are clearly only testing diagnostics, we drop the
if-present semir ranges and the associated TODO.
When the binding pattern appears within a `var` pattern, convert to a
reference. Otherwise, convert to a value.
This gets the advent of code examples to produce the right answers again
:)
---------
Co-authored-by: Geoff Romer <gromer@google.com>
import_use_generic.carbon has the comment "// We're just checking that
this doesn't crash. It's not expected to compile." Because it involves
import behavior by name, I'm not touching it. Other than that, while
maybe it's better to test with less, the `ImplicitAs` errors at best
feel difficult to understand, and at worst could be masking an issue.
Boaz Brickner
Richard Smith
Dana Jansens