Insert the cleanup if and when the pending block is inserted, not
eagerly. And if the pending block is inserted by overwriting an existing
instruction, create a cleanup for that instruction rather than for the
instruction in the pending block that we are discarding.
Assisted-by: Gemini via Antigravity
A destructor is added to the C++ class definition in
`CarbonExternalASTSource::CompleteType`. The destructor calls a Carbon
function that calls the `Destroy` operator.
This lets us stop eliding it in textual semir tests with dump ranges.
Previously it would always get elided, even though it was part of the
range being dumped, and was referred to by other instructions in the
dump range.
Since each `.Self` is unique (can change its type if not its value) in a
facet type, having each one distinct by location also aids
understanding.
Also, when constructing the diff link, make sure we pick a commit that's
on the current PR's branch as the starting point. github wasn't able to
properly process the links we were creating before, if the last
dependency PR had commits that weren't on the current PR.
Fix a couple of tests that were broken by a prior change.
Assisted-by: Gemini via Antigravity
Move logic to merge class and function definitions onto Class and
Function, matching how we handle merging for EntityWithParamsBase.
Assisted-by: Gemini via Antigravity
A `where` expression nested inside a `T impls X` constraint makes
`.Self` ambiguous on the right-hand side of the `where` if `T` is
anything other than `.Self`. After the `where`, the value of a `.Self`
could be `T` or could be the value of `.Self` before the `impls`
constraint: the so-called top-level value of `.Self`.
Implicit use of `.Self` in designators is always allowed, and they are
bound (and replaced by a reference) to the inner-most possible value of
`.Self`. On the right-hand side of the nested `where` above, they have
the value `T as X`.
`.Self impls ...` is also always allowed, since it acts more as a
keyword here, and it always refers to the inner-most possible value of
`.Self`.
Any other explicit use of `.Self` is diagnosed when ambiguous, in any
kind of constraint. This is done in the handling of `WhereExpr` since it
has enough context to allow `.Self impls` (which is an explicit use)
while disallowing other explicit uses. And because it has non-canonical
instructions to work with, so it is able to diagnose errors with precise
locations.
Since `.Self` is no longer going to be marked with depth modifiers, the
eval of `WhereExpr` does not need an input facet value instruction
representing `.Self` to compare with, as they are now going to all be
equivalent. So revert it back to just looking for the `PeriodSelf` name
id, through a shared helper being introduced as `IsPeriodSelf`. And drop
the period self InstId from the `WhereExpr` instruction. This causes
most of the formatted SemIR changes.
Move helpers for working with and replacing `.Self` to their own file,
out of the `facet_type.h` header/cpp files. These are working with
`.Self` facet values more than facet types, though `.Self` is a name
that only exists inside the scope of a facet type.
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.
Richard Smith
Nicholas Bishop
Dana Jansens
David Blaikie
Christopher Di BellaandDana Jansens
Geoff Romer
Chandler Carruth