For an expression such as `(Type as Interface).AssocFn()`, track the
`Self` type `Type` in the result of the member access so that it's
available when checking the function call.
This introduces a new kind of type, `ImplFunctionType`, that represents
the type of a function that is expected within an impl, modeled as the
type of the function within the interface plus a value to use as `Self`.
Calls to values of this type behave like calls to the underlying
function except that the `Self` parameter is pre-bound to the self type
from the facet.
In order to support this, fix an issue where the imported list of
generic bindings lost their association with their enclosing generic.
This adds a little complexity to `import_ref`, including a new recursive
cycle that I intend to address in a follow-up PR.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
### Context & Motivation
The error handling utilities in `//base/error.h` are very useful for
writing code with strong safety guarantees. While hardening the `Dump`
debug utilities (from review in #4866), I encountered a rough edge with
references and pointers. After a [brief Discord discussion in
#contributing-help](https://discord.com/channels/655572317891461132/1052653651895779359/1334675462877610038),
it was suggested that adding support for references to `ErrorOr` would
be a good candidate to move forward.
Using a reference type with the `ErrorOr` class (e.g. `ErrorOr<Node&>`)
produces two errors:
<ol>
<li><strong><code>variant can not have a reference type as an
alternative</code></strong>
<ul><li>From private field: <code>std::variant<Error, T>
val_;</code></li></ul>
</li>
<li><strong><code>'operator->' declared as a pointer to a
reference</code></strong>
<ul><li>From member function: <code>auto operator->() ->
T*</code></li></ul>
</li>
</ol>
### Changes
To support reference types, both errors are resolved:
1. `std::reference_wrapper` is conditionally used for storage when `T`
is a reference type
2. type trait aliases like `using ValueT = std::remove_reference_t<T>`
are used to produce compatible types for methods like `auto operator->()
-> ValueT*`
- The actual reason I started this: minor lowering updates in the golden
LLVM IR
- Process.inc changed enough to need a patch context update.
- https://github.com/llvm/llvm-project/pull/123126 added `proto_library`
uses without a `load`, which is broken in bazel 8
- Just commenting these out because we don't use them. I'll follow up
separately about a possible fix, but continuing to use `WORKSPACE` is a
bigger issue LLVM probably should address.
- Note this update is also triggering removal of `migrate_cpp`, in #4887
Although migration tooling is intended for Carbon, this tooling was
primarily a proof-of-concept prototype. The last time it got significant
work was in 2022
(https://github.com/carbon-language/carbon-lang/commits/trunk/migrate_cpp);
since then, we've mainly been doing small cleanups to keep it building.
We're now hitting an issue in #4886 that `TypeNodes.inc` isn't exposed
as a `#include`.
We already had thoughts about rewriting this as a `RecursiveASTVisitor`,
per `rewriter.cpp`/`rewriter.h`. That may justify a significantly
different approach than had been set up in `cpp_refactoring`. But, it's
hard to tell.
Either way, my sense is that rather than incrementally trying to keep
this code building, we should revisit it when we're ready and have a
long-term strategy for how migration should work.
Not sure if I mixed up a merge somewhere, but this is what
`parse_error.carbon` was supposed to test. Note
`unexpected_reply.carbon` was basically doing the same test, with a
minor issue of `change` vs `result` to get better output.
Introduce a principle that the Carbon language should not encroach on
the
developer's namespace. Satisfy this principle by making `Core` a
keyword.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
If there are no explicit params, then `last_param_node_id` should be
`implicit_params_loc_id`. But there is no use case currently, so for now
we assume explicit params exist and assert if not, so this can be found
and updated then.
I'm planning on eliminating DiagnosticConverter. As part of this,
collapse it into diagnostic_emitter.h, and refactoring the header a
little so that it's more readable when making changes.
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>
I think the name of `SemIRLoc` might be leading to a few suggestions to
use it for non-diagnostic purposes that I've been responding to. At the
same time, a short name seems desirable given its frequency of use for
diagnostics. I did also clean up misuse in #4857, and eval.cpp (noted
below) might be similar. We don't typically use `friend` to emphasize
relationships, but given the diagnostic-specific intent and ways it's
been used, perhaps it's reasonable to close the API of this type using
`friend`?
eval.cpp inspects contents, but it's not clear that's needed because
changing logic doesn't affect tests, so I'm just removing it. Note that
SemIRLoc could've also been a loc_id, and it seems like the current
approach would mishandle that (i.e., print for `LocId::None` when that
seems not to be the intent). I figure we can add a `has_value` or
`AddNoteRequiringLoc` or something like that if needed.
Symbolic constants are negative values (starting at -3 at the moment)
but instead of having to figure the correct integer value for
MakeConstantId, provide a function to make a symbolic constant directly.
Based on [the lastest thinking on
#4672](https://github.com/carbon-language/carbon-lang/issues/4672#issuecomment-2606209281)
, require a full syntactic match for impl redeclaration, instead of
excluding the `where` restriction. This means no updates to the impl
witness on redeclaration, and no diagnostics that those updates are
consistent.
Not included in this PR, but will need to be done in the future:
* Support for assigning values to associated constants in the body of
the impl definition. This will require moving the checking that
non-function associated constants are set from the definition start to
definition end.
* Identify semantic redeclarations that are not syntactic matches to
give a failed redeclaration diagnostic. This should be done once we are
already identifying impl declarations with the same type structure in
order to require they be identified in an impl_priority/match_first
block.
* Merging of the functions in `check/impl.cpp` that are now always
called together.
Also add some test coverage of `where` parsing I developed in PR I've
now abandoned because of this new simplification of the impl
redeclaration semantics.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Benefits:
* Provide a proper API for accessing lookup information.
* Make assumptions on whether the result is poisoned or not and how we
can use `InstId` explicit.
* Allow safely reusing the `InstId` value for pointing to the poisoning
entity for poisoned results (in a future PR).
* Consolidate `LookupNameInExactScopeResult`, `std::pair<SemIR::InstId,
bool>` and part of `LookupResult`.
Part of #4622.
Add a full entity representation for associated constants, and build a
`Generic` object for them. This `Generic` is parameterized by the
enclosing `Self` type, allowing the use of `Self` within the type of the
associated constant to be supported.
When performing impl lookup for an associated constant, produce the type
with the provided self type substituted for its `Self` along with any
generic parameters of the interface.
Split the handling of associated constant declarations into two parts,
corresponding to the code before the `=`, and the code between the `=`
and `;` (if any). The former goes into the generic declaration region;
the latter into the generic definition region. This prepares us to
handle the default value for an associated constant, but for now we're
just storing the information and not actually using it.
Remove the entity type field from `assoc_entity_type`, because it's
almost unused and is an attractive nuisance -- it must necessarily be a
type in the generic scope of the associated constant rather than in the
scope of the instruction (because there is no `Self` anywhere else),
which means that it's hard to substitute into or derive meaning from.
See `toolchain/check/testdata/impl/assoc_const_self.carbon` for tests of
the new functionality; these used to cause the toolchain to crash.
- Escape dumped token strings (what got me here)
- Change the quoting from backticks to quotes
- Also add a `FormatEscaped` helper function for this, updating other
`.write_escaped` uses
Switching from RE2 to StrReplaceAll because it seems a fair fit for what
actually needs to be done here. Also pick up \t for visibility reasons.
This came up because clangd's LSP-related APIs print carriage returns.
`toolchain/autoupdate_testdata.py --allow-check-fail` can now be used to
perform an autoupdate even if some `CARBON_CHECK`s are failing. What
this does will depend on how the toolchain behaves after the `CHECK`
failure, and of course there's no guarantees there, but this can be
useful if it's easier to debug the `CHECK` failure by looking at the
produced SemIR.
Internally, this uses `bazel build --config=non-fatal-checks`, which in
turn specifies a `--per_file_copt` for `check_internal.cpp`. The intent
here is that the rebuild required to enable or disable this mode is as
small as reasonably possible.
This mode is not compatible with `-c opt`, as it's important that check
failure calls are `[[noreturn]]` in `-c opt` mode.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Came up on #4831, style:
"For a type T whose values can be compared for equality, define a
non-member operator== and document when two values of type T are
considered equal."
https://google.github.io/styleguide/cppguide.html#Operator_Overloading
Note while we could put some of these out-of-line, it's helpful to keep
them inside the braces:
- For private member access
- For templated cases so that we aren't duplicating templates
- Very mild preference for keeping class's API documented within the
braces
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.
Changes the name of SemIR `import_ref`s to use the format
`<package>.<entity>`.
<table>
<tr><th>Before</th><th>After</th></tr>
<tr>
<td><code>%import_ref.05a: type</code></td>
<td><code>%Main.D: type</code></td>
</tr>
<tr>
<td><code>%import_ref.8f2: <witness></code></td>
<td><code>%Main.import_ref.8f2: <witness></code></td>
</tr>
</table>
* [Discord discussion in
#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1330253540999827577)
* Closes#4769
In order to write language-server tests, we need some way to pass stdin
input. This adds support for a split "// --- STDIN" which will be
provided as a temp file for testing.
Note this does more stdin -> input_stream style renaming, this is just
bugging me more since I know shadowing works but it can be subtle to
read, particularly since I'm now making direct use of stdin in a handful
of spots.
The language server needs stdin, and for tests we should be passing it
around. My intent is to pass in a faux stdin to Driver for language
server tests.
As long as I'm adding a new parameter, I was looking at also changing
the way streams are passed in to Driver for style (pointers since
they're held past construction lifetime). Since these are all stored in
DriverEnv, I thought it might be a net improvement to use the struct
directly, getting more explicit parameter names and also removing the
need for `SetFuzzing`.
I'm trying here to avoid functional changes, but there are a couple
additional fixes like removing an obsolete `find_insensitive` and
refactoring how `ValidateOptions` handles errors (because it reduces the
number of spots that operate on error_stream).
I believe `check_syntax` is already controlling the semantic vs
syntactic merge, added in #4149. Other parts of the TODO are clarified
per discussion. But this is tested, e.g. errors with the bool flipped:
```
impl i32 as I {
+ // CHECK:STDERR: method.carbon:[[@LINE+6]]:14: error: redeclaration syntax di
ffers here [RedeclParamSyntaxDiffers]
+ // CHECK:STDERR: fn F[self: i32](other: i32) -> i32 = "int.sadd";
+ // CHECK:STDERR: ^~~
+ // CHECK:STDERR: method.carbon:[[@LINE-7]]:14: note: comparing with previous
declaration here [RedeclParamSyntaxPrevious]
+ // CHECK:STDERR: fn F[self: Self](other: Self) -> Self;
+ // CHECK:STDERR: ^~~~
fn F[self: i32](other: i32) -> i32 = "int.sadd";
}
```
Richard SmithandJon Ross-Perkins
Calvin
Boaz Brickner
Alina Sbirlea
Dana Jansens
Geoff Romer
dependabot[bot]