This PR removes `PrintDepth` from statement and declaration.
Implements `PrintIndent` for better indented formatting along with
various changes to make printing of statements and declarations better.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
I was initially looking at llvm::seq, but then looking through uses it
seemed like enumerate/zip would be best for these, allowing less direct
indexing and more range-based looping.
The main motivation for this is to get python loads in using the
`native-py` lint fix. However, enabling that made me wonder, maybe we
should fix in general?
`native-cc` is delayed, but not wholly cancelled (and `native-py`
picking up might indicate `native-cc` won't be too far behind). There's
also some automated fixes for `.append` and dict sorting -- this felt
okay to me, maybe not something to eagerly add but probably not worth
stopping buildifier from fixing (I've noticed the warnings in the past
and had been ignoring them).
Running everything does mean that load orders are sorted automatically
now, which I think is a positive. Most generally, I think these fixes
aren't _harmful_, and having them done automatically seems beneficial:
my biggest concern about `native-py` and `native-cc` was actually that
regressions wouldn't be caught, but this addresses that issue
automatically.
Makes the trace output for pattern match more consistent with the rest
of the trace by using `Match()` and `Indent()` prefix methods and
wrapping the code in the trace with backticks.
Instead of manually putting symbols before and after heading, this
introduces two methods `Heading` and `SubHeading` inside `TraceStream`.
Both methods take `llvm::StringRef` as parameter, formats the given text
as follows and adds it into the output stream.
**Heading**
```
* * * * * * * * * * heading * * * * * * * * * *
-------------------------------------------------
```
**Sub heading**
```
- - - - - sub heading - - - - -
---------------------------------
```
Note: both methods assert that tracing should be enabled.
Defines methods into `TraceStream` for adding line prefixes. Instead of
directly using string literals.
Example usage,
```
trace_stream_->Start() << "declaring ... " << ... ;
```
will result in,
```
->> declaring ...
```
The Prelude path can be very long, which can make the `SourceLocation`'s
`Print(...)` method output unnecessarily verbose. This PR modifies the
`SourceLocation`'s `Print()` method to just print the **filename** and
**line number** for prelude.
Allow interleaving of STDOUT and STDERR check lines. Put STDOUT lines
after the line they're attached to, and STDERR lines before. If no
STDOUT check line is attached to any line, then put them all at the end
of the file instead.
This is intended to better handle the case where stdout contains
unreplaced mentions of line numbers, and also reflects that stdout is
typically a consequence of the test rather than commentary on it, so
placing it after the test seems likely to read better.
Updates the trace output for type checking to be more consistent like
the rest of trace output.
This PR also includes few changes in the lit tests related to trace
output.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
The constructor accepts a `bool` which we were setting to `true` by
converting from a heap allocated, and thus non-null, pointer. But that
heap allocation was dead and leaked, somewhat obviously. Leak checking
is disabled on our CI at the moment, but this was failing for me locally
with our default build on Linux where it uses ASan.
The `true` value also had no effect because the default argument to the
bool parameter is itself, `true`. :sigh:
It's really sad that this compiled. But it's the same thing as using a
non-null pointer in an `if`. There doesn't seem to be any `clang-tidy`
check for a `new` expression that is implicitly converted to a `bool`
type either. I've filled
https://github.com/llvm/llvm-project/issues/64461 requesting a check or
warning to catch this in the future.
See arena.h for discussion of what canonicalization means in this
context. This is primarily intended to support implementing a memo table
of template instantiations to resolve#2951, but could be useful for
other purposes as well.
Additional changes:
- Pass `VTable` constructor parameters by pointer, to avoid the need to
define `operator==` and `hash_value` for it.
- Clean up the recurring pattern of allocating identical `NamedElement`s
on the stack and heap. Instead we always allocate it on the heap and
pass it by pointer.
- Add `Print()` and `Dump()` to `Bindings` as a debugging convenience.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This migrates explorer tests to file_test, using the new --autoupdate
functionality. Per discussion, the trace tests that were using "not"
output are mostly migrated to checking full output. The main exception
is tests that were including trace output from the prelude: the prelude
output is pretty long (multiple MB already) and I think it wasn't the
intent to include, only trace output from the small program.
This is the remaining use of lit support, so the supporting libraries
are also removed here.
I've migrated the toolchain autoupdate scripts here, I just need a
little more time to do the explorer side (which I need to check
performance, that may require multithreading as we do in the current
script). However, this felt substantial enough to share and it means I
can handle autoupdate in more of the toolchain, including preparatory
work for autoupdate on multi-file tests.
Once explorer is done I'll remove the old script.
This is a simplification of the construction, although somewhat limiting
(it means that the caller can't register the same file multiple times,
though I stopped doing that anyways since it was causing confusion).
What this more importantly _allows_ is logic on the FileTestBase child
itself that's not test-specific -- in particular, autoupdate
functionality which wouldn't use RUN_ALL_TESTS.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This shifts explorer's file_test to use ExplorerMain in order to be able
to pass arguments similar to how the command line would. It also means
that various output wrapping done by the command line is shared, no
longer copied by file_test.
In order to help make this work, I plugged vfs::FileSystem into
explorer, similar to how we're doing this on the toolchain side (might
try to share more code later). I was having trouble getting an overlay
working, I think due to how paths are specified -- but due to the issues
in fixing this, I'm just loading the prelude into the InMemoryFilesystem
for now.
Under this approach, I'm unifying more of the file versus string
handling. I think we should shift towards an approach where, like
toolchain code, anyone trying to use Explorer should use a vfs
implementation to supply code. This should reduce the number of distinct
code paths which we're maintaining/testing.
Short-term, this allows the trace testdata to be merged into file_test
with less special casing, using ARGS:. Long-term, it means that the
file_test knows the ARGS to pass to generate checks to match against,
which is the direction I'm planning for autoupdate.
I'm integrating absl flag support with a few thoughts here...
1. It simplifies the handling of files in file_test.
- Removes the need for a separate subset target.
3. Looking forward, I'm planning to add a flag to allow for autoupdate
of golden files.
4. In explorer, there's been confusion about having file_test run tests
"twice" so hopefully it's clearer when it's a separate target with a
different flag on the target.
Associated constants were getting returned directly, crashing because
the value InterpProgram received was an AssociatedConstant instead of an
IntValue. We have a similar Convert call in ReturnVar, so I _think_ this
is the right approach.
This is sufficient to convert remaining toolchain tests to file_test. %s
and %t are currently used. While #2978 might use %T with lit, I think
that's resolving a lit-specific issue that's not necessary when
TEST_TMPDIR is readily available for any necessary operations (i.e.,
setting the working directory). With ARGS, I think it's feasible to
switch to file_test without %T.
Also adds documentation to file_test_base.h, which was starting to feel
like a significant gap.
This is not yet handled by autoupdate, but I'm eyeing that next.
Don't replace line numbers with offsets from the CHECK line, because
that's both hard to read and unstable when the trace output changes.
Instead, include the actual line numbers. Move all the CHECKs to the end
of the file so that they don't reference lines that appear after them.
This is intended to reduce the number of merge conflicts in
full_trace.carbon and make the test CHECKs more readable.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Putting fuzzer files under //testing to emphasize the testonly aspect
(consolidates bazel and common subdirectories). The attributes on
explorer_fuzzer are also a little skewed from what's desirable; it's
been working okay, but this should still be a refinement.
This goes back to my comment at the end of #2934. Rather than
implementing support in the shared script, this only affects one
explorer test so would be better to have in the respective invocation.
It's coming up now because #3018 has me thinking about autoupdate
behaviors again.
#### Functional changes
* Avoid unnecessary copies when a value binding is created from a value
expression in call parameters
* Ensure the result of the value expression bound is destroyed
* Provide storage to initializing expressions used in call parameters
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Add state (stack and heap information) to the trace stream only when
there is a change in state, this would prevent consecutive repetition of
state information in the trace output.
Checks it based on change in size of the stack and heap, added
`std::pair<int, int> stack_heap_sizes` in `Interpreter` to store size of
stack and heap for comparison.
Adds the following information about name resolution to the trace
output:
* Name resolution process
```
** resolving stmt | decl `<stmt | decl>` (<source_location>)
...
** finished resolving stmt | decl `<stmt | decl>` (<source_location>)
```
* When a name is added to the static scope
```
--- declared `<name>` as `<entity>` in `<scope>` (<source_location>)
```
* When a name is resolved
```
--- resolved `<name>` as `<result>` in `<scope>` (<source_location>)
```
* Marking a name declared/usable
```
--- marked `<name>` <usable | declared but not usable> in `<scope>`
```
This will be more correct if a user file is named prelude.carbon, and
reduces the explorer runtime by about 10% by removing a string
comparison from the check for whether trace output is enabled.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Breaking apart namespaces from the main declaration list is, I think,
the most scalable approach to this. If we have more divergence in
supported declarations it gives a fairly straightforward way for
splitting, and roughly mirrors how class_body splits.
Fixes#2980
Prevent copies when initializing value expression from reference
expression. This is based on
https://github.com/carbon-language/carbon-lang/pull/2006, which
introduces expression categories, and how it is possible to convert
to/from those different categories. Continuation of
https://github.com/carbon-language/carbon-lang/pull/2907
## Functional changes
* Initializing a value expression from a reference expression takes its
value without a copy
* Reading from the value expression causes an error if the value changed
from the time it was initialized
* In this situation, prevents a copy both for variable definitions, and
call parameter bindings
## Main implementation changes
* Add new `ExpressionCategoryAction`, which evaluates an expression and
returns an `ExpressionValue` containing its category and address (if
any), in addition to the resulting `Value*`
* `ExpressionAction`s now invokes `ExpressionCategoryAction` and unwraps
the returned `ExpressionValue`
* `RuntimeScope::BindAndPin` method, and corresponding when attempting
to read a `value_node`.
## Next work
* Avoid unnecessary copies from value expression to value expression,
after ensuring that even value expression temporaries are registered for
destruction (https://github.com/Pixep/carbon-lang/pull/9)
Adding `name` to `glob_lit_tests` will make it conform with other
implementations of `glob_lit_tests` out there. If someone uses this repo
while providing a different version of `glob_lit_tests` that requires a
name, those build rules will become invalid.
To filter based on file contexts, you can use `-trace_file_context=...`
flag along with `--trace_file=...` flag. These are the following options
you can pass to the `-trace_file_context=...` flag (you can also pass
them as list by separating them with comma).
1. `main`: Include trace output for file containing the main function.
2. `prelude`: Include trace output for prelude.
3. `import`: Include trace output for imports.
4. `include`: Include trace output for all.
If the flag isn't used or non of the options are passed, by default only
main file context is traced.
File contexts are distinguished based upon the source location by
passing the source location to `TraceStream::is_enabled(..)` as
arguments. If no arguments are passed, file context is
`FileContext::Unknown` and for now it will add the trace from unknown
file context.
Note: `import` option currently doesn't work as imports aren't supported
yet.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
For `-trace_phase` flag, makes the selection of multiple phases possible
by separating them by comma.
**Example Usage:**
```
bazel run //explorer -- <program_location> --trace_file=... -trace_phase=source_program,execution
```
## Description
For filtering the trace output, we currently pass flags like
`-trace_type_checking`, `-trace_execution` etc individually and these
can be passed in a combination.
This PR introduces `-trace_phase` flag that takes a list of the
following options:
- `name_resolution`: Include trace output for the name resolution phase.
- `control_flow_resolution`: Include trace output for the control flow
resolution phase.
- `type_checking`: Include trace output for the type checking phase.
- `unformed_variables_resolution`: Include trace output for the unformed
variables resolution phase.
- `declarations`: Include trace output for printing declarations.
- `execution`: Include trace output for program execution.
- `timing`: Include timing logs indicating the time taken by each phase.
- `all`: Include trace output for all phases.
### Example usage:
```
bazel run //explorer -- <program_location> --trace_file=... -trace_phase=type_checking,execution,declarations
```
Reports an error (`value of generic binding T is not known`) instead of
performing an invalid cast when attempting to index into a tuple. Can
occur when trying to cast a generic constant in the interface position
of an implementation to `type`.
Closes#2938
Prabhat SachdevaandJon Ross-Perkins
Richard Smith
Chandler Carruth
kshokhin
josh11b
mx42
Adrien LeravatandJon Ross-Perkins
Jordan Rupprecht