This is in addition to finding a `where` on the RHS of another `where`.
Since a generic binding introduces `.Self`, any `where` expression that
isn't part of a facet type modifying the binding itself would introduce
an ambiguous `.Self`.
Add virtual parse nodes for let, var, and form bindings, which goes
before the type. This allows us to track if `where` appears in the
binding's type. We only need to look for an invalid `where` if any
appeared in the type. We combine these three nodes together into a
single node kind, which requires us to remove the name from it as a
child. We move it up to the Pattern node again, and rename the
PatternStart nodes to PatternTypeStart as they are now located in the
middle of the Pattern nodes, just before the type.
And we only need to thaw `.Self` in generic bindings. Non-generic
bindings can only have `.Self` through a `where` expression, since the
name is not provided otherwise to non-generic bindings. And `where`
expressions thaw their `.Self` independently. So the binding only needs
to thaw a `.Self` that it introduced, which is only for generic
bindings.
Implement the toolchain side of proposal #7254, removing the `:!`
binding
syntax for generic and template parameters in favor of the keywords
`generic`,
`template`, and `runtime` plus contextual defaults for phase.
For valid programs this is semantics-preserving: each binding resolves
to the
same phase, and produces the same SemIR, as it did under `:!`/`:`. The
parser
derives a binding's phase from its syntactic context plus any explicit
phase
keyword; new diagnostics and error recovery for misused keywords are
described
below.
Implementation details for each component:
- Lexer: remove the `:!` (`ColonExclaim`) token, move its virtual
parse-node
budget onto `:`, and add the `generic` and `runtime` keywords.
- Parser: thread a `BindingContext` (`ExplicitParam`, `DeducedParam`, or
`CompileTimeEntityParam`) from declaration introducers down through
parameter
lists to each binding pattern, using a one-token lookahead to
distinguish a
name-qualifier parameter list from a declaration's own final list.
Parameters
of a compile-time entity (`class`, `interface`, `constraint`, `choice`,
`alias`, `export`, `namespace`) and deduced `[]` parameters default to
checked
generic; explicit function parameters and local bindings default to
runtime.
`HandleBindingPattern` resolves the phase from that context plus the
keyword: a
`generic` keyword needs no node of its own (the phase is carried by the
binding's node kind), while a `runtime` keyword is preserved as a
`RuntimeBindingName` node so `check` can name it in a diagnostic. A
phase
keyword that is merely redundant with the contextual default is
diagnosed
here, without invalidating the parse tree.
- Check: a phase keyword that is invalid for its context (for example
`runtime`
on a checked-generic parameter) is diagnosed here, and recovers by
building an
error binding that still introduces the name so that later uses of it do
not
produce cascading errors.
The removed `:!` syntax is now rejected as an ordinary parse error.
The `form`/`:?`/`->?` ("extended types") portion of proposal #7254 is
left for a
separate change.
Assisted-by: Claude Code
Implements proposal #7016: `self` moves from the deduced implicit list
(`fn F[self: Self]()`) to the front of the explicit list. Its type may
be written explicitly (`fn F(self: Self)`) or omitted, in which case it
defaults to `Self` (`fn F(self)`, `fn F(ref self)`); `self` in the
implicit list is rejected.
Throughout checking, `self` is modeled as the first explicit parameter.
Because a method is just a function whose first parameter is `self`, it
can also be called as an ordinary function with the receiver passed
explicitly (`Type.M(obj, ...)`), not only as `obj.M(...)`. A new
`SemIR::CallArgParamPatterns` helper chooses the parameters matched
against the explicit arguments, excluding a leading `self` only when it
is supplied as a method-call receiver; arity checking, conversion, and
generic deduction use it. The resulting SemIR and lowering are
unchanged: `self` is still `call_param0`, and witnesses, thunks, and
vtables are unaffected.
An omitted `self` type is parsed as a `SelfBindingPattern` node with no
type expression; checking synthesizes the `Self` type so it behaves
exactly like `self: Self`. However, the exact spelling used must match
between a forward declaration and a definition, following #3763's rules
around declaration matching.
Generated functions, thunks, and C++ interop import/export build `self`
as the first explicit parameter, and the `self`-type override (e.g.
Derived->Base for a virtual override) applies to the explicit `self`.
Placement is validated by new diagnostics: `SelfInImplicitParamList`,
`SelfNotFirstParam`, and `SelfOutsideParamList`. The benchmark source
generator and the documentation adopt the `(self)` shorthand; the
prelude, the examples, and the test data are migrated in the following
commits.
Assisted-by: Claude Code with Claude Opus 4.7
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
If a keyword or a sized type literal (eg, `f2`) is used in a context
where we are confident that we are expecting an identifier -- either
before a `:` in a binding pattern or after a `.` in a member access or
designator -- then recover as if a raw identifier was used.
This appears to be a particular stumbling block for coding agents, so
seems worth paying special attention to.
Add a mechanism to the tokenized buffer to track additional tokens
synthesized for error recovery so that we can keep the lexed token
sequence immutable and still satisfy the invariants throughout the rest
of the toolchain for recovery tokens. Thanks to chandlerc for suggesting
this approach!
This change ensures that a function signature always starts with an
`IdentifierNameMaybeBeforeSignature` node (renamed from
`IdentifierNameBeforeParams`), even in the case of function declarations
like `fn F -> T` that have no parameter list. As a consequence, this
ensures that we push new entries onto `pattern_block_stack` and
`full_pattern_stack` when we start processing the function signature.
Every test that used `addr` before #6283 should be using `ref` after
this PR. In most cases that was done in #6283, but this PR transitions a
few that I missed in that first pass. In addition, #6283 cloned the old
`addr` tests from `foo.carbon` to `foo_addr.carbon` in order to maintain
test coverage during the transition; this PR removes those cloned tests.
We add a virtual node (`CompileTimeBindingPatternStart`) as the first
child of `CompileTimeBindingPattern` which holds the identifier
underneath it, so that it is checked just before the type expression of
the `CompileTimeBindingPattern`. When we reach this virtual node during
check, we add `.Self` as a name in the current scope, and when we reach
`CompileTimeBindingPattern` we remove it from scope, which ensures it's
present during only the checking of the type expression for the compile
time pattern.
At the moment the `.Self` has a different type (it's a `TypeType`) than
other `.Self` in the facet type (which are a single `FacetType`), but
the intention is to immediately substitute it out of the facet type
entirely, replacing it with a reference to the compile time binding (a
`BindSymbolicName`) itself. A TODO has been added for this.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Also renames variables of that type to match. A follow-up PR will rename
`Parse::StateStackEntry` to `Parse::State`. This is more consistent with
the naming of similar enums elsewhere in the toolchain, and with the
prevailing practice of using `state` rather than e.g. `entry` as the
name of a `StateStackEntry`.
See also discussion
[here](https://discord.com/channels/655572317891461132/963846118964350976/1326280585592700990)
What this really does is avoids shadowing names, so that we can
comfortable have things like `Check::DiagnosticEmitter` or
`Check::DiagnosticLoc` without shadowing being a concern.
Note, down this path I'm also thinking about:
- Renaming misc DiagnosticConsumer/DiagnosticEmitter classes, possibly
just to DiagnosticConsumer/DiagnosticEmitter (so
`Check::DiagnosticEmitter` instead of `SemIRLocDiagnosticEmitter`).
- Dropping `Diagnostic` from `Emitter::DiagnosticBuilder`.
- But not for `Check::DiagnosticBuilder`, because `Check::Builder` would
be ambiguous.
- Renaming diagnostics/diagnostic_* to drop "diagnostic".
[Discussion about SemIRLoc ->
DiagnosticLoc](https://discord.com/channels/655572317891461132/655578254970716160/1353771570463768698)
reminded me of this (in particular the older [Check::DiagnosticBuilder
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1344363562608627763)),
but I'd only do that rename if there's matching consensus about a path
forward where we keep SemIRLoc, and in a way that it's only ever used
for diagnostics (the divergence from which is at the root of current
LocId discussion).
I'm trying to keep that separate from a namespace addition for clarity.
Change parse tree from `template (T:! type)` to `(template T):! type`,
so that we have information about whether a binding is a template
binding available when forming the representation of the binding
pattern. This incidentally fixes a bug that we would accept `template
addr A:! B` instead of the intended `addr template A:! B`.
Track whether a symbolic binding is a template binding on the
`EntityName` object. I'm borrowing a bit from the `CompileTimeBindIndex`
for this in order to avoid making `EntityName`s larger. Longer-term, we
should think about using a different representation for symbolic
bindings, to avoid including these fields in all `EntityName`s, but
that's out of scope for this change.
So far, template bindings are treated as having the same phase as
checked bindings, but that will change in a future PR.
This change splits `NodeKind::IdentifierName` into separate node kinds
depending on whether the identifier is followed by parameters, and
similarly splits `NameQualifier` based on whether the qualifier has
parameters. This enables us to only push a pattern block when it's
actually needed, rather than "defensively" pushing one when it might be
needed.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Building on #4411, avoid using StringLiteral in format strings. This
includes a diagnostic check to prevent regressions (which is also how I
gathered issues).
Note, I haven't looked at `std::string` uses yet, but we might need
things like that to be able to pass strings in code back to the user.
StringLiteral though means that it's literally written down in the
toolchain, at which point it should probably be written in the format
string instead of separately.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This is a primarily automated change:
- Search & replace for capitalization
-
`(CARBON_DIAGNOSTIC\((?:\n\s+)?\w+,(?:\n\s+)?\s\w+,(?:\n\s+)?\s")([A-Z])`
- `$1\L$2`
- Search & replace for period
-
`(CARBON_DIAGNOSTIC\((?:\n\s+)?\w+,(?:\n\s+)?\s\w+,(?:\n\s+)?\s"(?:[^)]|\n)+)\.("[,)])`
- `$1$2`
- Limited search & replace for `ERROR: ` -> `error: ` in streamed things
- Leaving a TODO for command_line because there's more cleanup that can
be done there
- Modify diagnostic_consumer.cpp
- ERROR -> error
- WARNING -> warning
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Move subtree sizes over to TreeAndSubtrees, using the different
structure to represent the additional parse work that occurs, as well as
making it clear which functions require the extra information. My intent
is to make it hard to use this by accident.
The subtree size is still tracked during Parse::Tree construction. I
think a lot of that can be cleaned up, although we use it during
placeholder assignment so it may take some work. I wanted to see what
people thought about this before taking action on such a change.
I'm using a 1m line source file generated by #4124 for testing. Command
is `time bazel-bin/toolchain/install/prefix_root/bin/carbon compile
--phase=check --dump-mem-usage ~/tmp/data.carbon`
At head, what I'm seeing is:
```
...
parse_tree_.node_impls_:
used_bytes: 61516116
reserved_bytes: 61516116
...
Total:
used_bytes: 447814230
reserved_bytes: 551663894
...
1.43s user 0.14s system 99% cpu 1.565 total
```
With `Tree::Verify` disabled completely, it looks like:
```
parse_tree_.node_impls_:
used_bytes: 41010744
reserved_bytes: 41010744
...
Total:
used_bytes: 427308858
reserved_bytes: 531158522
...
1.20s user 0.13s system 99% cpu 1.332 total
```
Re-enabling just the basic verification (what is now `Tree::Verify`),
I'm seeing maybe 0.05s slower, but that's within noise for my system. I
do see variability in my timing results, and overall I think this is a
0.2s +/- 0.1s improvement versus the earlier (always testing `Extract`
code) implementation. That's opt; debug builds will be unaffected,
because the same checking occurs as before.
Note, the subtree size is a third of the node representation, which is
why I'm showing the decrease in memory usage here.
Most of these are places where we failed to include a header file and
simply never got an error about this. The fix is to include the header
file.
Most other cases are functions that should have been marked `static` but
were not. Finding all of these was a main motivation for me enabling the
warning despite how much work it is.
One complicating factor was that we weren't including the `handle.h` for
all the state-based handler functions. While this isn't a tiny amount of
code, it is just declarations and doesn't add any extra dependencies. It
also lets us have the checking for which functions need to be `static`
and which don't. For the `parse` library I had to add the `handle.h`
header as well, I tried to match the design of it in `check`.
I have also had to work around a bug in the warning, but given the value
it seems to be providing, that seems reasonable. I've filed the bug
upstream: https://github.com/llvm/llvm-project/issues/94138
I also had to use some hacks to work around limitations of Bazel rules
that wrap `cc_library` rules and don't expose `copts`. I filed a bug for
`cc_proto_library` specifically:
~https://github.com/bazelbuild/bazel/issues/22610~https://github.com/bazelbuild/bazel/issues/4446
Building on #3463. The PushState+PopState to construct a state feels
worth cleanup. The rest is just kind of making it easier to do without
adding another PushState overload.
oli-ej
Dana Jansens
Chandler Carruth
Richard Smith
Geoff RomerandDavid Blaikie
CJ Johnson