I'm looking at this as I start thinking about handling `import`. Syntax is based on llvm's `split-file` tool.
The `std::vector` -> `llvm::SmallVector` switch is minor, I'm doing it here because I had to touch everything anyways and I think for tests I'll lean slightly more towards the toolchain's way of doing things versus explorer's.
Allow an `impl` to overlap with a `final impl` if they agree on the overlap. Agreement is defined as all values comparing equal, and functions never comparing equal. Implements the decision in question-for-leads issue #1077.
This shifts logic so that bindings are added to name lookup only after the scope is complete, removing logic around adding/removing/re-adding names in certain scopes.
This does mean that things like a function's forward declaration will need to go through an extra hoop for name conflict checks, because under this approach a function definition does conflict checking when it adds names for the body's use. But, that seems easy to address, and better than the current hoops.
I'm looking at adding CallableId, so figured I'd do this API refactoring which should reduce code duplication. This increases the amount of cross-calls between APIs because it may not be an issue for performance after optimizations, and should simplify reading of the API.
Also note the prior static_asserts on layout were missing a couple types, which is why I'm moving them into Entry where it's going to be more obvious when something's added.
The "locals" naming is intended to reflect that I haven't really thought through how globals work, but suspect they're going to end up at least partially separated. Stepping away from the "node" naming feels like it'll reduce confusion, although maybe "values" might be better? (but values seems imperfect in the presence of globals)
The map is the more important part here; I want to avoid anchoring on the prior vector approach.
Start inserting loads when getting a value. This is done conditionally, but in theory we could start tracking nodes which need loading differently if the `isa` is considered cumbersome.
I believe this broke when I was changing path handling.
Bad:
```
bazel test //testing/file_test:file_test_base_test.subset --test_arg=/usr/local/...elided.../execroot/carbon/bazel-out/k8-fastbuild/bin/testing/file_test/file_test_base_test.runfiles/carbon/testing/file_test/example.carbon
```
Good:
```
bazel test //testing/file_test:file_test_base_test.subset --test_arg=testing/file_test/example.carbon
```
As I've worked on this more, it's just felt verbosely redundant -- lowered should be the default assumption, needing no further explanation. This does mean there's `context.GetType()` and `context.semantics_ir().GetType()`, but I feel like that's still reasonably clear on reading.
This is a rename of a commonly used accessor. I tend to prefer shorter names, but it's semantics_ir in LoweringContext, and it feels odder to shorten to semantics there than to lengthen to semantics_ir here. I already have builtins_ir around here, too. I think it's helpful to use consistent names for semantics_ir since they're dealing with essentially the same thing.
Per #257, we should be treating unformedness as all-or-nothing, rather than being a per-field or per-array-element property. Previously we initialized an array with no explicit initializer as containing a sequence of uninitialized values, but that led to crashes when attempting to access those values, as the checks for reading an uninitialized value only expected values to be uninitialized at the top level.
Also, we had existing tests that attempt to store to an element of an uninitialized array. We now detect that and treat it as UB during evaluation, rather than crashing due to trying to perform field access into an uninitialized value.
Finally, many of these problems can be detected statically, but the resolve_unformed pass wasn't catching them because it missed a few expression and declaration forms. Support for those cases has been added too. This causes the pass to recurse more often, and in particular our existing recursion test started hitting a stack overflow after this, so resolve_unformed now uses `RunWithExtraStack`. In passing, remove the need to explicitly tell `RunWithExtraStack` the return type, and infer it as the return type of the callable instead.
This adds canonicalization of struct types based on their type fields. It obsoletes the current CanImplicitAsStruct because the type ids should now be identical when they're structurally identical; there's only a reason to implicit CanImplicitAsStruct to detect _compatible_ conversions.
The type fields themselves aren't canonicalized because it would need to be done during the first parse, and could yield name conflicts being associated with the wrong location. i.e.:
```
var x: {a: i32, a: i32};
var y: {a: i32, b: i32, a: i32};
```
This should yield two separate name conflict diagnostics pointing at the type fields for each respective line, but if struct type fields were canonicalized then both would point at the first `a: i32` field definition. This isn't expected to be an issue for types because I'm trying to print those, but we may also end up with a "first defined at" situation in some cases (still, less confusing because the type should match). Regardless, I think individual fields gets much more awkward.
This switches types to using SemanticsTypeId instead of SemanticsNodeId, and lowering pre-builds its list of types. The empty tuple type is special-cased because we don't want to emit it unless it's in-use, but as the implicit return for functions, it's frequently used. Callables use invalid to indicate the implicit return, and that seems undesirable to change due to the size increase.
We do use the empty tuple type for function returns, so I don't want to get rid of it, but the value is unused. With this change, builtins are all types. Long-term the empty tuple value should have a representation similar to structs; just a tuple value that's empty, not a built-in.
This removes special-casing of empty structs, handling them as just a regular value instead of a builtin. Note the `{} as Type` is still special-cased.
In lowering, removes the test of calling a function using `{}` because it's missing the proper load/store. This setup notices that error whereas the prior worked due to said special-casing. Fixing this will need to be done as part of generally adding loads for variable uses.
This is the first step to refactoring types into a SemanticsTypeId. This only tracks what's in-use, but as a consequence starts funneling type information through in ways similar to how I'd want it to do SemanticsTypeId.
As I went through and analyzed the Carbon source code, I saw that there is typo in the comments, which may need to be modified to avoid misleading other people.
The old version wasn't compatible with ASan's instrumentation. Instead,
use a builtin when available, and even in the fallback disable inlining
and sanitizing as they can't do anything useful in this routine.
This fixes a confusing internal ASan error seen by some folks since the
stack detection was added.
I'm not sure this is really the best approach, as noted by the TODO. While it does reduce the generated IR in many cases (especially if we start eliding `()` in favor of void), it increases the amount of code in a common function which may have higher impact on performance. I was leaning towards the idea that review would favor the former for now, lacking benchmarking supporting adding unused content to the IR.
There was a warning "Unchecked access to optional value" in this piece of code inside `action.cpp`
```cpp
if (this->scope().has_value()) {
out << " " << *this->scope();a
}
```
Fixed it by accessing `scope_` directly rather than using `this->scope()`.
Adds stack space handling similar to Clang's approach, but with more support for forwarding return values. Refactors ParseAndExecute for better sharing, and for centralization of the InitStackSpace handling.
Fixes#2795
This is addressing an issue left behind by the context switch, removing a few diagnostics that had been in the header rather than figuring out proper homes. I'm splitting one for semis a little further, sharing one, and then the other two are actually able to be moved into more specific homes as-is (one is only used in one place, clearly an oversight that it wasn't there already).
This adds names for more LLVM IR entries. For now, `var` is just a placeholder due to the difficulty of associating a name, but the rest reasonably reflect intent.
I'm trying to comment logic for this because I suspect we'll want to make it more conditional later. Even though chandlerc noted the expectation that IRBuilder would have a way to disable them in output, that's fine for things like anonymous names; other places they'll still take extra work to calculate. e.g., as with struct fields where the type's fields are only fetched to print a name, not otherwise needed for the gep.
Previously, IR for arguments in calls and struct values was separated out. This merges it back in. Additionally, parameters for functions and struct types had their own IR; the block is still there, but there's a TODO to decide what to do with it.
In the LLVM IR, this has the consequence of emitting expressions that are inputs to a call or struct value within the scope of the function, which is pretty much where it should be. Importantly it happens before the call is encountered.
This change also tinkers with the int and real literal lowering. I'm pretty sure both are still wrong, but was having trouble figuring out a "better" way to do it, and this seems like it'll work for now.
The intent of this change is that instead of paths looking like `explorer/testdata/foo/bar.carbon` (repo-relative), they're now just `bar.carbon` (local). The consequence is that paths should be a bit more durable in various environments, and just paths should be shorter and easier to read.
The explorer's prelude is an exception to this since it comes from data, rather than being the test target. Due to the change in approaches, it needs the regex again.
Uses #2829
This is a convenient test helper for anything that can use injected
streams. Extract this to where it can be used by other tests and add
some basic tests, mostly documenting how it works.
This currently handles most of struct types and member access, but not values. The value issue is, I think, an underlying semantic IR approach (changed in #2824). I'd still like to get this in in order to ensure I'm creating IR at least reasonably well, but I want to be clear this is expected to be incomplete and is split out mainly to try to keep PRs in reasonable units.
- Moves most parts to //testing/lit_test to be consistent with //testing/file_test.
- Separates the autoupdate script out because it's shared between lit_test and file_test now, not lit-specific.
- Renames scripts to autoupdate_testdata (or autoupdate_lit_testdata for explorer's extra) to be more consistent with the non-lit-specific setup.
- Switches from execv to subprocess.call to head off a subtle issue regarding execution of multiple scripts, which we're likely to want in the future. Mostly in this PR because everything was already being touched.
- Removes autoupdate's dependency on merge_output in order to (a) better support the division of lit and non-lit logic and (b) remove a subprocess, for reasons similar to file_test's removal of subprocesses.
Isolating test execution time (no build time included):
- Linux, `lit` test-per-file: Elapsed time: 28.214s, Critical Path: 13.97s
- Linux, `cc_test`-per-file: Elapsed time: 11.534s, Critical Path: 6.05s
- Linux, merged `cc_test` with 50 shards: Elapsed time: 11.677s, Critical Path: 11.17s
- Mac, `lit` test-per-file: Elapsed time: 295.686s, Critical Path: 20.00s
- Mac, `cc_test`-per-file: Elapsed time: 55.788s, Critical Path: 3.81s
- Mac, merged `cc_test` with 50 shards: Elapsed time: 16.269s, Critical Path: 7.54s
In GH actions:
- [Before](https://github.com/carbon-language/carbon-lang/actions/runs/4866602695/jobs/8678306144?pr=2799):
- test / test (ubuntu-22.04, fastbuild) (pull_request_target) Successful in 20m
- test / test (ubuntu-22.04, opt) (pull_request_target) Successful in 15m
- test / test (macos-12, fastbuild) (pull_request_target) Successful in 36m
- test / test (macos-12, opt) (pull_request_target) Successful in 21m
- [After](https://github.com/carbon-language/carbon-lang/actions/runs/4875154751/jobs/8697004066?pr=2811):
- test / test (ubuntu-22.04, fastbuild) (pull_request_target) Successful in 10m
- test / test (ubuntu-22.04, opt) (pull_request_target) Successful in 9m
- test / test (macos-12, fastbuild) (pull_request_target) Successful in 12m
- test / test (macos-12, opt) (pull_request_target) Successful in 9m
I'm still leaving a handful of `lit` tests to test end-to-end binary execution. This is why testdata directories are split (`lit` tests next to the `explorer` binary, the `cc_test`s next to `ParseAndExecute`).
This echoes https://github.com/carbon-language/carbon-lang/pull/2818 and the philosophy is mostly covered there.
This isn't necessary for lowering right now, but things are likely to head in that direction and maintaining the context boundary will reduce blurring of lines. For now I'm just putting all the handlers in one file.
This echoes #2818 and the philosophy is mostly covered there.
Versus parsing, semantics uses fewer separate handler files (for now) because the logic has been shorter. However, the design is still intended to make it easy to split files along boundaries similar to the parser, as I've done for a couple more complex/inter-related sections.
The goal of this change is to start refactoring the monolithic file into separate files that will hopefully pose fewer conflicts for developers, and make it easier to skip to handling of specific functionality. It additionally addresses a scaling issue with parser.cpp where the file would continue to get larger as more features are added.
Switches Parser to a ParserContext, moves handlers to be free functions, and moves the controller logic into ParseTree. parser_handle_states.h does the declarations for handlers and little else; handlers are split out to individual files based on prefix (which is deliberately authored to cluster).
A couple things I'm avoiding based on historical discussion are:
- Having a subdirectory for all the handlers, such as `toolchain/parser/handlers/call_expression.cpp`
- Putting handlers in a namespace, such as `Carbon::ParserHandler::CallExpression`.
- The name of `Carbon::ParserHandlerCallExpression` is then necessary to minimize the chance of conflicts with semantics and lowering, where everything can be expected to be named similarly.
I'm globbing handlers because it seems hard to see missed ones under this approach -- names are too boilerplate.
I think this current setup could be split into target-per-file, but I'm not sure that's needed, so I'd delay until it becomes a build-time issue.
This builds on #2814 by adding the test framework to the toolchain. On Linux, this is 7s -> 4s for me. #2811 has more detailed timing for the explorer, which also had more dramatic changes because it's about 3x more tests run in 2x as many ways (6x total).
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This is really part of #2811, but is extracted out to allow a little review in parallelism because #2811 expects #2813. Getting this in will allow migration of toolchain tests, whereas #2811 is focused on explorer tests. For explorer test timing information, see #2811.
The syntax being used for matching deliberately mirrors the `FileCheck` setup, partly for compatibility if something changes, partly so there's nothing new to learn, partly so that we don't need to build more test updating.
Individual tests look like:
```
[ RUN ] ParseAndExecuteTestFile.explorer/parse_and_execute/testdata/assert/convert.carbon
To test this file alone, run:
bazel test //explorer/parse_and_execute:file_test.subset --test_arg=explorer/parse_and_execute/testdata/assert/convert.carbon
[ OK ] ParseAndExecuteTestFile.explorer/parse_and_execute/testdata/assert/convert.carbon (202 ms)
```
The printed command line is intended to assist developers in debugging a single test, particularly when sharding the main test. The use of a single `.subset` target means the total number of targets is constant even as the number of test files increases, which may be important for some `bazel` execution environments. I plan to make similar changes to the `glob_sh_run` implementation so that we have consistent setups, i.e. that we no longer create target-per-file scaling risks.
This uses `native_test` to share the test binary, avoiding re-linking if files are individually run.
Investigation did reveal a mistake where STDOUT/STDERR wasn't prefixed on empty output lines; this PR fixes that mistake, so that output is fully covered.
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Currently ParseAndExecute-style logic is done in main.cpp and a test. #2799 is adding another test that needs it, as is #2811.
Also more clearly marks fuzzing as testing.
This is being extracted out of #2799 in order to try unblocking progress while review continues.
Adrien Leravat
Jon Ross-Perkins
Prabhat Sachdeva
Richard Smith
josh11b
Aswin ShailajanandAvi Aaron
Geoff RomerandRichard Smith
CanftIn
Chandler Carruth
kshokhin
Zhou
Amr Hesham