I was thinking about this after `seq` changes in #5182, and looked for
other uses that might be replaceable. Here's the resulting cleanup
around `seq`:
- Switch to `enumerate` or `zip` when possible.
- `int _` -> `auto _` (it's typically a `size_t`, but there's no reason
to cast when unused)
- Fix a case of cast style `(size_t)...` -> `static_cast<size_t>(...)`
- Switch `(void)close_children_count` to `[[maybe_unused]]`
It's helpful for stability to not have the path in the repo reflected in
test files, something I'm separately running into. So to reduce this,
align INCLUDE-FILE with other split behavior:
- Use the filename (with a "include_files/" subdir to disambiguate),
rather than the full path.
- Note if we eventually want to support splits in these, the same
approach could be extended.
- Only provide as an arg if the user requests files as args.
Factoring AddFile back because it's hard to share; I'm also advocating
to remove the prelude manifest, which would mean the remaining call
could be removed.
Currently file tests for LSP must provide carbon source code as an
escaped string within notify params, i.e.
```
[[@LSP-NOTIFY:textDocument/didOpen:
"textDocument": {
"uri": "file:/class.carbon",
"languageId": "carbon",
"text": "class A {\n fn F();\n fn G() {}\n}\n"
}
]]
```
This works fine for simple, single line files but gets annoying when
working with more complicated files which are necessary when testing
more complicated features e.g. goto-definition
```
--- class.carbon
class A {
fn F();
fn G() {}
}
--- STDIN
[[@LSP-NOTIFY:textDocument/didOpen:
"textDocument": {
"uri": "file:/class.carbon",
"languageId": "carbon",
"text": "AUTOFILL"
}
]]
```
This PR extends file_test_base to be able to parse the notify/call
params and inject files from the test_file's splits into the JSON input.
I purposely avoid using the clangd types and manually parse the
llvm::json::Value here to avoid introducing a depdendency on clangd to
the generic file_test_base, but happy to change if we think that is
fine. Also happy to accept other suggestions on alternative methods to
achieve same result.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
The INCLUDE-FILE option is only used in the toolchain tests for now. If
specified in a file test, the given file path is added to the test's
arguments. For toolchain tests this makes the file's package available
to the test. The `--custom-core` command line flag is added to the
driver, which avoids adding the production `Core` package to the command
line. Together, these allow a test to provide their own minimal `Core`
package.
For example, this would replace `Core` with the package and prelude in
`facet_types.carbon`.
```
// INCLUDE-FILE: toolchain/testing/min_prelude/facet_types.carbon
// EXTRA-ARGS: --custom-core
```
To support this:
* //testing knows how to parse INCLUDE-FILE out of the header of a test
file.
* //testing adds the file to the virtual file system, and includes it in
the test's arguments.
* //toolchain/driver grows the --custom-core command line flag to avoid
loading the production `Core` package.
Tests that were creating their own minimal prelude to define BitAnd on
types are now pointed to
toolchain/testing/min_prelude/facet_types.carbon as the prelude. They no
longer need to `import Core` in each test as a result.
Such tests are no longer `no_prelude`, but instead have their own
prelude. So they are moved to a `min_prelude` subdirectory.
Closes#5076
This narrows the scope of the CRC to try to get better behavior around
mutex lock releasing on crash. Closes#5042.
This breaks apart `ProcessTestFileAndRun` because we need to process the
test file for `SET-CAPTURE-CONSOLE-OUTPUT`. The test file processing
should more reliably not crash than the core `Run` logic though, so
should be reasonably safe to put outside the CRC.
Also support --threads=1 for disabling threading. This is the flipside
for me of reducing how much is in the CRC: make it easier to run on a
single thread if the CRC gets in the way of debugging. This also means a
typical copy-paste execution of a single test will be single-threaded.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Inconsistent execution environments make using a path-as-define
difficult, so switch to an embedded file.
Also fixes the lldb launch so that passing tests run cleanly, and adds
TEST_TARGET to gdb (but without testing there). I'm dropping `sourceMap`
because it's not handled quite correctly (also not great to be trying to
pass source mappings in two different ways), and `env` didn't seem to be
working as intended either; maybe specifying `initCommands` causes other
things to not be evaluated. But the straight `initCommands` looks like
it's working. I used lldb to validate execution of these changes.
```
Running initCommands:
(lldb) command script import external/+llvm_project+llvm-project/llvm/utils/lldbDataFormatters.py
(lldb) settings set target.source-map "." "/usr/local/google/home/jperkins/dev/carbon-lang"
(lldb) settings set target.source-map "/proc/self/cwd" "/usr/local/google/home/jperkins/dev/carbon-lang"
(lldb) env TEST_TARGET=//toolchain/testing:file_test
(lldb) env TEST_TMPDIR=/tmp
Running tests with 128 thread(s)
.
Done!
Note: Google Test filter = ToolchainFileTest.toolchain/check/testdata/const/collapse.carbon
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from ToolchainFileTest
[ RUN ] ToolchainFileTest.toolchain/check/testdata/const/collapse.carbon
[ OK ] ToolchainFileTest.toolchain/check/testdata/const/collapse.carbon (0 ms)
[----------] 1 test from ToolchainFileTest (0 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 1 test.
Process 3869310 exited with status = 0 (0x00000000)
```
We had a long discussion of this, so trying to document what seems to be
the conclusion... and also clean up the exceptions that I could find.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This takes the mechanism currently used for autoupdate and expands it to
the regular tests (deliberately trying to unify logic for
test/autoupdate/dump to deliver consistent behavior). I'm seeing about a
85% reduction in test time, though results will vary based on test
system.
This does some small edits to test output to make it fit better with the
new flow. Note I'm stopping printing of the "here's how to run" on every
test by default, since it's autoupdated into file content by default.
However, it's still there for test failures.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Noted CopyNameFromImportIR while glancing around (this one's interesting
because it's NameId, not void nor auto), did a scan just for a few other
cases. Not an exhaustive fix, and TBH assuming we'd prefer `auto ... ->
auto` since equivalent Carbon syntax would probably be `fn ... -> auto`
`ValidateRun` is explorer-specific, so move out the error production to
be specific within explorer. This is related to other work I'm trying to
do which would make this require more special-casing to maintain; since
the toolchain doesn't need it, it's easier to drop.
I'm refactoring logic to try to make these files more manageable,
particularly as I'm looking at ways to use more threads. I'm renaming
FileTestBase::TestContext to TestFile and FileTestBase::TestFile to
TestFile::Split to try to be more consistent in how we talk about test
files and file splits in general. This change is not expected to change
any behavior, it's just refactoring.
This PR has two commits:
- Copying files for viewing deltas in GH
- Moving logic
The delta of the two commits is the main PR. The second commit can be
viewed on its own to see the delta versus file_test_base.* files where
the logic currently rests (there's still a reordering in run_test.cpp as
part of making one function static).
This should make it easy to add more tests which need the specific
behavior here. It also isolates where we reach into GoogleTest's
internals to a single common place.
Small cleanup for code sharing. Note, `*ReadFile` will trigger a
CHECK-failure on error; the relevant implementations would previously
have failed silently (empty string).
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Also makes the style guide explicitly comment on void, but this was the
intent IIRC because it matches Carbon's `-> ()` (and "always" versus
"except for void", which we definitely went back and forth on).
Includes adjusting function pointers, which I definitely forget this
syntax works sometimes.
Excludes utils/tree_sitter/src/scanner.c because it claims to be C, but
really we should probably fix that to be cpp.
Adds @LSP-CALL and refactors LSP keyword handling to implicitly handle a
little more of the LSP structure. This is coming out of textDocument
call handling, where this at least reduces some boilerplate of
`"params": {...}`.
- Escape dumped token strings (what got me here)
- Change the quoting from backticks to quotes
- Also add a `FormatEscaped` helper function for this, updating other
`.write_escaped` uses
Switching from RE2 to StrReplaceAll because it seems a fair fit for what
actually needs to be done here. Also pick up \t for visibility reasons.
This came up because clangd's LSP-related APIs print carriage returns.
In order to write language-server tests, we need some way to pass stdin
input. This adds support for a split "// --- STDIN" which will be
provided as a temp file for testing.
Note this does more stdin -> input_stream style renaming, this is just
bugging me more since I know shadowing works but it can be subtle to
read, particularly since I'm now making direct use of stdin in a handful
of spots.
The language server needs stdin, and for tests we should be passing it
around. My intent is to pass in a faux stdin to Driver for language
server tests.
As long as I'm adding a new parameter, I was looking at also changing
the way streams are passed in to Driver for style (pointers since
they're held past construction lifetime). Since these are all stored in
DriverEnv, I thought it might be a net improvement to use the struct
directly, getting more explicit parameter names and also removing the
need for `SetFuzzing`.
I'm trying here to avoid functional changes, but there are a couple
additional fixes like removing an obsolete `find_insensitive` and
refactoring how `ValidateOptions` handles errors (because it reduces the
number of spots that operate on error_stream).
This adds a RawStringOstream. Versus TestRawOstream, which is
consolidated over to RawStringOstream, it uses a string for storage
instead of a vector, mainly to support move-to-string semantics. Versus
llvm::raw_string_ostream, it owns the string and supports pwrite (which
is needed for driver and its fd_ostream compatibility requirement).
This converts most uses of llvm::raw_string_ostream, leaving behind a
few in InstNamer that explicitly cannot own the string, such as:
```
llvm::raw_string_ostream(name)
<< "_" << tree.tokens().GetColumnNumber(token);
```
I have this as its own library so that it can use CHECK.
Yes this doesn't save much code, but it's code we repeatedly write.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Add `EXTRA-ARGS:` support to file_test, to add arguments without
overriding the default arguments. Use `EXTRA-ARGS: --no-dump-sem-ir` to
turn off SemIR dumping and thus SemIR testing in the int builtin tests,
which validate correct behavior through diagnostics instead.
This doesn't get us any closer to supporting more targeted SemIR dumping
/ testing, but this seems to be a generally useful feature anyway. Most
existing
tests using `ARGS` have been switched over to using `EXTRA-ARGS`.
Requested in review of #4716.
As part of migrating to the latest bazel configurations in #4729, I'm
running into proto toolchain issues. For example:
"Error: <target @@protobuf+//:cc_toolchain> (rule
'proto_lang_toolchain') doesn't contain declared provider
'ProtoLangToolchainInfo'"
Although we may eventually want more use of proto, right now the only
use is for the explorer fuzzer. The explorer codebase is essentially
frozen, so continuing to run it isn't gaining us much (in fact, we've
already disabled autofuzzing for it).
So, rather than trying to fix the proto setup, this change:
1. Deletes `explorer/fuzzing`
2. Removes proto portions of `testing/fuzzing`, which were only in-use
by the explorer
3. Removes some ancillary proto support, which would otherwise break
from the bazel changes and would be difficult to validate as "still
working"
This change is partly isolated in order to make it easier to revive bits
of (3).
I was wondering, instead of treating `misc` differently and enabling
specific checks, maybe we can flip that since we actually seem okay with
most of the checks?
The main check I'm enabling, with significant edits here, is
`misc-no-recursion`. But maybe this is helpful to enable, even with the
necessary NOLINTs, since we want to avoid recursion in the toolchain?
This PR shows some example fixes in subst.cpp (which are more stylistic,
since the code shouldn't actually have recursed due to its structure; I
think we could remove the warning on TryResolveInst the same way). Some
also just don't seem worth fixing, like those in tests files (I didn't
see a way to exclude files in .clang-tidy, so instead I'm using
NOLINTBEGIN). But I think we might actually want to fix inst_namer, and
there's enough in convert that I didn't look closely.
Also, I made some protected -> private style fixes based on
`misc-non-private-member-variables-in-classes` (this is also how I
noticed `class Real` versus `struct Real`). With node_stack, it looks
like the `protected` wasn't even used. [Per
style](https://google.github.io/styleguide/cppguide.html#Access_Control),
data members should be private outside tests. But since we can't
trivially exclude `protected` members in tests, I'm turning it off -- I
don't view it as offering enough benefit on the whole.
migrate_cpp issues are preexisting (I believe we just aren't monitoring
it), but changes there make `bazel build --config=clang-tidy -k //...`
work cleanly.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Disables three new warnings because they lean more towards style
conflicts than fixes. I've brought these up on #style.
Other than that, mostly fixing basic issues, and things that
clang-tidy-20 seems to fire where clang-tiday-16 didn't. One particular
curious case is `llvm::StringLiteral::data()` uses, which are flagged as
not strictly null-terminated; I'm switching to `const char*` in those
spots which matches `llvm::formatv`'s format argument, but feels worse.
I'm removing `run_clang_tidy.py` here because I'm observing it give
fewer warnings than `bazel build --config=clang-tidy -k
//toolchain/...`. The latter matches how we enforce in GitHub actions
(and also caches results, and suppresses output for files that have no
issues), so I'm dropping the bespoke script.
This is essentially the result of looking at `.begin()` uses. We also
frequently do `std::shuffle`, but unfortunately STLExtras doesn't
provide a wrapper for that.
This changes to an `Error` return to let the driver do the "error: "
prefix, except for one case with `help` that needs more work to change
(I'm not planning on picking up that TODO). It also changes
capitalization, backtick use, and a few minor punctuation things to try
to better match the diagnostic style.
This also adds `Error` matchers so that the changes to command line
testing are clearer.
As discussed in #4530 . This required switching to using
`llvm::IntrusiveRefCntPtr` in a number of places.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
This avoids producing an LSan leak report for the objects that got
leaked by the crash, which would otherwise scroll all the useful
information about the crash off the terminal.
Use the diagnostic kind printing in #4425 to catch when we have
diagnostics with no tests.
This merges a couple other use-cases of filegroup manifests into a
common rule.
Note I do add a few tests for things, and also some things are
_actually_ unit tested (just not in the file_test structure). But I
stopped when I realized that dealing with merge conflicts is going to be
a pain. I might end up reverting test changes (as part of merge conflict
resolution) and doing narrow test additions in a separate PR, after both
this and #4425 are merged.
Since formatting covers comments, and the CHECK lines are in comments,
it can create recursive behaviors. This introduces AUTOUPDATE-SPLIT as a
way of formally designating a split to exclusively be used for
autoupdate output.
One of the things that ClangRunnerTest is doing is capturing
stderr/stdout because clang prints to it directly. This adds support for
that to FileTest.
I'm renaming the current `capture_output` field to `dump_output` because
the name is ambiguous after this change, and the flag is already named
`--dump_output`. It's still not great, but at least it's more distinct.
Note ClangRunner still doesn't use the vfs; that still needs work. I'm
just moving the NoArgs test over as a trivial test of the functionality.
This switches `DCHECK` and `FATAL` as well.
The goal is to reduce the code size impact of these assertions so that
we can keep more of them enabled. Currently, the largest cost I see from
`CHECK` is not the actual check or the cold code itself, but actually
the failure to inline trivial functions due to the presence of the cold
code. This means that our goal isn't to reduce apparent code size in the
final binary but the LLVM IR cost assessed for these routines in the
inliner, which closely correlates with code size but is a bit different.
As discussed in #4283, experimentation shows that a single function call
with a minimal number of arguments is the lowest cost model for these.
This is easily achieved with a format-string API that internally uses
`llvm::formatv`. This PR is essentially the `CHECK` version of #4283.
However, the check macros are substantially harder to make work with
both format strings and streaming because they also take a condition.
Also, unexpectedly, I was very successful at devising a regular
expression based automated rewrite from the streaming to the format
string form with only low 10s of manual fixes. This includes compacting
strings broken up across lines, etc. Given how well that went, I've
prepared this PR which just directly switches to the format string API
and migrate everything to use it.
One nice side-effect is that the format string approach ends up greatly
simplifying the implementation here as well.
This is ... *shockingly* effective. Parsing speeds up by more than 3%
with just this change. And checking speeds up by **8%** with this change
alone:
```
BM_CompileAPIFileDenseDecls<Phase::Parse>/256 86.3µs ± 1% 82.9µs ± 1% -3.94% (p=0.000 n=17+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/1024 431µs ± 1% 415µs ± 1% -3.76% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/4096 1.77ms ± 1% 1.71ms ± 1% -3.18% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/16384 7.44ms ± 1% 7.17ms ± 2% -3.56% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/65536 30.7ms ± 1% 29.7ms ± 1% -3.15% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/262144 131ms ± 1% 127ms ± 1% -2.81% (p=0.000 n=18+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/256 878µs ± 2% 800µs ± 1% -8.91% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/1024 1.88ms ± 2% 1.72ms ± 1% -8.56% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/4096 5.78ms ± 2% 5.28ms ± 1% -8.70% (p=0.000 n=20+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/16384 21.9ms ± 1% 20.1ms ± 1% -8.02% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/65536 90.4ms ± 2% 83.1ms ± 1% -8.04% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/262144 381ms ± 2% 352ms ± 1% -7.79% (p=0.000 n=19+19)
```
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
This opens the door for replacing all `library ...` lines in toolchain
test files with `library "[[@TEST_NAME]]";`. That's technically more
typing in a lot of cases, but OTOH means we can just do some copy-paste
boilerplate and stop carefully writing library names.
Also cleans up the test setup, because it's getting messy. I'm trying to
make it easier to see the divisions of tests and the output associated
with them. StringSwitch looked like a way to do this, with a few edits
to make it work nicely.
From the driver's perspective, `FindPreludeFiles` is closely tied to
`compile`. This makes it difficult to refactor commands without
affecting the test dependencies on `FindPreludeFiles`. `InstallPaths`
seems like a decent home since it is responsible for the install
structure.
I'm switching to an `Error` return to allow callers to choose how to
handle it (e.g., in file tests, we typically don't want the direct error
stream).
This teaches our source generation tool to create interesting type
references. This include both referencing a weighted distribution of
explicitly specified types, and referencing types that are being defined
in the generated file.
Generating more interesting explicit types will exercise more of
Carbon's prelude, but because C++ doesn't have an automatic prelude with
fundamental types like `int64_t` or tuples, we include some minimal
headers when generating the C++ analog. This likely makes the comparison
more fair rather than less fair as Carbon's toolchain isn't processing
just the generated source, but also its prelude.
The current set of fixed types is based primarily on the set of types
that the toolchain currently implements and a set that seems reasonably
interesting to exercise for compile time performance. We want to try to
cover things that should be optimized in the toolchain, even if a single
source file might not typically hit all of them.
The weights of everything are completely arbitrary, based on intuition
and some hand inspection of some random source files. There is also an
intentional bias towards non-zero coverage and so the tail is much
larger than it should be in reality. The result is that the weights more
reflect the _priority_ of optimizing compile time than the _observed_
distribution in practice. We can refine the weighting scheme in the
future though, potentially with multiple modes to separate coverage from
maximally representative weights, etc. The goal is just to have a
starting point.
The scheme for referencing the defined types requires some care and
complexity to avoid referencing types before they are defined while
still referencing all of the types defined and ensuring the number of
references is stable even as the order is randomized to avoid fixed
patterns in the source code.
All of this also triggered some minor refactoring of the state used to
generate class definitions in the source generator. There are probably
some good follow-on refactoring opportunities, but I'd prefer to leave
those to future work.
I don't have any tests here because most of how this is observable is
already tested -- the existing tests ensure the file sizes remain
consistent and that the generated code is compiled correctly. But if
folks have any ideas of useful tests here, happy to add them.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Discussed in the toolchain meeting today - we'd like to try having this
on by default and see if the cost isn't too high.
The nodebug test is a bit verbose, because it doesn't have the
`--exclude-dump-file-prefix` that test_file would usually add. Is there
a nicer way I could write this test to verify that --no-debug-info does
what it's meant to?
Boaz Brickner
Jon Ross-Perkins
Dana Jansens
DavidLoftusandJon Ross-Perkins
Chandler Carruth
josh11bandJosh L
Brymer Meneses
David Blaikie