```
class C {
impl as I;
}
```
is redeclared
```
impl C.(as I)
```
for purposes of `match_first`/`impl_priority` blocks and definitions.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
`IRBuilderBase::SetInsertPoint` weirdly replaces our debug location with
one copied from the new insertion point, so undo its damage after
calling it.
Also included: a couple of cleanups I made while tracking this down.
Ran into this trying to debug #5428
Before:
```
lex.cpp:793:21: runtime error: null pointer passed as argument 1, which is declared to never be null
string.h:90:51: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lex.cpp:793:21
```
After:
```
lex.cpp:793:21: runtime error: null pointer passed as argument 1, which is declared to never be null
string.h:90:51: note: nonnull attribute specified here
#0 0x5572db22a680 in Carbon::Lex::Lexer::MakeLines(llvm::StringRef) /proc/self/cwd/toolchain/lex/lex.cpp:793:14
#1 0x5572db227ee4 in Lex /proc/self/cwd/toolchain/lex/lex.cpp:738:3
(etc)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lex.cpp:793:21
```
Interestingly, even though this is labelled as UB, it's using the
ASAN_SYMBOLIZER_PATH (specifically not LLVM_SYMBOLIZER_PATH). But
canonically UBSAN_SYMBOLIZER_PATH may also be used per
https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/ubsan/ubsan_flags.cpp#L53,
so I'm adding it to the list out of an excess of caution.
Also doing some small related cleanup:
- Removing `ASAN_SYMBOLIZER_PATH` from `--test_env` because it should
now be getting overridden by these settings (also was a little
inconsistent in that `LLVM_SYMBOLIZER_PATH` was not included).
- Improving the environment construction and documentation.
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>
A final impl must be written in the same file as the root self type or
the interface. This provides tests that should fail but don't yet for
writing a final impl in a third file that defines neither, as well as a
blanket final impl over an interface outside the file that defines the
interface.
Fixes a compile failure with the new version, essentially:
```
external/+llvm_project+llvm-project/llvm/include/llvm/Support/FormatVariadicDetails.h:157:1: error: implicit instantiation of undefined template 'llvm::support::detail::missing_format_adapter<clang::LookupResultKind>'
```
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.
When we fill the witness table with errors, set the witness id to an
error too, which signals to impl lookups to not use the impl.
Make the use of the `Impl` from the store more consistent once it's been
added to the store (or known to be there already).
This consolidates Lex::TokenizedBuffer::DumpSemIRRange and
Parse::TreeAndSubtrees::TokenRange into a single InclusiveTokenRange,
also making the OverlapsWithDumpSemIRRange function take the new struct.
I considered switching to `llvm::iterator_range<Lex::TokenIterator>`,
but we often want to see if the range is size one. Using `TokenIterator`
just looked like it'd add a bunch of offsetting to make it work; I view
that as low-value overhead.
For example:
```
Lex::InclusiveTokenRange token_range = GetSubtreeTokenRange(node_id);
auto begin_loc = tree_->tokens().TokenToDiagnosticLoc(token_range.begin);
if (token_range.begin == token_range.end) {
return begin_loc;
}
auto end_loc = tree_->tokens().TokenToDiagnosticLoc(token_range.end);
```
would become:
```
llvm::iterator_range<Lex::TokenIterator> token_range = GetSubtreeTokenRange(node_id);
auto begin_loc = tree_->tokens().TokenToDiagnosticLoc(*token_range.begin());
if (token_range.begin() + 1 == token_range.end()) {
return begin_loc;
}
auto end_loc = tree_->tokens().TokenToDiagnosticLoc(*(token_range.end() - 1));
```
So I'm keeping the bespoke struct.
- Adds parent information as a single-pass calculation
- Moves off `GetSubtreeTokenRange` because it's O(N)
- Stops using the node's subtree when formatting a single instruction
- This excludes `%F.call` and the following `return`, for example,
because only parameters are marked for formatting
When checking whether we can use the function in an impl directly to
satisfy a signature in an interface, allow the parameter names to differ
between the two declarations.
Instead of building an eval block as a separate pass at the end of a
generic, build the eval block incrementally.
The larger change here is that asking for the type or constant value of
an instruction now always returns an unattached type or constant value,
in order to preserve the behavior that we previously achieved by doing
the rewrite to attached types and constant values at the end of handling
the generic.
This also incidentally fixes some subtle issues where attached types and
constant values would leak out into check and cause it to get confused
about differences between attached and unattached values. Check should
no longer see attached values except where it explicitly asks for them.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Don't import `ImplWitnessTable` into the `constants` block, because we
generally don't put `Unique` constants there. This matches the handling
of the other kinds of `Unique` constants. In order to keep the
instruction visible in formatted SemIR, add it to the `imports` block
instead.
Also fix a bug in the instruction formatter that resulted in
instructions in the `imports` block being omitted from the output if
they were only referenced by earlier instructions in the `imports` block
and by instructions in the `constants` block. This was already resulting
in some referenced instructions being omitted from the output, but also
occurred frequently for `impl_witness_table` instructions after this
change because it is common for the only reference to those instructions
to be from `impl_witness` instructions in the `constants` block.
I was thinking about this while working on parent generation on #5394
(which does a similar loop), it's just a simplification.
Also remove surplus spaces in the preorder print.
This prints instructions that are inside the range, and entities that
overlap with the range. Note this can lead to incomplete printing of
entity contents.
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>
Trying to improve resilience against failures such as:
```
INFO: Repository rules_jvm_external+ instantiated at:
<builtin>: in <toplevel>
Repository rule http_archive defined at:
/home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl:392:31: in <toplevel>
ERROR: /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl:137:45: An error occurred during the fetch of repository 'rules_jvm_external+':
Traceback (most recent call last):
File "/home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl", line 137, column 45, in _http_archive_impl
download_info = ctx.download_and_extract(
```
https://github.com/carbon-language/carbon-lang/actions/runs/14719625211/job/41311145495?pr=5379
It looks like GitHub currently has a high rate of these, which it
shouldn't, but also maybe we can do a little more to weather these
service issues.
To show flag behavior:
```
╚╡./scripts/run_bazel.py --attempts=5 --retry-all-errors :foo
Command ':foo' not found. Try 'bazel help'.
Retrying exit code 2 because it may be transient...
Command ':foo' not found. Try 'bazel help'.
Retrying exit code 2 because it may be transient...
Command ':foo' not found. Try 'bazel help'.
Retrying exit code 2 because it may be transient...
Command ':foo' not found. Try 'bazel help'.
Retrying exit code 2 because it may be transient...
Command ':foo' not found. Try 'bazel help'.
╚╡./scripts/run_bazel.py --attempts=5 --retry-all-errors query //... | wc -l
INFO: Invocation ID: ffdf9480-0245-442d-885f-ee91a3d86b68
Loading: 0 packages loaded
367
╚╡./scripts/run_bazel.py --attempts=5 :foo
Command ':foo' not found. Try 'bazel help'.
```
On the last run, [test (ubuntu-22.04,
opt)](https://github.com/carbon-language/carbon-lang/actions/runs/14737410033/job/41366888473?pr=5386)
has an example of this working:
```
INFO: Invocation ID: 3f276297-6dc5-4007-a333-dcadd4db55f4
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
WARNING: Download from https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz failed: class java.io.IOException GET returned 618 jwt:jwt-not-provided
INFO: Repository bazel_skylib+ instantiated at:
<builtin>: in <toplevel>
Repository rule http_archive defined at:
/home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl:392:31: in <toplevel>
ERROR: /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl:137:45: An error occurred during the fetch of repository 'bazel_skylib+':
Traceback (most recent call last):
File "/home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl", line 137, column 45, in _http_archive_impl
download_info = ctx.download_and_extract(
Error in download_and_extract: java.io.IOException: Error downloading [https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz] to /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_skylib+/temp62784[869](https://github.com/carbon-language/carbon-lang/actions/runs/14737410033/job/41366888473?pr=5386#step:5:887)09382546624/bazel-skylib-1.7.1.tar.gz: GET returned 618 jwt:jwt-not-provided
no actions running
no actions running
ERROR: Error loading '@@rules_python+//python/extensions:python.bzl' for module extensions, requested by /home/runner/work/carbon-lang/carbon-lang/MODULE.bazel:147:23: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_python+/python/extensions/python.bzl:48:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_python+/python/private/python.bzl:17:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_features+/features.bzl:3:6: Encountered error while reading extension file 'globals.bzl': no such package '@@bazel_features++version_extension+bazel_features_globals//': no such package '@@bazel_skylib+//lib': java.io.IOException: Error downloading [https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz] to /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_skylib+/temp6278486909382546624/bazel-skylib-1.7.1.tar.gz: GET re
ERROR: Error loading '@@rules_cc+//cc:extensions.bzl' for module extensions, requested by https://bcr.bazel.build/modules/rules_cc/0.1.1/MODULE.bazel:12:29: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_cc+/cc/extensions.bzl:16:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_features+/features.bzl:3:6: Encountered error while reading extension file 'globals.bzl': no such package '@@bazel_features++version_extension+bazel_features_globals//': no such package '@@bazel_skylib+//lib': java.io.IOException: Error downloading [https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz] to /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_skylib+/temp6278486909382546624/bazel-skylib-1.7.1.tar.gz: GET returned 618 jwt:jwt-not-provided: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_cc+/cc/extensions.bzl:16:6: at /h
ERROR: Error loading '@@rules_python+//python/extensions:python.bzl' for module extensions, requested by /home/runner/work/carbon-lang/carbon-lang/MODULE.bazel:147:23: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_python+/python/extensions/python.bzl:48:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_python+/python/private/python.bzl:17:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_features+/features.bzl:3:6: Encountered error while reading extension file 'globals.bzl': no such package '@@bazel_features++version_extension+bazel_features_globals//': no such package '@@bazel_skylib+//lib': java.io.IOException: Error downloading [https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz] to /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_skylib+/temp6278486909382546624/bazel-skylib-1.7.1.tar.gz: GET re
INFO: Invocation ID: e2062912-715f-47ec-a0bc-9d1b4fee9e5d
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
<root> (carbon@_)
Retrying a failure because it may be transient...
INFO: Invocation ID: 7d4ee250-882f-480d-9c8d-2d92e67462fc
Loading: 0 packages loaded
367
```
Trying to make it easier to see the API at a glance. The class has
become really long, and this doesn't fundamentally change that, but
hopefully makes it easier to navigate. The entry structure also had some
cruft that I'm removing.
I'm trying to keep functions in the same order as they currently are.
The delta still looks unhappy because of the churn, but hopefully this
at least explains the ordering in formatter.h. You can try using the
"Add indent" commit on the PR to see a better before-after delta.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
The location of the diagnostic is an instruction id, not a parse node.
Converting to a parse node to call ToTokenOnly will drop the descendents
of the instruction being diagnosed, but the diagnostic is about the
whole instruction not just the root parse node of whatever instruction
it happens to be.
This was originally needed to support constant evaluation of name
expressions, but that's now done in a different way.
This is actually a step toward treating all patterns as constants. The
upcoming change will do so in a slightly different way, and so it will
simplify the review to start from a baseline where patterns are never
constant.
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>
Trying to make it easier to skim test output for failures. Note this
excludes all the RUN and OK. Failing tests may still be verbose due to
the diff printed, but CHECK-fails should become short.
```
==================== Test output for //toolchain/testing:file_test:
Running tests with 128 thread(s)
...
Done!
[==========] 1272 tests from 1 test suite ran. (688 ms total)
[ PASSED ] 1272 tests.
================================================================================
```
Trying to make it easier to see possible bottlenecks.
Disabling hyperthreading seems like a significant reduction in
contention (15% improvement for me). Going down by half again reduces a
contention a little further, but not significantly from what I see. My
thought is that just flipping the flag is going to work best for people
cross-system versus a "divide by four", but welcome to other opinions
there. Note, I'm not digging into the source of the contention here,
just observing it.
Current default on my system (equivalent to `--threads=128`):
```
Running tests with 128 thread(s)
...
Ran 1272 tests in 3955 ms wall time, 397615 ms across threads
```
Disabling hyperthreads (equivalent to `--threads=64`):
```
Running tests with 64 thread(s)
...
Ran 1272 tests in 3520 ms wall time, 161957 ms across threads
```
`--threads=32`:
```
Running tests with 32 thread(s)
...
Ran 1272 tests in 3329 ms wall time, 69327 ms across threads
```
And for `./autoupdate_testdata.py --threads=64 --print_slowest_tests=5`:
```
Running tests with 64 thread(s)
...
Ran 1272 tests in 3417 ms wall time, 157946 ms across threads
Slowest tests:
- toolchain/lower/testdata/function/generic/call_recursive_basic.carbon: 1508 ms, 1484 ms in Run
- toolchain/lower/testdata/builtins/print_read.carbon: 1506 ms, 1506 ms in Run
- toolchain/lower/testdata/array/field.carbon: 1488 ms, 1487 ms in Run
- toolchain/lower/testdata/builtins/int.carbon: 1482 ms, 1475 ms in Run
- toolchain/lower/testdata/function/definition/params_one.carbon: 1472 ms, 1471 ms in Run
```
In test:
```
==================== Test output for //toolchain/testing:file_test:
Running tests with 64 thread(s)
...
Ran 1272 tests in 2968 ms wall time, 177732 ms across threads
Slowest tests:
- toolchain/lower/testdata/builtins/int.carbon: 1544 ms, 1533 ms in Run
- toolchain/lower/testdata/array/function_param.carbon: 1539 ms, 1537 ms in Run
- toolchain/lower/testdata/basics/zero.carbon: 1537 ms, 1536 ms in Run
- toolchain/lower/testdata/function/call/params_one.carbon: 1535 ms, 1534 ms in Run
- toolchain/lower/testdata/function/definition/params_zero.carbon: 1531 ms, 1531 ms in Run
[==========] Running 1272 tests from 1 test suite.
[----------] Global test environment set-up.
```
Richard Smith
Jon Ross-Perkins
Dana Jansens
Geoff Romer
Boaz Brickner
Ivan Duranandjosh11b