This is stamping out the per-instruction structs, similar to what we do
elsewhere. `BuiltinInstKind::label` then finishes shifting to
`InstKind::ir_name`.
The new `FacetValue` instruction represents `C as I` for some type `C`
and facet type `I`. It is named `FacetValue` instead of just `Facet` to
parallel the `FacetType` instruction.
This PR uses this instruction represent the facet value `Self` in an
`impl` declaration. This instruction will be used in the future to also
support things like:
* `C as I` where `C` is a class; and
* forming a specific for a generic with a `T:! I` parameter where `T` is
being given a concrete value.
(Here `I` is an interface or other non-`type` facet type.)
Also do some renaming and add some comments to make things a bit more
clear.
* `FacetTypeAccess` -> `FacetAccessType` to clarify this is not access
of a facet type, but access of the type of a facet
* `.facet_id` -> `.facet_value_inst_id` to parallel the `FacetValue`
instruction
`FacetAccessWitness` will be in a future PR.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Replace the special case adding the base class to the list of extended
scopes with a fully general approach. We use an ImportRef to lazily
import the extended scopes on first lookup.
This adds language to explain where the types of constraints can or can
not appear (attached-to an impl-as vs in-a type expression). And
describes the impact of using a rewrite vs same-type constraint inside
the body of the affected code, and thus why a rewrite is preferable when
the constraint is of a single facet type.
Adds per-builtin instructions, removing `BuiltinInst`. This collapses
`builtin_inst_kind.def` into `inst_kind.def` so that we have a single
place for all macro uses. I still want to remove `BuiltinInstKind`, but
it's something I think is better separated from the `BuiltinInst`
removal.
I'm collapsing the build targets `ids` and `inst_kind` into one because
they both have links to builtin kind information now. It's hard to
separate without a cycle. I'm using the `typed_insts` name because that
seems like the actual most significant thing there, and more interesting
relative to the `inst` target.
I'm looking at adding more significant logic to checking, particularly
for interop. But check.cpp is getting large, and I think just adding
more logic will make it harder to reason about, so I'm looking at
splitting it up. This moves out NodeIdTraversal and
DeferredDefinitionWorklist because they're already independent from the
other code, and are reasonably sized to have their own files.
These tests typically take 10-20s, but I'm seeing some timeouts
[here](https://github.com/carbon-language/carbon-lang/actions/runs/11899548036/job/33158400417).
This seemed particularly suspicious due to the _absence_ of output
(copied below). That got me looking, and maybe the subprocessing tickles
a cpu bottleneck, so proposing this approach to remove the exec. Even if
this doesn't solve the flakiness, I think it's a simpler implementation.
Note I believe this is intended to work. The `sh` rules rely on shebangs
(as noted at https://bazel.build/reference/be/shell#sh_test), and are
essentially just subprocessing to the input. Note this could've also had
`args` on a `cc_test` rule, but I'd expect the same args to be passed to
`run` where instead the benchmark behavior should be default (and I'm
assuming you'd rather not have args there). Fundamentally this becomes a
symlink:
```
bazel-bin/common/map_benchmark_test -> .../execroot/_main/bazel-out/k8-fastbuild/bin/common/map_benchmark
```
Copying snippet from timeout below:
```
==================== Test output for //common:map_benchmark_test:
/private/var/tmp/_bazel_runner/e591f63ed099023de1f206992dfce127/execroot/_main/bazel-out/darwin_arm64-fastbuild/testlogs/common/map_benchmark_test/test.log
-- Test timed out at 2024-11-18 19:32:13 UTC --
INFO: From Testing //common:map_benchmark_test:
================================================================================
```
This is for more clearly distinct names, and to make it a clearer
transition from `BuiltinInst` for name conflicts. `FloatType` is also an
instruction, and we have `Carbon::Error` (common/error.h). This avoids
affecting tests, although the name is embedded in the builtin test.
In `LegacyFloatType`, `Legacy` because I was having trouble coming up
with a more appropriate name. I'm not clear this is a `FloatLiteralType`
at present, it needs some work to mirror `IntLiteralType`.
In `ErrorInst`, the suffix `Inst` was discussed as good and similar to
`BuiltinInst` (although I'm trying to get rid of that).
For the few remaining uses of the builtin `i32` type, manually build an
`IntType(Signed, 32)` value instead. These are:
- The return type of `Run`.
- The type that int literals in an `if` expression are converted into.
- The type of an array index expression.
We should consider converting those three cases away from `i32` over
time.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
The prettier pre-commit mirror is no longer supported
(https://github.com/pre-commit/mirrors-prettier). This switches to a
direct call, and updates to 3.3.3. And I'm now specifying types for it
to apply to, rather than letting it ignore unknown files; overall just
trying to separate out which linter sees what.
To comment on formatting changes:
- In most cases, seems to be getting confused by `[]` use in markdown
when it's not part of a link. This looks like a regression, but not one
we're broadly affected by.
- p0107.md - caught an issue with a malformed broken bad link which I've
tried to fix.
- p3720.md - looks like a fix.
Note, prettier has a 4.0.0 alpha release. As best as I could tell, that
only affected the .prettierrc.yaml processing. I changed the glob there
for forwards compatibility.
As discussed in #4530 . This required switching to using
`llvm::IntrusiveRefCntPtr` in a number of places.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
The name ComparableFromDifferenceFn comes from the next example. In this
example ComparableFromDifference is the name of the class that will be
implictly cast to a Facet matching Comparable.
This initializes the DriverResult::per_file_success field explicitly
with `= {}` in order to encode that DriverResult can be constucted via
aggregate initialization while omitting the per_file_success field. This
prevents -Wmissing-designated-field-initializers from firing in newer
clang versions when constructing DriverResult like:
```
return {.success = false};
```
Newer clang-tidy warns that the `= {}` is redundant however it is not,
as its marking which fields need to be explicitly initialized. So we
suppress it.
Stop rejecting `extend adapt` of non-class types such as struct and
tuple. These don't actually work just yet because name lookup into
struct and tuple types is a special case that doesn't handle adapters,
but this gets us a bit closer.
This also slightly improves error recovery for name lookup into an
invalid scope.
When an `IntLiteral` appears as an operand of an `if` expression,
convert it to `i32` for now, so that we don't reject things like `if
cond then 1 else 2` due to having a non-constant value of type
`IntLiteral`.
For tuple indexing expressions such as `(a, b).0`, convert the index to
type `IntLiteral`, not to type `i32`. This isn't strictly necessary to
do in this PR, but avoids the need to provide an `IntLiteral` -> `i32`
implicit conversion for `no_prelude` tests using this syntax.
Instead of leaving array bounds as whatever integer type they arrive as,
convert them to the `IntLiteral` type as part of forming an `ArrayType`.
This ensures that array types canonicalize properly even when the bounds
are specified with different types.
Create an empty generic definition for a generic builtin function to
avoid this causing "use of undefined generic function" errors.
We do not intend to use std::ranges in the Carbon implementation due to
concerns of compile time cost, largely due to implicit instantiation of
types involved in calling and typechecking the functions and their
requires clauses.
In #4539, we converted std::any_of to std::ranges::any_of, but this
replaces that with llvm::any_of from llvm/ADT/STLExtras.h.
This conversion was suggested by the modernize-use-ranges clang-tidy
check. We can keep the check on, and use it to guide conversion to llvm
helpers that do similar things (as was done in this CL now). If it's
being too confusing, then it can be disabled as well.
This is at a point where it's getting difficult to read, and #4479 is
adding a large block comment. In order to more clearly delineate
members, add blank lines between.
In preparation for changing integer literals to be of `IntLiteral` type.
Conversions from the integer literal type are only permitted when the
value fits within the destination type.
For now, if conversion cannot be checked because the source value is a
symbolic constant, produce a symbolic constant representing the
conversion rather than rejecting it.
Commenting on danakj's clang-tidy PRs, it would've been helpful to just
have a quick reference for older checks. So while I've been getting
comments for new things, go back and comment the ones that predate
adding per-check comments.
I did this mainly by running clang-tidy and seeing whether we could
re-enable them, thus also fixing the clang-tidy wrapper script.
Adding backticks to try to make it easier to scan.
- Do not include entities imported from files that we are not dumping.
- Do not include constants and import_refs that are not referenced by
something that we are including in the formatted output.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
This warns unhelpfully on enums like:
```
enum Kind: int8_t {
Value,
ValueOrRef,
...
FullInitializer,
Last = FullInitializer
};
```
It claims that all enum values should have explicit values if any of the
values do, but that's not what we would want to write here.
I think there are a few related ways to do this. I considered
llvm::make_scope_exit, but the return type is difficult to work with. I
particularly was thinking I could encapsulate the duration logic this
way.
Trying to improve robustness against failures such as
[here](https://github.com/carbon-language/carbon-lang/actions/runs/11845529962/job/33011203628):
```
WARNING: Download from https://ftp.gnu.org/gnu/m4/m4-1.4.18.tar.xz failed: class java.net.ConnectException Connection refused
```
Which is coming from the target-determinator invocation:
```
subprocess.CalledProcessError: Command '['/home/runner/.cache/carbon-lang-scripts/target-determinator', '--bazel=/usr/local/bin/bazelisk', 'a720921dfb99c21f08832663c91a1f9e48a1bcc4']' returned non-zero exit status 1.
```
My thought is that the `bazel cquery` should trigger equivalent
downloads (equally though, I'd thought the `bazel mod deps` would do
that, so I don't want to give the impression of confidence).
It's harder to inject this into the target-determinator command line,
since we'd need something that retries by default.
As a prerequisite for switching the type of int literals to be the
`IntLiteral` type, add support for performing conversions of in-bounds
integer constant values to other integer types in which they fit.
This incidentally is our first compile-time-only builtin function, so
add very minimal support for compile-time-only functions while we're
here.
* The `extended_scopes` in a `NameScope` were represented by a
`NameScopeId`. Replace that with an `InstId` of an instruction returning
the type that is extending this name scope.
* `Context::LookupQualifiedName` now can take multiple scopes to look
in.
* `GetAsLookupScope` was moved out of `member_access.cpp` and is now
`Context::AppendLookupScopesForConstant`
This PR also fixes some existing issues that were revealed as part of
writing and testing this PR:
* Additional validation and handling of invalid ids.
* `extend impl` in a class is not properly imported yet, but at least
now it doesn't crash.
The change to use an `InstId` also allowed some diagnostics and
formatting to be improved.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
This switches from `int_store*` to `int*` as this file contains both the
ID and the store for integers.
This was supposed to be added to #4487 before merging, apologies for
missing that.
I'm working to make sure remaining diagnostics have coverage, at least
the ones I'd previously added a TODO for. Note in particular that I
couldn't figure out a repro for UnaryOperatorRequiresWhitespace; if you
have one, I can add a test, but otherwise maybe it's actually
unreachable due to being diagnosed through infix logic (or, maybe
this'll let fuzzing tell me an example).
The first change here is to canonicalize away bit width when tracking
integers in our shared value store. This lets us have a more definitive
model of "what is the mathematical value". It also frees us to use more
efficient bit widths when available, such as bits inside the ID itself.
For canonicalizing, we try to minimize the width adjustments and
maximize the use of the SSO in APInt, and so we never shrink belowe
64-bits and grow in multiples of the word bit width in the
implementation. We also canonicalize to the signed 2s compliment
representation so we can represent negative numbers in an intuitive way.
The canonicalizing requires getting the bit width out of the type and
adjusting to it within the toolchain when doing any kind of math, and
this PR updates various places to do that, as well as adding some
convenience APIs to assist.
Then we take advantage of the canonical form and embed small integers
into the ID itself rather than allocating storage for them and
referencing them with an index. This is especially helpful for the
pervasive small integers such as the sizes of types, arrays, etc. Those
no longer require indirection at all. Various short-cut APIs to take
advantage of this have also been added.
This PR improves lexing by about 5% when there are lots of `i32` types.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This PR adds a `--dump-timings` flag to the `compile` subcommand
(similar to the existing `--dump-mem-usage` flag), which collects timing
data per compilation unit for each compilation phase. For example, on my
2020 M1 MacBook:
```
$ bazel build -c opt //toolchain
$ bazel-bin/toolchain/install/run_carbon compile --phase=lower --dump-timings examples/sieve.carbon | tail
...
---
filename: 'examples/sieve.carbon'
nanoseconds:
lex: 30792
parse: 25458
check: 226625
lower: 1136958
Total: 1419833
...
```
Most of the changes are pretty straightforward. There were a couple I
wasn't sure about though; let me know if I should change:
- new `Timings` class in its own file, pretty similar to the existing
`MemUsage` class
- added a `timings_` field to the `CompilationUnit` class
- added a `timings` field to the `Check::Unit` struct
- renamed `CheckParseTree` function to `CheckParseTreeInner` for ease of
timing with early `return`
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
The test update shows a class derived from an imported base class with a
vptr, and without this change the derived class got its own vptr, with
this change the derived class can see the base is dynamic, so the
derived doesn't need to add a vptr and can rely on the base class's vptr
instead.
This can lead to us trying and failing to print certain kinds of
constant value, but we can fix that in future changes.
Note that `StringifyType` should probably be substantially refactored.
For this change I'm trying to leave the overall structure relatively
intact, but hopefully this additional formatting support will help guide
future refactorings.
While here, also change the diagnostic emission to pass the interface
type rather than the interface name. This prepares us to include the
arguments in the diagnostic.
Instead, wait until we know whether it is used as a value or reference
expression. This allows us to avoid materializing a temporary if it is
used as a value and the initializing representation holds a copy of a
value representation.
Jon Ross-Perkins
josh11bandJosh L
Chandler Carruth
Dana Jansens
David BlaikieandJon Ross-Perkins
Sam EstepandJon Ross-Perkins