This is mirroring the structure of codegen/codegen.h, lower/lower.h, and
check/check.h. I recently did lex/lex.h, so parse/parse.h is the last.
Now, the directory's main API file is eponymous with the directory.
I could've used a friend function to avoid making the Tree constructor
public, but in other places we make less use of `friend`, just leaving
things public. This felt more consistent, and simple because it only
affects the constructor.
Goal is to increase type safety, though more work needs to be done (see
added TODOs).
Note that, after this change, check handlers corresponding to deleted
parse node kinds will no longer compile.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Use this new category to replace the unconstrained `NodeId` child of
`MemberAccessExpr ` and `PointerMemberAccessExpr`. For now this new
category matches `IdentifierName` and `BaseName`, but later this will be
expanded to support `a.(b.c)` and `p->(b.c)` syntactic forms.
QUESTION: Is it time to make a `node_category.def` x-macro file?
ANSWER: Not yet.
Tests previously uncovered code. Fix uncovered problems:
* formatting of trace output
* package & import directives need to be classified as declarations
* the problem that meant the previous problem wasn't caught by existing
tests (since `Tree::Verify` didn't check that top-level declarations
match `AnyDeclId`, as required by `Tree::ExtractFile()`).
Previously would fail with:
```
CHECK failure at toolchain/parse/node_kind.cpp:74: Lex::TokenKind::Error == expected_token_kind || token_kind == expected_token_kind: Created parse node with NodeKind NamedConstraintDecl and has_error 1 for lexical token kind Constraint, but expected token kind Semi
```
Issue found by fuzzing.
These are intended to allow the structure of a parse tree node to be
described more precisely in code, to support these use cases:
- Automated checking that the parse tree conforms to the expected
structure. (Added to `Tree::Verify`.)
- Easier reading and understanding of the structure of the parse tree by
toolchain developers. (See `parse/typed_nodes.h`.)
- Easier navigation of the parse tree, for example for tooling uses and
for use when forming diagnostics.
On this last point, an object representing the file may be inspecting
using `Tree::ExtractFile`, as in:
```
auto file = tree->ExtractFile();
for (AnyDeclId decl_id : file.decls) {
// `decl_id` is convertible to a `NodeId`.
if (std::optional<FunctionDecl> fn_decl =
tree->ExtractAs<FunctionDecl>(decl_id)) {
// fn_decl->params is a `TuplePatternId` (which extends `NodeId`)
// that is guaranteed to reference a `TuplePattern`.
std::optional<TuplePattern> params = tree->Extract(fn_decl->params);
// `params` has a value unless there was an error in that node.
} else if (auto class_def = tree->ExtractAs<ClassDefinition>(decl_id)) {
// ...
}
}
```
The `Extract...` functions collect the child nodes into the typed parse
node's fields (internally using a `Tree::SiblingIterator`) for easy
access. However, this is not as fast as directly observing the tree
structure using the postorder strategy being used by the check stage.
These functions rely on using struct reflection on the typed parse node
definitions from `parse/typed_nodes.h` to get the expected structure of
child nodes and then populate them.
Note that validating these in `Tree::Verify` adds significant cost to
it, and is currently included in the parsing stage. Without this change,
a 10 mloc test case of lex & parse takes 4.129 s ± 0.041 s. With this
change, it takes 5.768 s ± 0.036 s.
This builds upon and completes #3393.
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Move handling of mismatched brackets out of the main lexing loop into a
separate pass that is only run if there are mismatched brackets This is
done in preparation for using both lookahead and lookbehind to work out
how to match brackets, and to get this code far away from the hot lexing
loop.
Fix bracket insertion location to be immediately after the token that
we're inserting the bracket after, rather than potentially at the end of
a comment. When there are open brackets at the end of the file, say that
there are open brackets, not that there's a closing bracket without a
matching opening bracket.
We have `StringLiteral`s in multiple other `Carbon` sub-namespaces.
Rename to a more specific name to avoid collisions.
We should likely also rename `Carbon::IntId` -> `Carbon::IntValueId` and
`Carbon::RealId` -> `Carbon::RealValueId`, but this collision is
prioritized because it was blocking work on typed parse nodes which
introduces a `Carbon::Parse::StringLiteralId`.
This builds on the series of changes to NodeKinds, aiming to simplify
the NodeKind implementation a little, also making it clearer that
there's a single associated token for each parse node (or, for
placeholders/invalid parses, not validated).
Note that prior to the relevant changes, there were nodes with multiple
tokens. This change is also locking in the approach of one token per
parse node, by refactoring macros to stop supporting multiple.
Note #3486 rewrites the macro behavior, and is already approved: so this
PR is only for the changed enforcement during error. Also, #3493 already
changed several things to allow any token while this PR was awaiting
review, but this still changes enforcement for `For` and `If`.
This was brought up on
[#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1182066616456970251),
and I think this any-on-error approach gets at least some support. We
could try setting it to the introducer, but it's quite possible we want
it to be something like the token which led to the parse error, rather
than a static token. That leads to a conclusion that, most typically,
we'll expect arbitrary tokens when error conditions may lead to tokens
which aren't the expected token.
A couple related, recent `CARBON_IF_ERROR` crash fixes can be found in
#3404 and #3424. Something like #3404 would've been needed regardless
because `namespace` didn't have `CARBON_IF_ERROR` before, although I
might've missed the underlying issue with declarations because only
`namespace` had a relevant test (that is, if #3404 had added
`CARBON_ANY_TOKEN_ON_ERROR`, I wouldn't have had a crash in #3462).
#3424 would've been avoided with this change because there was a
`CARBON_IF_ERROR`, and it was just too restrictive.
This leaves a single state for each in the expr loop. I was trying to
think through ways to have per-token states, but they felt sort of
bulky.
Note this is more verbose: but I think the long-term is going to be that
when we start wanting to add handlers, we're going to need to switch to
different names based on the token found. As a consequence, the parse
state logic will end up diverging a little, and we'll just want to align
towards boilerplate handlers.
Short-term, this opens up a path for saying that each parse node
corresponds to precisely one token in success states, and separates out
what were becoming big handler functions in check.
Another incremental change to diagnostic formatting. I simply recurse
over all the tokens in the subtree of a parse node and construct a
`DiagnosticLocation` that covers all of the tokens.
I believe it's nicer for the user to be directed at the entire chunk of
source where the error is occurring rather then just pointing at the
bracketing/terminator tokens, but let me know if you all agree.
StructFieldUnknown was used previously for invalid parses. But we have
added other, more common ways of talking about those; so this is
removing the special-case.
This remains structurally valid although we don't use this operator or
have a design for it, this at least fixes a fuzzer-found crash.
---------
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
This is also doing the parse node split, allowing lower reliance in
formatter on the tokenized buffer (something that I may be touching more
due to import handling).
(Split out of #3410)
Consistently use `ParenExpr` solely for parenthesized single
expressions, and use more syntax-oriented terminology for states and
nodes that might represent either a `ParenExpr` or a tuple literal.
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.
Building on #3462, trying to make the flow easier to see, also making a
little more use of macros for boilerplate modifier handling.
---------
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
In theory because none are allowed. This is to improve consistency in
handle_decl_name_scope's modifier handling, removing the namespace
special-case.
I noticed there's a crash bug on `impl <declaration>` which I'll address
separately.
This builds on #3461.
This is supporting a direction that all parse nodes should correspond to
a single token, allowing for reduced tokenized buffer access during
checking (it's still necessary for diagnostics, and some literals).
One of the justifications for a unified parse node was implementation
LOC: note this is slightly smaller, using macros to reduce some
duplication. While this does add more switching in HandleDeclScopeLoop,
that's offset by less explicit switching in the check handlers. Also, I
think the duplication in HandleDeclScopeLoop can be reduced by shifting
the flow there, which I'll do in a separate PR.
This creates a namespace for `package` scope.
It looks like names of class_decls in namespaces lead to an unexpected
instref. This is already true, as best as I can tell. I'm not sure if
there's a preferred approach to address that, so I've left a TODO for
now.
This reflects how we're naming classes that derive from these classes,
and matches usage for each existing `Id` and `Index` type, except:
- `Parse::NodeId` previously inherited from `ComparableIndexBase`, and
is no longer comparable.
- `SemIR::MemberIndex` previously inherited from `IndexBase`, and is now
comparable.
Making `Parse::NodeId` non-comparable reflects that it's intended to be
an opaque identifier for a node and that the ordering is an
implementation detail rather than part of the intended public interface.
`PostorderIterator` and `SiblingIterator` still rely on the numerical
meaning of `NodeId`s, but that's OK since they're part of the node
implementation.
The main difference I'm aiming for is that clangd doesn't complain about
the struct being unused, but it does miss the function's use. But really
these are specific to formatv for diagnostics, so this is more clearly
marking such, and probably makes for a better pattern for the future.
I was suggesting this because `FloatingPoint` is pretty long. `int` and
`float` should be familiar abbreviations. `unsigned` should be familiar
to developers too, but `UnsignedInt` still feels usefully clearer for
the additional chars.
These are manual fixes; mostly from clang-tidy, some from clangd (which
notes unused includes).
In typed_insts, adding inlline due to misc-definitions-in-headers. Per
discussion, clang-tidy is wrong, but inline silences it.
For parameter name skew in definition versus declaration, I'm just using
the name from the definition.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This gets to a lifetime subtlety, particularly with things like the
sorting diagnostic consumer that delay output. In order to reduce the
chance of accidental references, disallow StringRef in the diagnostics.
For example:
```
./toolchain/diagnostics/diagnostic_emitter.h:162:5: error: static_assert failed due to requirement '!std::is_same_v<llvm::StringRef, llvm::StringRef>' "Use std::string or llvm::StringLiteral for diagnostic lifetimes."
static_assert(
^
toolchain/check/convert.cpp:477:11: note: in instantiation of member function 'Carbon::Internal::DiagnosticBase<std::string, std::string, llvm::StringRef>::DiagnosticBase' requested here
CARBON_DIAGNOSTIC(StructInitMissingFieldInConversion, Error,
^
./toolchain/diagnostics/diagnostic_emitter.h:47:7: note: expanded from macro 'CARBON_DIAGNOSTIC'
::Carbon::Internal::DiagnosticBase<__VA_ARGS__>( \
^
```
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Per [#toolchain
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1176632520834560211)
We'd at one point been trying to put `[[nodiscard]]` everywhere, but
then we stopped because it had felt verbose without finding many issues
(plus, people plain forgot to add it). Some history in #888.
Since newer code gets added without it, we now have code like:
```
auto GetLineInfo(Line line) -> LineInfo&;
[[nodiscard]] auto GetLineInfo(Line line) const -> const LineInfo&;
auto AddLine(LineInfo info) -> Line;
auto GetTokenInfo(Token token) -> TokenInfo&;
[[nodiscard]] auto GetTokenInfo(Token token) const -> const TokenInfo&;
auto AddToken(TokenInfo info) -> Token;
[[nodiscard]] auto GetTokenPrintWidths(Token token) const -> PrintWidths;
```
Here, the lack of `[[nodiscard]]` doesn't mean anything: for example,
`GetLineInfo` should not have its result discarded if it's called. But
the mix could be confusing for readers.
As a resolution, remove the attribute. `[[nodiscard]]` should be treated
like other attributes going forward, which essentially means "avoid in
general, add a comment to explain why the attribute is needed" rather
than use-as-default.
Jon Ross-Perkins
josh11bandChandler Carruth
Jacob Schneider
Geoff RomerandRichard Smith