This is making it consistent with other places we set a Python version:
- contribution_tools.md
- .python-version
- bench_runner.py
- build-setup-common/action.yml
Assisted-by: Google Antigravity with Gemini
Noticed this in bazel central registry, I'm interested in trying it out.
It's using a faster approach, but leaving the other around for the
moment in case it doesn't work out well.
Assisted-by: Google Antigravity with Gemini
Trying to work on some behaviors:
- Using `black` to format Python files (using `pre-commit` makes better
use of allow-listed commands)
- Writing Python code over 80 columns (adding more style notes)
- Running `bazel` (being more emphatic about `bazelisk`, splitting tool
usage out to its own skill to try making clear it's not
toolchain-specific)
Assisted-by: Google Antigravity with Gemini
Missed in #6848 (had it sitting in my workspace uncommitted, apparently
have gotten too used to jj; using git here)
Assisted-by: Google Antigravity with Gemini
By using git_override, we get some validation from the sha, while
removing the sha256 on the .tar.gz which has been brittle lately. Note
the difference between downloading via sha is this still locally
validates content.
Versus something like #6844, this doesn't update the llvm version, just
how we get it.
Assisted-by: Google Antigravity with Gemini
In addition to the general updates, this switches to a required python
3.10 for pre-commit (3.9 is losing support from black).
Note endpoints for build actions are expanding significantly: see
https://app.stepsecurity.io/github/carbon-language/carbon-lang/actions/runs/22779388360?tab=recommendations&jobId=66080970460
for example, I think just the sources are being increased as a
side-effect of updates (and possibly also things not performing as well
as they should have before).
Similarly allowing sudo in pre-commit because it was actually causing
errors in part of build setup, which used sudo to remove files.
Assisted-by: Google Antigravity with Gemini
Giving both of these their own sections under optional tools because I'm
mainly doing this to share example configs.
Assisted-by: Google Antigravity with Gemini
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
We may want to split some out to skills, I'm just trying to merge in
some info of my own now.
Assisted-by: Google Antigravity with Gemini 3 Flash
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This uses the `CARBON_KIND_ANY(AnyImportRef, auto import_ref):` syntax
that seemed to be favored [on
Discord](https://discord.com/channels/655572317891461132/655578254970716160/1478486848207720478).
This converted uses in the `sem_ir` directory to show it works
initially, then added `check` for full coverage plus validating the
`SemIR::` namespace discard.
Note in inst_namer.cpp, AnyBindingPattern includes FormBindingPattern
which wasn't previously handled.
I'm disabling clang-format because I think it formats with readability
issues, e.g.:
```
#define CARBON_KIND_ANY_EXPAND_AnyBinding(X, SEP) \
X(::Carbon::SemIR::AliasBinding) \
SEP X(::Carbon::SemIR::FormBinding) SEP X(::Carbon::SemIR::RefBinding) \
SEP X(::Carbon::SemIR::SymbolicBinding) \
SEP X(::Carbon::SemIR::ValueBinding)
```
Since `SEP` is typically a comma, it's also a nuisance to treat as an
argument to `X` (which could get better results).
Assisted-by: Google Antigravity with Gemini 3 Flash
This is iterating on how `Destroy.Op` generates, to start adding body
capabilities. This changes the way the signature is created, and adds a
`CoreWitness` function kind so that mangling can prevent name
collisions. The result is that what _was_ `DestroyOp` is now
`Core.Destroy.Op` or, as can be seen in
toolchain/lower/testdata/interop/cpp/nullptr.carbon,
`_COp.<hash>:core.Destroy.Core` where `:core` is indicating that it's a
core witness (taking a note from `:thunk`).
Assisted-by: Google Antigravity with Gemini 3 Flash
This overlapped a little with `Destroy` work; adding the `:enclosed`
identifier (similar to `:thunk`) just to make it easier to identify. I
believe the TODO still applies.
Assisted-by: Google Antigravity with Gemini 3 Flash
Fixes ordering (using DIAGNOSTIC_ON_SCOPE). Removes an obsolete TODO to
add an error that's adjacent to the indicated error.
Also moves the file to patterns: it was the only file in `dataflow`, and
patterns also contains the related underscore binding tests.
Assisted-by: Google Antigravity with Gemini 3 Flash
This is a refactoring change with no output changes.
The chunk logic already separates the concepts of "nodes with children"
and "nodes with content" in practice, but it's not obvious in the API.
This rewrites the logic to make the separation clearer.
This also subtly takes advantage of the API to avoid creating lots of
empty chunks... Right now, there's always an empty chunk between two
tentative chunks. With this change, it lazily creates a chunk only when
`out()` is used (which it often isn't), which should substantially
reduce the number of chunks created.
This currently doesn't include much, but we expect to be generating more
entities, such as `Destroy`, which I'm aiming to get more clearly
categorized here instead of `imports`.
Assisted-by: Google Antigravity with Gemini 3 Flash
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This shifts logic a little so that empty top-level scopes are printed
less often. This affects imports mainly for now, but should be expected
to affect the soon-to-be-added generated scope more significantly.
Assisted-by: Google Antigravity with Gemini 3 Flash
I'm looking at making `constants { ... }` etc omitted when empty,
because in turn I'm looking at adding a third section, and seeing more
boilerplate empty sections just seems awkward to me. This PR starts down
the path by factoring out the chunk logic, which I may want to refactor
further.
This changes the `size_t` chunk id into a wrapped type for type safety.
This PR is just a refactoring, and doesn't make any behavior changes.
Assisted-by: Google Antigravity with Gemini 3 Flash
This TODO had been written before C++ types were generating destroy
implementations, which is resolved now.
Assisted-by: Google Antigravity with Gemini 3 Flash
This is related to #6727, but is generally a necessary fix even without
that issue. I'm not adding a specific test of #6727 because it should
also be covered by the tests in #6726.
Assisted-by: Google Antigravity with Gemini 3 Flash
- Add a `char` type literal mapping to `Core.Char` and equivalent to
C++'s
`char`.
- 8 bits, unsigned, treated as a single UTF-8
[code unit](https://en.wikipedia.org/wiki/Character_encoding#Code_unit).
- Add a `Core.CharLiteral` type for character literals, similar to
`Core.IntLiteral`.
- Allow operations for `char` and `Core.CharLiteral` which reinforce the
"character" concept, versus an integer value.
- Revokes and replaces
[#1964: Character
Literals](https://github.com/carbon-language/carbon-lang/pull/1964).
Assisted-by: Google Antigravity with Gemini 3 Flash
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This was motivated by `MakeFunctionDecl`, which has been added to
function.h as a helper function for making function declarations (an
unintentional naming collision).
I was wondering about renaming these functions to mark them as more
clearly import-specific, reducing the chance of name collisions like
this. Note the `Add` functions renamed here are typically updating an
imported declaration with a definition -- not sure whether `Make...Decl`
+ `Add...Definition` vs `Import...Decl` + `Import...Definition` is
actually losing anything though, since both seem to still require an
understanding of the two-stage import process.
Assisted-by: Google Antigravity with Gemini 3 Flash
Trying to update obsolete mentions in the "adding features" info (this
is just a skim, I may have mistakes and/or missed items).
Assisted-by: Google Antigravity with Gemini 3 Flash
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Change `SortingConsumer` from sorting by last processed token
(per-phase) to
additionally allow diagnostics to request sorting by start position
(line and
column) when the last processed token is the same.
Assisted-by: Google Antigravity with Gemini 3 Flash
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
As part of using the evolution process with the toolchain, alternatives
should
be in proposals. This proposal migrates existing alternatives here.
Assisted-by: Google Antigravity with Gemini 3 Flash
Also scrutinizing how it runs from another directory, because that's
what I did to test these changes. Switching to the repo root is to make
it easier to just look for ".jj".
Assisted-by: Google Antigravity with Gemini 3 Flash
Provides bidirectional mappings for types of integer and floating-point
literals
between Carbon and C++. For example, given a literal `123`, defines the
interop
type.
Co-authored-by: Ivana Ivanovska <iivanovska@google.com>
I need to do more work on the custom witness functions. This is trying
to make it easier to see the differences between the approaches before I
resume work there (e.g. this helps flag a possible reason I was having
trouble switching definitions when it came to generics, I think those
are mishandled right now).
This changes the thunk test because it was doing
`CheckFunctionDefinitionSignature` in a different order from
`handle_function.cpp`, and I think `handle_function.cpp` is more
canonical here (changing that affects tests with defined functions).
Assisted-by: Google Antigravity with Gemini 3 Flash
The intent is that `last_byte_offset` is still the main sorting key.
Diagnostics issued normally (e.g. in an expression) will keep sorting
the same, and come before the new diagnostic sort. Diagnostics issued at
the end of a scope (e.g. `unused`) can request sorting by their start
location, and would become interleaved through that.
Choosing "on scope" because I think that's the main way we'll use this
functionality (on scope changes); can always rename later if usage
expands.
Assisted-by: Google Antigravity with Gemini 3 Flash
Mainly because "sorting_diagnostic_consumer" is legacy, since
`SortingDiagnosticConsumer` became `SortingConsumer`. Also better
reflecting contents of these files.
Where I'm not renaming, I'm less positive about dropping "diagnostics"
from "file_diagnostics" and "null_diagnostics" (which contain both a
consumer and emitter, and "null.h" seems like poor naming), so not doing
that here. Also "diagnostic.h" contains `struct Diagnostic`, so is a
decent fit.
Assisted-by: Google Antigravity with Gemini 3 Flash
For example, see toolchain/check/testdata/class/fail_incomplete.carbon
for the diagnostic changes. `IncompleteTypeInFunctionReturnType` should
remain, while the redundant `IncompleteTypeInFunctionParam` is removed.
Note I'm deliberately trying to validate the return type after other
parameters, because I think that's the better user experience. This does
also incrementally change IR.
This is a mostly routine update, with some edits for a benchmark API
change.
I'm not updating LLVM here, since that could conflict with other ongoing
work.
Most versions are through `pre-commit autoupdate --freeze`, clang-format
was manually updated to the latest at
https://github.com/ssciwr/clang-format-wheel
My read of the style changes here are that they seem fine, none of them
look like regressions (which has caused me to delay/adjust updates in
the past).
OwningArrayRef is being removed upstream, per
https://github.com/llvm/llvm-project/pull/169126. This replaces uses
with `SmallVector`.
I've also made a separate commit which does init changes; these aren't
strictly necessary, but I added to make it a little more idiomatic in
spots.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Adding the note about recursion because it occasionally comes up, and
I'm thinking it'd be helpful to document why we prefer iterative
algorithms.
Also moves a few long style points to headers so that they're easier to
link (I wasn't sure it makes sense to do to all of "syntax and
formatting", but either way what's remaining is shorter if that _is_
linked for reference).
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Pursuant to recent decisions on #6124, switch `Destroy` to use a
`CustomWitness` for its implementation. Right now this is manufacturing
no-op implementation functions on each lookup, which obviously isn't
ideal but is intended as a first pass. I'm mostly trying to find the
right balance between updating the approach to reflect new decisions,
while still breaking apart work in a way.
The `CoreInterface` logic is intended to build on `CoreIdentifier`
support. We have a number of additional interfaces that require
specialized logic, and that'll extend pretty far with C++ interop, so it
seemed easiest to have a generic function for it. That's what's
replacing the logic inside C++ interop that was doing string comparisons
(which could have already been moved to `CoreIdentifier`, I just missed
it in my first pass).
This adds `CustomWitness` support because the `Destroy` witnesses can be
imported cross-file. `CustomWitness` was previously only used for C++
types, which don't yet support import, which is why that wasn't
previously an issue. The addition of `query_specific_interface_id` is
similarly needed in order to get correct sorting of witness blocks when
imported.
This PR also removes builtin constraint logic (note this is in a
separate commit to help review; it's not a separate PR because it's
difficult to split apart without tests breaking). This had been made
generic with the expectation that destroy, copy, move, and conversions
would all need related support. Under the new decision, we are not going
to do blanket impls and will instead just manufacture a `CustomWitness`
for everything.
A lot of SemIR fingerprints change, but that's probably because the
addition of `Destroy` on core classes is yielding structural changes.
This delays the conversion of `query_self_const_id` until the actual
witness creation, mainly so that users of `BuildCustomWitness` don't
need to do extra work to ensure `GetFacetAsType` logic is
shared/applied. This is coming up for destroy logic.
We're going to want to track `Destroy` and some other interfaces in ways
similar to what the C++ logic wants. Rather than having both do their
own comparisons with similar values, this tries to centralize logic.
Note the prior
`context.identifiers().Get(interface.name_id.AsIdentifierId())` is also
obsolete due to #6486, this is just replacing it in a single swoop and
avoiding string comparisons as a consequence.
Instead of naming the root namespace `package` (because it's accessed by
the `package` keyword), change it to use the current package name. Note,
buried in the checksum changes,
`toolchain/check/testdata/package_expr/fail_not_found.carbon`:
```
- // CHECK:STDERR: fail_not_found.carbon:[[@LINE+4]]:16: error: member name `x` not found in `package` [MemberNameNotFoundInInstScope]
+ // CHECK:STDERR: fail_not_found.carbon:[[@LINE+4]]:16: error: member name `x` not found in `Main` [MemberNameNotFoundInInstScope]
```
for:
```
// CHECK:STDERR: var y: i32 = package.x;
// CHECK:STDERR: ^~~~~~~~~
```
I'll leave it to you if you prefer this; the alternative I see is to
just rename `IsCorePackage` to `IsImportedCorePackage`, and/or change it
to a helper that takes a `Context` and does the right thing with
`parse_tree` (which, I need for `Destroy`-related reasons and was my
default approach).
This is in anticipation of using the same construct for all
implementations of `Destroy`, as well as other similar use-cases with
language-defined interfaces.
I'm doing this because I figured it'd be an incremental improvement for
all the operator lookups that we do. Even to the extent that we've
discussed witness caching, I think it'll still apply. It does add one
more step to adding new interfaces (before, you'd just write the string,
now you add it to the def file and reference it).
I'll claim it makes GetClangOperatorKind a lot friendlier to read/edit,
nevermind removing the string comparisons. :)
Per discussion, makes all symbolic local bindings a TODO. We should
implement them more correctly before making them operable. Right now
things partially work, but because constants behave mostly right in the
symbolic situations under tests. More broadly, it has incorrect behavior
and crashes, thus the TODO.
This converts most tests using `let` to instead using parameters, but
leaves some behind where a conversion either didn't make sense (e.g. in
`let` tests) or a conversion was unclear to me (multi-layer `let`, which
relies more on planned behavior that seems more bespoke to a local
`let`).
In let's `fail_generic.carbon`, there's a "// TODO: Should this be
valid?" that I'm removing because my understanding is the code in
question should be valid (the file is merged into let's
`generic.carbon`).
Refactoring `HandleAnyBindingPattern` a little because there's a TODO to
make it shorter, and it seemed like a reasonable drive-by change (let me
know if you think there's more I should do, or if I should remove said
TODO even though it's still a bit long).
Fixes#5982
I think these are obsolete, at least as far as I can tell. The former
appears tested (adding to be sure), the latter looks like it may no
longer occur.
Right now, the impl lookup can both fail to resolve the specific
definition because it's symbolic, and return a "final" constant because
it's a `final impl`. This is adding an instruction to help ensure the
specific is resolved.
The constant evaluation is fully recursive, but I'm not adding a TODO
since that's a known issue with impl lookup in general.
I was trying to figure out the right way to get specifics to be added to
the work.
Technically, we could keep the pending_specific list; this is taking a
different approach of inserting inside the work stack, which will do
extra work moving entries, although typically that should be expected to
be small. One challenge of `pending_specifics` is that if we would need
to shift them to work after both `Done` (for immediate processing) and
`Retry` (for processing after the current instruction is later revisited
and done). That feels kind of awkward as additional tracking to do.
Also, the common case is probably that there's either 0 or 1 specifics
being added, so an additional vector may be significant overhead. That's
why I leaned more in this direction of just inserting them in the vector
of work.
About the same # of LOC, but maybe less work to analyze correctness?
Versus the template, could also stamp that out in the helper function
and still avoid the duplication of calls before/after HasNewWork.
Similar to how I've left `rewrite_constraints`.
Alternately I'm also kind of tempted to rename GetLocalSpecificInterface
and GetLocalSpecificNamedConstraint to instead be overloaded functions
(or to provide overloaded versions), which would allow this to drop the
function type parameters. But, naming is hard.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This continues work to eliminate pending generics/specifics and get them
to be interleaved with instruction imports. I'm trying to use
`FinishGenericOrDone` here as a way to help ensure that code correctly
handles generics, where the simple alternative would be for each
`TryResolveTypedInst` call `SetGenericData` directly (but which might
make it easier to call the wrong `ResolveResult` function, and we do
need the `GenericId`s to be passed).
This is just an incremental step towards removing pending logic. The
rest seems like it'll be more complex due to interdependencies (I've
been poking at behavior).
This is part of trying to rewrite pending specific/generic code to make
use of the standard constant resolution flow. The LoadImportRef code was
a particular sticking point due to the recursion it does, which makes it
difficult to adapt over.
The to_array was mainly needed for zip_equal, and the
GetBlockAsTypeInstIds is forming a vector that should also be size two.
But just writing this out should avoid memory allocations.
Of course, then I'm like "but maybe a lambda or function would be
clearer than a for loop"... So the second commit.
There are two uses I'm not converting here, that seem to want the
"shortest" behavior. For everything else, I'm going to `zip_equal` since
it's more restrictive.
I wish `zip` were named `zip_shortest`.
- Makes a little more use of `MakeImportedLocIdAndInst` instead of
`UncheckedLoc`
- Requires use of `MakeImportedLocIdAndInst` with `ImportIRInstId`;
previously optional
- Relevant `if constexpr` moves to `AddPlaceholderImportedInst`, but is
more narrowly scoped there.
- Refactors out `AddPlaceholderImportedInstInNoBlock` to reduce how many
spots do an explicit `imports().push_back(...)`
I'd also considered removing `MakeImportedLocIdAndInst` where possible,
but went this route so that changes to the expected parse node wouldn't
affect callers. When it's required, `MakeImportedLocIdAndInst` is always
there; when it's conditionally present, changing `Parse::NodeId` between
enforceable and not-enforceable would require refactoring any callsites
that assumed one or the other.
Right now some of the `ResolveResult` factories are on it, ones that
involve `ImportRefResolver` aren't; this more consistently makes callers
use `ResolveResult::` when returning a result.
I was looking at this due to the addition of more
`GetAsTypeInstId(AddLoadedImportRef(` in #6344. Looking at
`AddLoadedImportRef`, it also felt like the first declaration would be
clearer if collapsed into its overload (the overload is the only
caller). Note one benefit of using `ImportContext` in
`AddLoadedImportRef` is being able to call
`local_constant_values_for_import_insts` to handle the `GetRawIndex`
code.
This has subtle effects on the number of imported instructions, but
seems more standard for how this code is being written...
`GetLocalConstantId` calls `GetLocalConstantValueOrPush` which does
`local_constant_values_for_import_insts().GetAttached`. So what this is
really doing is causing some intermediate import steps to be skipped.
But per test changes, that doesn't really affect SemIR and will probably
have negligible effect. This *seems* right to me, otherwise I'd expect
we should probably refactor all `GetLocalConstantId(InstId)` calls.
This turns `Cpp` into a keyword, and makes it map to `NameId::Cpp` and
`PackageNameId::Cpp`.
Per discussion with zygoloid, the keyword versus identifier question is
deliberately kept open by #4846. This PR switches to a keyword because
mapping to a specific `PackageNameId` works best with a special `NameId`
not backed by an `IdentifierId`. We could in theory make it work using
`IdentifierId` or a runtime-tracked `PackageNameId` for `Cpp` (e.g.
stored on `SemIR::File`), but this approach is consistent with `Core`
and so seemed like a good starting point.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
#6289 absentmindedly added fields in more places, and this is undoing
that plus further fixes.
This does some cleanup of types with relation to singletons. For
`TypeType` and `ErrorInst`, they're always complete due to a
`SetComplete` call in `file.cpp`. For `CppVoidType`, it's intended to be
incomplete by construction, and so a `TypeId` should be okay. The intent
though on not generally providing these had been that `GetSingletonType`
needs to be called to get a type to be marked as complete.
In the case of `AutoType`, removing `TypeId`does change a small printing
detail. I think that's old legacy that's just been carried forward.
Otherwise, for both `InstType` and `AutoType`, I've added
`GetSingletonType` calls where they were used in order to ensure
completeness is applied correctly. These calls cause small SemIR
permutations.
This causes `AutoType` to be seen by lowering, so I'm adding a
placeholder for it. Also merging two functions that look like they're
identical in intent -- not sure why they're separate.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
`ImplWitnessTablePlaceholder` is the only non-type singleton instruction
(`ErrorInst` is a type; while `ImplWitnessTablePlaceholder` exposes
`TypeInstId`, it's only used as an `InstId`).
In order to allow simpler handling of singleton instructions, replace
`ImplWitnessTablePlaceholder::TypeInstId` uses with
`InstId::ImplWitnessTablePlaceholder`. Since the placeholder instruction
was never evaluated, this has no significant effect on behavior.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This is just reducing boilerplate in `typed_insts.h` because we have a
number of singleton types, and keep adding more.
The changes to `TemplateString` allow `TemplateString IrName` to be used
as a `StringLiteral`.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This is to avoid edge cases where there are multiple `ConstType`
instructions, which code may not handle appropriately. I was thinking
about this for #6279
A `final impl` can have a symbolic witness, but that witness is still
final. Using "final" here per discussion on
[#generics-and-templates](https://discord.com/channels/655572317891461132/941071822756143115/1428851511672312120).
I'm also changing the variant a little because `concrete_witness` was
only called when `has_concrete_value` was true, so it can be more
careful about its contract. Having a more explicit `None` also
simplifies `has_value`. I think it doesn't change the overall cost much
past that.
Rather than assume reallocations can occur, we've switched to providing
stable references, so simplify related code.
Only removing the comment in `BuildGeneric`, no refactoring, because I
don't feel a refactoring would be a significant improvement.
This fixes and tests two crashes related to what I was observing [on
#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1422723976483831848).
The approach to Cpp imports taken in #6086 is problematic because it
returns before the work stack is completed, and doesn't store the
resulting constants. This fixes the approach taken in that PR.
Jon Ross-Perkins
