Also preserve the error state rather than overriding it, though this
doesn't seem to make a difference in practice.
Assisted-by: Gemini via Antigravity
The `split(" ")` function will split two consecutive spaces apart,
giving an empty string in its output. So `dump context inst5` was
mis-parsed to have arguments `["context", "", "inst5"]`. If `split()` is
called with no arguments, it splits on whitespace but ignores
consecutive whitespace, so we correctly parse the args to be
`["context", "inst5"]`.
`dump context facet_type` was an error before since we expected that to
be followed with an id value. While `dump context facet_type 5` still
works, if there's no id value, try to use `facet_type` as a variable
name. This allows us to dump an inst id if it happens to be named
`inst`, etc, without having to use `--` to disambiguate.
The removed TODO warned that we'd end up with O(n^2) witness table
entries per specific. This isn't a problem for a single associated
function but will cause issues for larger custom witnesses.
This commit generates at most two `SpecificId`s with an inner `Self`:
one for associated constants and one for associated functions.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
When any field of a Carbon class is access from C++ for the first time,
all fields are exported as `clang::FieldDecl`s (this is necessary
because clang fields have an internal index that is initialized on first
use).
`ClangDeclStore` now provides bidirectional mapping. This allows looking
up a `ClangDeclId` by `InstId`, so when Carbon class fields are exported
they can be looked up that way.
This enables thunking to work when the function has `ref` parameters,
without jumping through hoops to add `ref` tags in the desugared
function body.
This also renames `is_operator_syntax` to `is_desugared`, which is more
general and more accurate.
See
[here](https://docs.google.com/document/d/1rWcueFwIfZox6GKVGxiUG4cBzjrZ6djXiIDGyJDtrE4/edit?tab=t.0)
for the design doc.
This also removes the default value of the `result_type_inst_id`
parameter of `HandleAction`, moves it before the action in the parameter
list, and documents it. This solves two problems:
- The default made it easy to forget, leading to unnecessary
`TypeOfInst` instructions.
- When it was present, putting it after the fairly "bulky" action
argument tended to make the callsite harder to read.
When importing a C++ function with an rvalue reference parameter, we
previously produced a Carbon value parameter. This would lead to the
toolchain believing it could pass the address of a non-expiring object
to the function, which would lead to a use-after-move.
Instead, we now map non-const rvalue reference parameters to Carbon
`var` parameters. This forces the object passed into C++ to be unique
and owned by the call. While that's not an exact match for C++ rvalue
reference parameters, given that it provides "always move" not
"conditionally move", it's the closest match we have at the moment.
When a namespace that was imported from C++ is indirectly imported, find
the corresponding namespace in the current C++ AST and return that
instead. This namespace may have completely different contents than the
one we found before; that's fine. The current file's view of a namespace
depends on what it imported.
Assisted-by: Gemini via Antigravity
Use A..HEAD, where A is the head commit of the most recent dependency
PR. This should list all commits that are in the current PR that are not
part of that dependency commit. Produce the "warning" message if that
diff will include any commits that are in any other dependency PR.
Assisted-by: Gemini via Antigravity
When an initializing expression is used to initialize a var parameter,
we need to create the storage earlier in SemIR than the initializing
expression. To do so, pass a pending block to initialization containing
the var storage.
Also stop using `temporary` for this purpose, since we treat temporaries
as potentially-constant and immutable, but `var` parameters can be
mutated by the callee. We should ideally introduce a new kind of
instruction for this purpose but for now we just use `var_storage`.
This gets us back to being mostly clang-tidy clean. This turns out to be
important for agentic coding agents, which otherwise sometimes try to
"fix" these false-positive lints.
Assisted-by: Gemini via Antigravity
The `-Oz` flag has been [removed from
LLVM](https://github.com/llvm/llvm-project/pull/191363). The documented
replacement is to use `-O2` in conjunction with the `optsize` or
`minsize` attributes, which we already apply in lowering.
When a class template specialization is indirectly imported, map the
template arguments into the importing File and find the corresponding
local class template specialization. This is a short-term fix:
eventually we should import the C++ AST from the imported file into the
C++ AST for the current file, but we're not ready to do that yet.
So far we only support very simple template arguments: just classes and
builtin types. Unfortunately we can't just map the C++ template
arguments to Carbon types, then import the Carbon types, then map them
back, because mapping from C++ template arguments to Carbon types would
require a `Check::Context` for the imported code, which we don't have.
As this is only a temporary workaround, directly mapping from one C++
AST to another will do for now.
Assisted-by: Gemini via Antigravity
The labeling script will now directly set a check status for the PR as
`pending` when it marks something as dependent, and clear it when it no
longer is. This emulates a check that starts when marked as dependent
and runs until the last dependency lands, allowing automerge and other
workflows to work cleanly.
The branch protection rule will have to be updated to the new spelling.
This should do the same key thing as #7113, but integrated to the new
script.
Assisted-by: Antigravity with Gemini
Instead of linking to the first commit to be reviewed, link to the
complete series of commits to review. Include a warning if not all the
commits in that range should be reviewed due to non-linear history.
Assisted-by: Gemini via Antigravity
Because this is an `__attribute__((used))` method in a templated base
class it forces a _huge_ amount of template instantiation in every
translation unit.
Often this was just printing the members of the type, which is still
useful in some cases (such as test output), but adds no value in the
debugger.
A more successful pattern for dumping has been namespace level
functions, and particularly static ones that more transparently don't
expand the non-debugger API surface. Add the few missing functions there
that cover `Printable` types with more interesting contents.
For several of these, it just gives us a "dump the whole thing" function
as a compliment to "dump this entity in the thing". These probably
aren't especially high value, but moving them here they become cheap, so
I've left them in.
For a couple, this expands the rich dumping support of SemIR constructs,
which should be substantially more useful than the previous `Dump`
behavior.
This reduces `check` cumulative object file size by another 14%.
Assisted-by: Antigravity with Gemini
This makes rewrites of a constraint that requires a generic `I(Self)`
work. The `Self` there is replaced in the identified facet type by the
impl-as self type. But the rewrite is for the interface `.I(.Self)`, so
they don't match. Once `.Self` is replaced with the impl-as self type,
then they match and the rewrite is applied.
This should detect when a PR has a dependency of another open PR and add
a comment and label describing it. The comment will even do a
best-effort to compute the best starting commit for review.
Whenever PRs are closed, it will also scan the open depnedent PRs and
try to either remove the PRs in the comment or if it reaches zero the
label.
It works to update a single comment on a PR rather than adding more
comments.
Assisted-by: Antigravity with Gemini
Fixes a crash that would occur due to `scope_id` of the class being
unset.
Relands #7106 that was reverted by #7103 due to a github infrastructure
bug.
Co-authored-by: David Blaikie <dblaikie@gmail.com>
This allows `T impls X` constraints to function, since they must contain
some reference to `.Self` in order to be valid. This should be
sufficient to support the interfaces we need for for loops over C++
range-for-compatible types.
We replace `.Self` in the following places:
- In a require decl, as we have a specific self facet to replace it with
from the declaration, either a user-specified facet or the symbolic
`Self`.
- When identifying a facet type, as we have a specific self that we are
identifying the facet type with. That self gets used for all `.Self`
references.
- Implicit `.Self` references on the RHS of an `impls` constraint when
building a facet type. The `.Self` references there no longer refer to
the top level self facet, so replace them with the facet that we now
know they refer to, which is found on the LHS of the `where` before the
`impls`.
- Rewrite constraints in impl lookup when validating them and comparing
them with constants from witnesses, which come from identifying a facet
type.
- Rewrite constraints in ImplWitnessAccess eval when comparing them with
constants from witnesses, which come from identifying a facet type.
Substitution is done through `SubstPeriodSelf`. It handles replacing
`.Self` and `.Self as type`, for a replacement facet that is either of
type FacetType or TypeType.
Eval currently diagnoses some ambiguous `.Self` references when doing
substitution of `.Self` but this is the incorrect place to do it, so
there are TODOs about moving this to name lookup. To support these
diagnostics there's some additional complexity in `SubstPeriodSelf` that
can go away once the TODOs are addressed, such as asking the caller if
they want to replace each `.Self`, in order for it to report a
diagnostic.
There are a number of follow-up work items here:
- Some TODO tests.
- Remove `SymbolicBindingType` since its intention was to support
`.Self` but we don't need it with this approach.
- Replace `.Self` in rewrite constraints of require decls.
- Replace `.Self` in rewrite constraints of impl as when constructing
the witness table.
- Reject explicit `.Self` in name lookup when it would be ambiguous.
- Officially disallow `.Self.A = B` in rewrite constraints in the design
docs, so that we don't have the case where `.A` is allowed but `.Self.A`
is not due to ambiguity.
The existing `GetCoreInterface` has linear-time complexity and adds more
than the search criteria to the `CoreInterfaceCache`. Adding a new field
to `Interface` changes this operation when building packages other than
`Core`, as this should only be set for the core library.
We were not copying named constraints in the base facet type over to the
result of the WhereExpr eval.
Add tests that cover this by doing `impl as Constraint where ...` with
rewrite contraints either in the impl-as or in the named constraint.
When the interface is generic, these tests fail (as TODOs). When the
impl is used in impl lookup, we crash (with TODOs in the tests).
Part of #6991.
There are dependencies that kept this from upgrading automatically, but
while the new version technically includes breaking changes, they aren't
ones that cause any problems for VSCode.
Upgrading this is helpful as the old version has an irrelevant (for us)
security issue. With this we should be able to avoid distracting
security scanners.
Also updates other packages where relevant, all those automatically.
Assisted-by: Antigravity with Gemini
Without this, basic `bazel test //...` style wildcards would build a
bunch of extra configurations because of gaps excluding things. With
this, the action count of a normal build should be much more reasonable.
The switch from `target_compatible_with` to tagging is based on looking
at what ends up being most idiomatic and easiest -- trying to articulate
the complex and convoluted compatible with restrictions that would avoid
extraneous build configurations was really painful and this seems much
simpler and easier to deploy in a systematic way.
While here, also change the name of a rule that confused me to no end
while debugging this -- the rule that installs a `.bzl` file that
happens to be spelled `carbon_runtimes` is very different from all of
the other "installed carbon runtimes" kind of things in the tree. Adding
the file extension helps make that (much) more obvious.
Note that this is essentially a re-do of #7088 but now without any
dependencies that can mess up the merge.
Assisted-by: Antigravity with Gemini
When importing Carbon code that refers to a C++ class, look for a
corresponding C++ class in the current context and import that instead.
This is a workaround for not having proper cross-file C++ import
support. For now, we only support non-templated namespace-scope class
types.
Assisted-by: Gemini via Antigravity
When creating the C++ thunk, make the parameters references if the
corresponding callee parameters are `ref`s.
When creating the Carbon thunk, tag the call arguments as `ref` if the
corresponding callee parameters are `ref`s.
Instead of treating all C++ code as coming from a single synthetic
`CheckIRId`, track the `SemIR::File` associated with each C++ location.
This is necessary since each `SemIR::File` has a distinct `CppFile` and
therefore distinct `SourceLocation`s and `ClangSourceLocId`s.
Assisted-by: Gemini via Antigravity
- A function with a return declaration always has exactly one
`ReturnSlotPattern`, representing the whole return declaration (whereas
previously that was omitted for value and reference returns).
- The `ReturnSlotPattern` always has a subpattern with the same form.
`OutParamPattern` already plays that role for initializing forms, and
`TuplePattern` will play that role for tuple forms. This change
introduces `ValueReturnPattern` and `RefReturnPattern` to represent
value and reference return forms.
- As before, the `ReturnSlotPattern` has a corresponding `ReturnSlot`
that represents the output that is initialized by a `return` statement.
Its structure parallels the structure of the `ReturnSlotPattern`, so we
need `ValueReturn` and `RefReturn` insts that correspond to
`ValueReturnPattern` and `RefReturnPattern`.
This is a step toward supporting generic return forms, where the
`ReturnSlotPattern`'s subpattern may be an action: this change ensures
that evaluating the action for a specific form produces the same SemIR
as if the form were concrete to begin with. More speculatively, this
should simplify the implementation of `return` statements with compound
return forms.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This file contains base64 hashes that sometimes contain word-shaped
sequences like ...+nD+... that generate precommit false positives from
codespell.
MODULE.bazel.lock was already excluded from all precommit checks; it
seems consistent to do the same for package-lock.json too.
Assisted-by: Gemini via Antigravity
This shouldn't change any functionality, but simplifies (significantly)
the logic in the installed toolchain, and also provides a better
conceptual balance between these.
I've tried to minimize the changes beyond a pure refactoring, but it was
a bit tricky to get everything working so some things have been mixed
in...
Assisted-by: Antigravity with Gemini
This reverts commit 4babfdbf22.
This was a stacked PR that was merged by accident, losing the commit and
description of the base change. Reverting and will re-land
independently.
Without this, basic `bazel test //...` style wildcards would build
a bunch of extra configurations because of gaps excluding things. With
this, the action count of a normal build should be much more reasonable.
The switch from `target_compatible_with` to tagging is based on looking
at what ends up being most idiomatic and easiest -- trying to articulate
the complex and convoluted compatible with restrictions that would avoid
extraneous build configurations was really painful and this seems much
simpler and easier to deploy in a systematic way.
While here, also change the name of a rule that confused me to no end
while debugging this -- the rule that installs a `.bzl` file that
happens to be spelled `carbon_runtimes` is very different from all of
the other "installed carbon runtimes" kind of things in the tree. Adding
the file extension helps make that (much) more obvious.
Assisted-by: Antigravity with Gemini
ImplWitnessAccess can contain a LookupImplWitness instruction as an
operand, which used to be SymbolicOnly but has now become Conditional in
#6915. This opens up the possibility for LookupImplWitness to have a
concrete value, without resolving to a different instruction kind. If
this occurs when it's the operand of an ImplWitnessAccess, and the
access is unable to find a different value to resolve to through the
witness' self type, then the ImplWitnessAccess can also become concrete.
This can happen in particular when:
- You have an ImplWitnessAccess into a `.Self` symbolic in a facet type.
- You convert a concrete type to the facet type.
- The `.Self` is replaced by a concrete type, causing the impl lookup to
be on that concrete type.
- The (now concrete) impl lookup fails to find any impl witness, so it
remains a concrete LookupImplWitness
This results in a diagnostic, as the incoming type does not satisfy the
facet type, but in the meantime we have a concrete ImplWitnessAccess,
which we do not want to crash.
For example in this test:
```carbon
interface I {
let I1:! type;
}
interface J {}
fn F(T:! I where .I1 impls J) {}
fn G() {
class C;
// This identifies `C as (I where .I1 impls J)`, which replaces `.Self.I1`
// with `C.(I.I1)`. This is a concrete lookup since C and I are both concrete,
// but it doesn't find anything as there is no impl.
//
// As such, the call to F fails to deduce a value for T.
F(C);
}
```
This PR splits out the change to ImplWitnessAccess from the larger
change of replacing `.Self` in identified facet types (and comparisons
with identified facet types).
Richard Smith
Dana Jansens
Christopher Di BellaandDana Jansens
Nicholas Bishop
Geoff Romer
Chandler Carruth
dependabot[bot]