The parse nodes are still tracked as part of the same value store
interface in order to ensure parity, but they're split out from Inst
itself in order to reduce the size of Inst -- the expectation is that
they don't need to be passed around quite as much.
This change doesn't actually reduce the passing very much, although
there are hints of it: AddInstAndPush doesn't typically need a separate
parse node from the one on the Inst itself, for example. In a couple
spots I changed code to rely a little more on the InstId until the
ParseNode is needed, but it's very low hanging fruit where done. I think
convert could do more to not eagerly fetch the parse node before its
use, but more cleanup felt it would be easier to handle separately. I'm
currently viewing this as making such cleanup _possible_ rather than
executing on it up-front.
But also, I want to make sure there's a consensus to head in this
direction before pulling the trigger. We speculated that this would
result in the parse node being passed around less, and I do think that's
the case, although it's a bit fuzzy in the change.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This is currently built on top of #3575. I'm just getting to the point
where imports have enough logic that I'm thinking they may be able to
stand on their own.
Namespaces are copied, which means also adding their name to the
underlying instruction. It happened not to be done previously; the name
was only in name lookup.
Since the only import supported right now is the default import,
functionality is limited; in the future I'll need to deal with namespace
vs package conflicts.
Tests of namespace imports are under "namespace" -- I figured this would
be best for scaling as more instructions get support.
This also improves some debugging-related output that I was trying to
use while trying to build the support.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This is a step towards adding enclosing scopes for imports. It creates
an indirection for all bind names.
We discussed specializing for bindings that are in function scope (i.e.,
not a useful enclosing scope for imports or diagnostics). However, the
thought is to go ahead with this singular approach for now, and only
change structure if it's a performance issues so that we have
incrementally fewer instructions to handle.
This sets things up to use `bazel` to run `clang-tidy` using
https://github.com/erenon/bazel_clang_tidy.
I'm fixing issues outside of explorer, and disabling clang-tidy for
targets in explorer that have legacy issues. I was going to disable
clang-tidy for targets in explorer such as interpreter anyways, because
they're slow to parse, and just extended that to the currently failing
targets.
This adds a block for exported InstIds, rather than scanning the package
scope. This working down a path discussed last month, which we'll need
to add enclosing namespaces to the Inst in order to complete import of
something like `namespace NS; var NS.a;`
Exports could've been a separate `vector<InstId>` on `SemIR::File`, but
using an entry in `inst_blocks` felt more consistent.
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>
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>
By adding an InstId to the NameScope, we can determine whether the
declaration is being added to a scoped entity (versus a namespace).
The choice of InstId on NameScope is chosen versus other solutions
because, for imports, we want to just have a list of InstIds to import
and, from those, get the containing namespaces for addition. Similar may
also be desirable for printing fully qualified names given a singular
InstId. That means an InstId must have a path to find enclosing name
scopes.
What we're looking at here is:
- NameScopeId knows its InstId. (done here)
- Inst knows the enclosing NameScopeId. (future work)
- To walk up enclosing scopes for an Inst:
1. Fetch the Inst.
2. Find its enclosing NameScopeId (which will be per-declaration due to
Function etc complexity).
3. Fetch the NameScope if not Package scope. (if Package scope, done)
4. Use the InstId on the NameScope to go back to step 1.
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 out a little infrastructure for one name scope to `extend`
another. We'll need more refinement here to cover other cases, but this
should provide some foundation for that future work.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
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.
Adds a `LazyImportRef` instruction. Versus `CrossRef`, this is intended
to represent an instruction which cannot be used directly, and must be
replaced when it comes up due to name lookup. The intent is to use this
to avoid recursive loading of imported IR instructions.
Note, under this model, when `ResolveIfLazyImportRef` is called, it
essentially needs to load both inst and type information to a sufficient
point where any further attempts would hit name lookup again. That will
probably be complex, and the current implementation is just touching the
surface of the issue. I was heading down this route because it would
mean we have a limited number of points that need to consider whether
they're going to talk about a `LazyImportRef`.
I'm considering whether `CrossRef` should be dropped in favor of more
specific `Builtin` special-casing, due to the divergence of desired
behaviors. This could mean dropping the `builtins` IR since it's not
looking useful right now.
Modify `NameScope` to track whether the scope is associated with a load
error. This is to handle cases where one or more imports failed, so we
do not want to issue warnings for related scopes.
The 0-size on `ValueStore` comes up due to the changes to `NameScope`,
which make it too large for the default handling. After discussion with
zygoloid, the thought was we might want to try reserving a roughly
correct value based on parse node counts, but the stack default wasn't
buying much.
Fixes a bug where the implicit import used the package name instead of
the invalid identifier.
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 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>
This adds instructions so that we get printing. I may adjust the
instruction format a little further to add a type, but I think the basic
setup will remain.
Note this builds on #3414
- Treat an input of `-` as meaning stdin.
- Fix building of an llvm::MemoryBuffer from a non-regular file.
- Do not enforce filename restrictions on non-regular files.
- Do not invent an output file name based on the name of a non-regular
file.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This should cover:
```
library "lib" api;
import Foo library default;
import library default;
import library "lib";
```
This splits out `PackageName` and `LibraryName` to their own parse nodes
so that checking can ignore them and still get a balanced parse tree
(otherwise, we essentially need to implement handling of the parse nodes
only to remove the identifiers/string literals -- the optional names
mean we can't blindly do that as before). For reference, these nodes
don't need to be handled because CheckParseTree will need to directly
funnel import information along with checked IRs.
As I was working on this, I noticed `import` and `library` syntax needs
to be fixed for how it imports the current package, and for `Main`
libraries. This mostly reflects the current state in its testing.
Otherwise, this should handle most of the errors I could think of:
dependency cycles, redundant imports, etc.
It does not actually deal with the nuances of cross-IR references.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
1. In general, `semantics_ir` -> `sem_ir`, to match the directory name.
2. For the list of `ValueStore`-related accessors on `SemIR::File`, add
them to `check`'s `Context` object, shortening access.
Building on #3311, change SemIR to use the SharedValueStore. Since this
removes hermeticity, raw output no longer prints ints, reals, and
strings. TokenizedBuffer accessors are modified to return IDs because
values are often passed through in semantics without needing to read
them.
I would've put SharedValueStores on Context, except for the
GetArrayBoundValue convenience method. I felt awkward removing that, so
it's on File, at least for now. That's then used by the formatter and
Lower too. The flipside of this is that TokenizedBuffer has a
SharedValueStores only for printing, so maybe that's similar enough to
what File is doing.
This doesn't start shifting other SemIR members to ValueStore, but that
seems like a next step.
Building on #3214 and #3215, updates sem_ir yaml to be:
```
- filename: name
sem_ir: [ ... ]
```
Also, changes the textual format from `package { ... }` to `file
<filename> { ... }`. My thought on packages there is:
```
file "foo.carbon" {
package MyPackage
...
}
```
The reason for putting the file first is that it's easier if we put what
we're grouping on first, whereas the package is an "annotation" on the
file.
Continuing with #3070. Just a dir and file rename (mostly removing
prefixes, although for parse_tree_fuzzer and parse_tree_file_test I'm
dropping "tree" instead of "parse"). Everything in the parse dir should
be marked as a move.
Continuing along with #3070. Note this is just a file rename, with BUILD
edits; every file previously in semantics/ should show as moved (except
maybe BUILDs, which split).
Jon Ross-Perkins
josh11bandChandler Carruth
Jacob Schneider