Outside of `match_first` this adds diagnostics for invalid non-final and
final `impl` declarations in line with those being proposed in
https://github.com/carbon-language/carbon-lang/pull/5337.
- Two non-final `impl`s with the exact same type structure is invalid.
- A `final impl` that matches the self/constraint of another `impl` as a
query would always be preferred, making the second one invalid.
- Two `final impl`s that overlap (have compatible type structures) in
different files is invalid.
- Two `final impl`s that overlap (have compatible type structures) in
the same file is invalid outside of `match_first`.
- A `final impl` in a different file from its root self type and
interface is invalid.
We add tests for all these scenarios as well as correct scenarios.
The "compatible" test for two type structures was being done
symmetrically, which is incorrect. We want it to test that a query type
structure is the same _or more specific_ in a compatible way with an
impl's type structure. This is corrected in the implementation, and the
diagnostics now have to test both directions to get the desired output,
as expected.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
This requires:
* Making `FunctionDecl` mutable since generating code
(`HandleTopLevelDecl()`) requires a mutable declaration and since we
manually add `used` attribute to force code generation.
* Passing the file system to `Lower` since it's needed by Clang code
generation.
* Creating an internal Clang LLVM module and link it against the Carbon
LLVM module.
Demo:
```c++
// hello_world.h
extern int puts;
inline void hello_world() {
((int (*)(const char*))&puts)("hello world");
}
```
```carbon
// main.carbon
library "Main";
import Cpp library "hello_world.h";
fn Run() -> i32 {
Cpp.hello_world();
return 0;
}
```
```shell
$ bazel-bin/toolchain/carbon compile main.carbon
$ bazel-bin/toolchain/carbon link main.o --output=demo
$ ./demo
hello world
```
Based on https://github.com/carbon-language/carbon-lang/pull/5406.
Part of #5405.
Such impls will never be used, so they should not exist. And test that a
final impl partially overlapping a non-final impl is accepted.
There is a question about a final impl partially overlapping a final
impl that is part of
https://github.com/carbon-language/carbon-lang/pull/5337
The canonical location of the instruction may be an entirely different
instruction, which the instruction in question was not imported from. In
particular, we shouldn't assume that we can use the constant value of an
instruction that the *location* of an imported instruction refers to as
the constant value of the imported instruction.
The only time we should be looking at the `ImportIRInstId` for a `LocId`
is when determining its location in some other file.
Fixes a crash when importing thunks (which can contain instructions
whose location points to an instruction in a differnt IR).
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
When two non-final impls have the same type structure (neither is a
specialization of the other), it is an error unless they are within a
`match_first` block. For now, we don't have `match_first` implemented,
so it's always an error.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Right now we construct `tree_and_subtrees_getters` a couple different
ways, it's just not obvious because one's abstracted in `check`. But
also, when formatting IR, we'll repeatedly do the `IncludeInDumps`
string check, which felt odd to me since it only needs to be calculated
once per IR.
This also shifts `CheckIRId` selection a little earlier, and in doing so
makes `CheckParseTrees` accept a sparse `units` argument. I actually
think this is a positive: it makes `CheckIRId` a little more stable
across possible command lines, when file loading fails (which is the
only time that a file will have a `CompilationUnit` but not a
`Check::Unit`).
Trying to build on the shared issue between these, I'm adding a
`MultiUnitCache` to store the calculated arrays. For the subtree
getters, this is very minor and avoids at most one incremental array
construction (moving logic out of `CompileSubcommand::Run` might be the
bigger benefit). For `include_in_dumps`, when dumping SemIR, this is
changing a calculation run once per entity (in each IR) to be calculated
once per IR (globally), i.e. O(M*N) -> O(N).
Note this seems to be marginal for performance of file_test:
- Before: Stats over 10 runs: max = 5.3s, min = 4.7s, avg = 4.9s, dev =
0.2s
- After: Stats over 10 runs: max = 4.9s, min = 4.7s, avg = 4.8s, dev =
0.1s
I was mainly thinking about this in the context of dumping SemIR ranges.
There, the impact may actually decrease because a range won't do any
cross-IR printing. But, I'm expecting to add another layer for whether
we're printing IR for a file, and that made the `should_format_entity`
callback stick out for me.
Once a concrete result has been found, it's not legal to write an `impl`
that would change the concrete result afterward.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Remove calls to `InstStore::GetLocId()` to build a LocId from an InstId
now that they can be constructed directly from the InstId. Most uses of
LocId are just plumbing, so this does not affect them. However places
that want to look inside the LocId do not want to work with the InstId
form. In these places, introduce `InstStore::GetResolvedLocId()` which
converts a LocId (or an InstId as an optimization) into a LocId which is
not backed by an InstId. These locations can be printed (they have a
line and column when they are a NodeId), they can have flags added to
them (`ToImplicit`, `ToTokenOnly`), they can be converted to an
underlying ImportIRInstId, or they may be `None`.
`Dump()` is made to print a resolved location instead of printing the
InstId in the location, since (at least in my experience) the resolved
location is what is interesting in debugging, and this saves manual
`MakeInstId` steps in the debugger every time a location is of interest.
The LocId constructor from InstId is made `explicit` to add clarity to
function calls passing an `inst_id` now directly instead of calling
`context.insts().GetLocId(inst_id)`. To avoid needing to construct
`SemIR::LocId(...)` explicitly in all cases though, the diagnostics code
in Check uses `DiagnosticLocId` as its template parameter which accepts
InstId as well and does the construction of LocId from it.
Because LocId now requires an explicit construction from InstId, any
callers to `AddInst()` functions will have to explicitly convert to
LocId if they had an InstId, but not if they pass a NodeId. To make this
difference clear to callers, we `requires` that the input type can be
converted to LocId. This ensures that passing an InstId results in an
error at the callsite where the InstId is passed, instead of generating
a compiler error when trying to construct `LocIdAndInst` inside
`AddInst()`, which is less clear about what went wrong and doesn't seem
entirely intentional.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Right now, return_scope_stack is being used to determine whether logic
is in a function scope. However, we need to handle nested entities
inside function scopes. For example where this crashes right now:
```
base class C(B:! bool) {}
fn F() {
class B {
extend base: C(true or false);
}
}
```
This is doing a few things to make this kind of code not crash:
- Split `scope_stack().Push` into `PushForDeclName`, `PushForEntity`,
`PushForExpr`, and `PushForFunction` so that better decisions can be
made about behaviors.
- Hide `return_scope_stack` in the API, instead using interfaces to get
at the underlying data.
- Also using `PushForFunction` to update it similar to the other stacks
that `ScopeStack` manages.
- Add `IsInFunctionScope` as the best way to determine presence in
function scope.
- Remove `PeekIsLexicalScope` since destruction really wants function
scope information anyways.
- Clean up `destroy_id_stack` handling to be for function scopes rather
than lexical scopes.
- Return after related `context.TODO`s in a couple more spots, so that
code doesn't proceed to add control flow in spite of the lack of
support.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This fake generic was used for two reasons:
- The declaration name stack assumes that each declaration name is
processed within a generic scope. This is important if the name might
have generic parameters, which are always parsed even for declarations
that disallow them in check.
- Out-of-line redeclarations of generic entities produce instructions
with symbolic constant values in non-generic scopes.
The former case is addressed by pushing a generic each time we start a
declaration name, even if we will reject generic parameters later. The
latter case is worked around for now by not building a symbolic constant
type or value for instructions that appear outside of any generic, and
will be addressed more completely by #5310 and follow-ups.
These constant instructions are all TypeInstId already in their type,
and this makes their names match.
Change the name of MakeSingletonInstId as well and update its comment.
We frequently want to operate on singletons. Per discussion, drop
`Singleton` to make the code shorter.
This started off as wanting to write `inst_id.is_error()`, but the
dependency relationship between ids.h and singleton_insts.h would
require some kind of delayed evaluation to allow the implementation to
remain in headers (which I suspect is helpful to have for inlining). I
could have added something like `IsErrorInst`, forward declared in ids.h
and defined in singleton_insts.h (which would always be included by
typed_insts.h), but the template approach felt like a decent balance
between (a) removing the boilerplate `::SingletonInstId`, (b)
understandability, (c) still visually mirroring if we immediately return
a singleton, and (d) flexibility for more than just `ErrorInst`. But TBH
I'd probably still have written `is_error()` if it didn't require
addressing the cross-header cycle.
Then I tried `SemIR::InstId::Is<SemIR::ErrorInst>`, which generally
worked with types but generated the complaint that it didn't shorten
*all* singleton uses. So pulling back on `::Is`, and instead just
dropping `Singleton`.
The main goal of this is to collapse the LocId and SemIRLoc types into a
single type, eliminating the need for APIs to decide which to use. This
originated from discussion about UnwrapSemIRLoc in #5169. Although that
was removed in #5202, it's probably still a good direction for LocId.
This changes the packing of LocId to allow adding InstId, making it
tri-modal: ImportIRInstId, InstId, or NodeId. This has a side-effect of
reducing the available space for ImportIRInstId, although not by much
due to the pre-existing `ImplicitBit` behavior. If needed, we could also
probably play with packing a bit more since `ImplicitBit` really only
applies to `NodeId`, but I was trying to keep the logic a little
simpler. Note `TokenOnlyBit` can still apply to `ImportIRInstId`.
This leaves in place a typedef for SemIRLoc -- I intend to clean that up
separately.
Some Discord discussion is
[here](https://discord.com/channels/655572317891461132/655578254970716160/1353755830058745959).
Emitting definitions in check. This resolves the crash in lowering which
necessitated definitions be emitted.
Some of the test changes need further review.
As of #5087, these terms are no longer synonyms. This change preserves
the original meaning.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
`AddDefaultValue` doesn't quite capture the intended semantics; it
should typically be replaced with an actual value when dealing with
control flows. Trying to indicate the "assign later" with
`AddPlaceholder`, mirroring `AddPlaceholderInst`.
Shifting the `protected` functionality on `BlockValueStore` so that it's
not providing functions just for `InstBlockStore` to use. Also hoping
that seeing the comments next to the function name makes them easier to
understand, whereas `using` buries that a little.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This doesn't change functionality, but I was seeing better diagnostics
in VS Code.
This also changes the NodeId constructors for related types (also
NodeCategory and NodeIdForKind) to use UnsafeMake for construction. That
originated from avoiding ambiguity coming from `requires`, but the
constructor mode is also one we should typically avoid (e.g., preferring
`Parse::Tree::As`).
ASTUnit is owned by `CompileSubcommand`, passed through `Unit` to be
populated in `ImportCppFiles()` and used via `SemIR::File`.
When generating the AST, pass `-x c++` args to compile C++ (temporary
until we pass args properly).
`Cpp` namespace is marked as a special namespace and has dedicated logic
in `LookupNameInExactScope()`.
The logic for importing declarations from C++ to Carbon is in
`import_cpp.cpp`, but we're likely to want to refactor this
signfiicantly over time as it grows (perhaps a dedicated directory?).
Part of #4666.
Right now, some post-run logic is does in `Run()`
(`CheckRequiredDefinitions();` and
`context_.sem_ir().set_has_errors(unit_and_imports_->err_tracker.seen_error());`)
whereas other parts are done by `Finalize`. Noting the goal to move
things off `Context`, this consolidates into a new `FinishRun`. Note
#4962 is adding another bit of post-run that can be consolidated in;
this seems likely to keep growing slowly.
Note this also creates more parity with mutation source, like the
`context_.scope_stack().Pop();` matches the push done by
`CheckUnit::ImportCurrentPackage` and
`context_.inst_block_stack().Pop()` was pushed in `CheckUnit::Run()`.
Also makes `exports()` more consistent with other Context APIs. Makes
`VerifyOnFinish` `const` so that it can't accidentally mutate state, and
is instead only validating that the Context is in its expected
configuration at completion.
Emit diagnostics for a function declared in a non-owning library, that
is not redeclared (or defined) in the owning library.
---------
Co-authored-by: jonmeow <jperkins@google.com>
This in particular uses free functions because it's likely to end up
more consistent with types (versus a wrapper object for InstStore).
Note, this is unlikely to have a performance impact, but if it does, we
can look into related approaches (and we've already discussed using
LTO).
Renames `PendingBlock::AddInst` to `PendingBlock::Add` because
`MakeElementAccessInst` expects the matching name to exist.
This creates a new check/type.h for most logic, and also moves some
functions to TypeStore in sem_ir/type.h. My approach for TypeStore is to
focus on moving the read-only functions there.
Change representation of package names from `IdentifierId` to
`PackageNameId`, and add a special value `PackageNameId::Core` for the
Core package. Add a `Core` expression to name the Core package, and
support for parsing the `Core` keyword in `package` and `import`
declarations.
For now, I've made no changes to instruction fingerprinting or name
mangling. This means that fingerprints and mangled names will collide
between names in the `Core` package and names in a `r#Core` package. See
#4908.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
At present, we typically define a DiagnosticConverter, then store an
instance of it and a DiagnosticEmitter that wraps it. This is relatively
minor in general, but I've been trying to create more self-contained
DiagnosticEmitter classes (which hold their own DiagnosticConverter,
similar to NullDiagnosticEmitter), and there it just gets in the way.
Since we don't reuse DiagnosticConverter instances, this combines the
definition into DiagnosticEmitter. Mainly this means we don't have a
separate object in play, and less to carry around.
The most impact is probably to SemIRDiagnosticConverter, which was also
the most complex. Now `SemIRLocDiagnosticEmitter`, this gets some
different construction flow. Note in the PR I've split the file rename
to its own commit, to try to help delta views. However, the most
substantial parts of the refactoring are split into #4876, which this
depends upon.
I'm looking at eliminating `DiagnosticConverter`. This change removes
`NodeLocConverter` (albeit adding `UnitAndImportsDiagnosticConverter`),
and in doing so, refactors lex conversion functions to extract them out
from the `DiagnosticConverter` functions.
I'll be following up with changes that collapse `DiagnosticConverter`
logic into `DiagnosticEmitter` locations. The intent is that we
shouldn't need separate ownership of both types.
This switches most error printing to use diagnostics instead of direct
stream writes, even when not a specific file diagnostic. I'm allowing
empty filenames for this use-case.
This allows a little more specific testing to validate coverage of
output using the diagnostic coverage test. I'm adding a few tests to
cover things that weren't previously tested.
Separately, this also forces a little more standardization in format...
considering how changes like #4568 show effort being spent to _mirror_
diagnostic style, my thought is now to just use diagnostic code where
possible.
Note this also allows incrementally better testing of the language
server; I'm changing the crash fix from #4847 in favor of diagnostic
testing.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This is a followup from #4834, I searched for "invalid" uses in our
codebase. This is mostly changing comments, and a couple debug
functions, but shouldn't affect testable behavior.
Note a couple things I'll highlight as not changing (but could) are:
- `ReturnTypeInfo::is_valid`
- `"invalid"` uses in the formatter
- `AddInvalid` for `!has_value` in `inst_fingerprinter` (because the
cases it's called sound invalid-ish)
High level, replacing `Id::Invalid` with `Id::None` and `Id::is_valid`
with `Id::has_value` for clarity, as discussed
[here](https://discord.com/channels/655572317891461132/655578254970716160/1331664574545395794).
The `IntId` refactoring is needed together with `AnyIdBase` because it's
also used with `ValueStore`.
Note, trying to be careful not to rewrite `EnumBase::InvalidIndex`, or
`is_valid` in general (e.g., `IdKind::is_valid`).
I've tried to sequence commits here:
1. Automatic replacements:
- `((?:Id|Index)(?: |::|\(|Base(?:\(|::)))Invalid((?:Index)?\W)` ->
`$1None$2`
- `<invalid>` -> `<none>`
- `InvalidNodeId` -> `NoneNodeId`
- `/\*invalid\*/` -> `/*none*/`
- `id((?:_|\(\))(?:\.|->))is_valid` -> `id$1has_value`
2. Manual edits:
- In `int.h` and `int_test.cpp`
- `IntT` has `is_value`, which I'm renaming to `is_embedded_value`.
- Manual edits to comments in this file.
- `AnyIdBase` and `IdBase`
- Declaration of `is_valid` -> `has_value`, `InvalidIndex` ->
`NoneIndex`.
- In `ids.h` and `ids.cpp`
- `is_valid` -> `has_value`
- `// An explicitly invalid ID.` -> `// An ID with no value.`; similar
for index
- Various math on `InvalidIndex` -> `NoneIndex`
- Various mentions of "valid" in comments
- In `value_store.h`, for `IdT::Invalid`, plus one comment
- In `impl.h` and `tokenized_buffer.h`, we had different initialization
of `::None` values (versus `ids.h` syntax) that I fixed manually.
- Spot checks to compile
- Particularly where `is_valid` replacements didn't catch spots due to
different naming.
3. Autoupdate tests
4. verbose.carbon (NOAUTOUPDATE)
5. Comment spot checks
Note there are probably other mentions of "Invalid" that should be swept
up, but I'd like to argue for merging and separating out remaining
cleanup since this is so sweeping (and likely to hit merge conflicts
from churn). We'll probably have lingering mentions of "invalid" for a
bit regardless, just because there are uses of "invalid" in non-Id APIs.
With this change, we now support impl of interfaces with non-function
associated constants.
Also:
* Make impl diagnostics use more consistent names
* Make some impl tests "no_prelude"
Still to do:
* Facet type resolution as a separate, reusable step
* Using the assigned values of associated constants (see
`fail_todo_use_assoc_const.carbon`)
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This change deliberately breaks away from the line/column ordering, and
instead focuses on a last byte offset corresponding to the final token
processed as part of producing the message. Where that's equal, this
maintains stable ordering in order to reflect the order that diagnostics
were produced.
The intent of this approach is that lex, parse, and check diagnostics
are interleaved based on where they are produced, but that
subexpressions still have diagnostics emitted prior to containing
expressions. In particular, the prior line/column sort essentially
sorted on the _start_ of where a diagnostic was associated, and this is
closer to sorting based on the _end_. As a consequence, something like
`F(1 2)` will have the error for `1 2` emitted _before_ a diagnostic for
`F(1 2)` not matching parameters, instead of _after_.
In check, we track the last handled node. This provides a
last_byte_offset _separate_ from where a diagnostic is associated. The
intent is that this creates an ordering of diagnostics which may be
associated with earlier code, to cause the diagnostics to be emitted
later. An example consequence of this is the change in ordering of
modifier diagnostics: we are diagnosing those from the same place, but
they have the same last_byte_offset, so we print them out in the order
produced.
I've added similar tracking to parse, but cannot identify any test which
is affected by it (note the separate commit, I thought about this late).
I'm not sure whether we have good out-of-order errors we could produce
for this.
A significant number of tests have reordered diagnostics as a
consequence of this change, so this change does not add further testing.
* Change `InterfaceWitness` -> `ImplWitness`
* Include a `SpecificId` in the `ImplWitness`. This allows the
`InstBlock` it contains to have its own identity, allowing it to be
changed as the impl is processed. Evaluation only updates the specific.
* Create the `ImplWitness` at the start of the impl definition. In the
future, this will be populated with the values of non-function
associated constants. For now, it starts full of invalid instruction
ids.
* Implements the model suggested in #4672 .
Note that the non-SemIR testdata changes are to these file:
* `toolchain/check/testdata/impl/lookup/fail_todo_undefined_impl.carbon`
* `toolchain/check/testdata/struct/import.carbon`
* `toolchain/check/testdata/tuple/import.carbon`
The last two are due to an import of generics bug exposed by this PR,
which will be fixed in a follow-on.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Also fix a bug in `Context::GetClassType` that previously tried to
complete the class type before returning it. That's not correct --
`GetCompleteTypeImpl` is only appropriate for cases where the type can
trivially be completed and completing it can't fail -- and led to
infinite recursion with this change because we would call `GetClassType`
when producing a diagnostic if completing that class type failed.
This is primarily moving code around, to try to create a logical split
of the code in check.cpp, makingthe API boundaries clearer.
There's one small, deliberate logic change around false returns from
`HandleParseNode`, where before there was a `CARBON_CHECK` instantiated
by the `#define` (per `NodeKind`), and now it's outside the `#define`
(done mainly because the message didn't keep up with the `Handle##Name`
-> `HandleParseNode` rename).
Thomas Köppe
Dana Jansensandjosh11b
Boaz Brickner
Richard SmithandJon Ross-Perkins
Alina Sbirlea
David Blaikie