Trying to more proactively catch when autoupdate is missed. Most of the
execution time of these tests should be in running the program under
test, not processing output, so this should have marginal overhead in
order to produce a useful reminder.
- `Peek()` no longer returns a useful value, because we often don't
allocate a `NodeBlockId` until we finish building the block. Remove it
and update both its callers.
- Rename `PeekForAdd()` to `PeekOrAdd()` since it's no longer used to
get a block ID to add elements into.
- `PushForAdd()` was unused. Remove it.
- Add `NodeBlockStack::AddNode` to combine the operations of adding a
node and inserting it into a block.
Instead of modeling array initialization as a thin wrapper around tuple
initialization, handle it like a function call, with a return slot as
part of its input. This better matches how initialization via a call to
`ImplicitAs::Convert` will eventually work, and in particular lets us do
in-place initialization of arrays rather than always creating a
temporary.
I'm finding the current autoupdate difficult to reason about. What I'm
trying to do here is use the class to make it easier to add helper
functions.
For example, I merge the vector+cursor for stdout/stderr into an object,
passed to helpers together instead of as two parameters.
ShouldAddCheckLine can check against output_file_number_ without passing
that through a couple levels of function calls. In turn,
ShouldAddCheckLine is shared with the end-of-file logic instead of that
having its own comparison from what AddCheckLines does.
Also, I'm trying to get the pre-AUTOUPDATE edits in their own loop,
distinct from the main code. The class means AddRemappedNonCheckLine is
a helper function to share code, instead of a lambda (which I was
thinking would just confuse the flow further).
I'm also changing the input non_check_lines to a single vector to match
stdout/stderr. Because of the AUTOUPDATE + SPLIT lines, we're guaranteed
to have at least one line per file.
I realized file_offset_in_new_lines is redundant with output_line_number
so code now uses the latter (because it's older -- I think one's as good
as the other, otherwise).
Note, this change deliberately does not affect output. I'm only trying
to make it easier to read for the next changes, using the lack of change
in results as a good indicator that this is getting it right.
`SemIR` now only stores complete blocks, and placeholders for blocks
where we need a block ID before we know the block contents.
This is a preparation step towards the new node block allocation design.
This removes the risk of accidentally performing a vector copy when
calling these functions, and is a preparation step towards the new node
block allocation design.
This required changing how we build call expressions. Instead of
finishing the argument block and then later adding a return slot, we now
delay finishing the argument block and checking for conversions to
parameter types until after we've added the return slot to it.
Include information in the test crash output to identify which test we
were running, and the corresponding resolved argument list, if either
`bazel test` or autoupdate crashes.
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.
Lex now prints its yaml as:
```
- filename: name
tokens: [ ... ]
```
New support in file_test allows the `filename` marker at the top to
define the default file number for later lines, meaning multi-file
output from lexing is now associated with the appropriate file. Similar
support will probably also apply to lowering, semir, and other places
that print a filename once for the full dump.
This hammers a bit at how line number replacements work in file_test,
allowing stacking them so that lex errors and stdout can both be
line-associated properly. I've tried to make the autoupdate more
frequently work in one pass, now also taking into account the file index
when doing line replacements.
There are still some issues with EndOfFile that it may be good to
discuss: because CHECK lines are appended to the end of the file now,
and the EndOfFile token points at the last line including comments, new
lex tests now take two runs to autoupdate (because without CHECK lines,
the EndOfFile points at a content line, which content is then inserted
after). Note that removing CHECK lines from the test is not a solution:
autoupdate also started inserting blank lines, which breaks this for a
similar reason. One solution here might be to not have EndOfFile
associate with a line or column, which has been a bit of an issue
regardless.
Also fixes a small issue with toolchain's autoupdate script.
We're seeing issues building on the 20230911.1.0 release. I think there
may be a misconfiguration; using `/usr/lib/llvm-15/bin/clang++`, I'm
getting errors such as:
```
external/com_google_absl/absl/base/config.h:56:10: fatal error: 'cstddef' file not found
#include <cstddef>
^~~~~~~~~
```
e.g.:
https://github.com/carbon-language/carbon-lang/actions/runs/6166275499/job/16735504697
(the passing ubuntu run is on a 20230903.1.0 image using llvm-14)
(note if this PR fails testing, it needs to be merged before it would
take effect)
Reported on https://github.com/actions/runner-images/issues/8253
This better reflects the purpose of these semantics nodes, and prepares
for adding TupleValue and TupleInit nodes to represent forming values
and initializers from literals.
When printing callable declarations, it does not print the self pattern
in the deduced bindings.
Example:
User code:
```
class A {
fn Fun[self: Self]() {}
}
```
Print output is missing the self pattern
```
class A {
fn Fun ()
{
}
}
```
This PR fixes it and includes the self pattern in the print output.
```
class A {
fn Fun [self: Self]()
{
}
}
```
---------
Co-authored-by: Geoff Romer <gromer@google.com>
In order to maintain diagnostic quality, add a mechanism to add notes to
any diagnostics that are produced as part of initialization of function
parameters. As suggested in review of #3205.
In passing, fix the only caller of `ImplicitAsRequired` outside of the
implementation of `Check::Context` to instead use
`ConvertToValueExpressionOfType`. This causes some missing
`value_binding` nodes to be added to the produced SemIR. Also fixed a
matching bug in lowering where a bogus load was being added, that
resulted in assertion failures when the checker bug was fixed.
The warning `-Wnon-virtual-dtor` starts producing false-positive
warnings after this change. Replace it with the fixed version,
`-Wdelete-non-virtual-dtor`.
toolchain/driver/testdata/fail_missing_file.carbon has no content after
its AUTOUPDATE line. This caused it to oscillate between three states:
1) If there were no content lines after AUTOUPDATE, autoupdate would add
a blank line and then its STDERR CHECK line.
2) If there was a (blank) content line after AUTOUPDATE, autoupdate
would move the STDERR CHECK line to immediately after AUTOUPDATE,
leaving a trailing blank line.
3) pre-commit would remove the trailing blank line, leaving no content
lines after AUTOUPDATE.
Handle the special case of no content lines after AUTOUPDATE by
producing "early" STDERR check lines immediately after the AUTOUPDATE
line, rather than before the next line, which might not exist. We
already did this in the case where there were early STDOUT check lines.
Specifically this should improve linking by producing one large binary
instead of one-per-directory. The inclusion of the driver hits the size
issue. Separating out things which have more llvm deps has been
discussed, but I'm not doing that here because I think the semantics
layer will need to depend on clang for interop, and we'd lose a lot of
the benefits that way. Also, having just one place to look seems
simpler.
Includes supporting changes to file_test infrastructure, the most
significant of which is probably passing tests via file instead of a
really large args thing, using a custom rule to do that. That's because
dealing with the layered filegroups that allow the toolchain setup is
more complicated, and this approach scales well.
Combined test time is ~9s, so not sharding right now.
I wasn't sure if people would prefer having the autoupdate script under
testing, so I left it alone for now.
Write unattached CHECK:STDOUT lines at the end of the complete test
file, not at the end of the first split file.
Also, perform line number remappings for the current file even if we see
a check line for an earlier file first. We used to stop performing
remapping after the first check line that referred to a previous file.
To facilitate this, instead of splitting the check lines up by output
file prior to forming the output, we instead form a single list of check
lines and have the check lines track which file they refer to.
This updates SourceBuffer to diagnostics. Some additional edits to
diagnostics were necessary due to issues moving arguments around, which
seems to stem from a compile error with clang 14 (fixed in later
versions).
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
### Description:
This pull request introduces two significant changes:
**Improved Error Handling in `remap_file` Function:**
- The error handling in the `remap_file` function has been simplified
for improved clarity and user-friendliness. The original code included a
combination of `assert` and `exit`, which could be perplexing during
runtime.
**Changes Made:**
- Replaced the original `assert` and `exit` with a more straightforward
`if-else` block for handling errors.
- Utilized `sys.exit` for clear error messages and proper program
termination when an error occurs.
**Why I Did It:**
- Improved Clarity: The original code's `assert` primarily served
debugging purposes and might not behave as expected during runtime. The
new code offers clarity and predictability.
- User-Friendly Errors: The updated code provides users with easily
understandable error messages.
**Added `.DS_Store` to `.gitignore`:**
- `.DS_Store` files are commonly generated by macOS Finder to store
folder-specific metadata. They do not need to be tracked in version
control.
**Changes Made:**
- Added `.DS_Store` to the `.gitignore` file.
**Why I Did It:**
- To prevent `.DS_Store` files from being tracked in the Git repository,
ensuring a cleaner repository and avoiding unintended commits of
macOS-specific files.
### Improvements:
- **Better Error Handling:** The new code in the `remap_file` function
ensures the program exits gracefully with informative error messages
when errors occur.
- **Enhanced Code Clarity:** The code now communicates its intentions
more clearly with `sys.exit` for handling expected runtime errors.
- **Cleaner Git Repository:** The addition of `.DS_Store` to
`.gitignore` prevents the tracking of macOS-specific files in version
control.
Rearranges driver logic into CompilationUnits in order to associate
artifacts from the various stages of compilation.
Note, I'm not totally sure what the right thing to do is for
lower/codegen, so I'm just doing a rote change there for now that
mirrors prior phases (this is all the code supports anyways, so is
probably right for now regardless).
SourceBuffer error output is moved local for consistency with other
steps, and so that it's less ambiguous whether the error should be
expected to already include a filename.
In general, LLVM's parsing of decimal integers to `APInt`s forms an
`APInt` that is 4n bits wide, where n is the length of the integer, and
the `isNegative` check only checks the high bit.
In this case, we form an `APInt` that is four bits wide, with the high
bit set, which we reject because we think it's "negative". These two
array lengths are the only ones where this happens -- if the decimal
integer value is two characters long, we form an `APInt` that is eight
bits wide but holds a value < 100, so the high bit is never set, and the
same applies for longer integers too.
An `IntegerLiteral` is never negative, so we don't need the `isNegative`
check, and in fact it only detects the n=8 and n=9 cases.
IntegerLiterals are not signed, so get the zero-extended value rather
than the sign-extended value. Sometimes we use the high bit of the
`APInt`, though currently this only happens for the literals 8 and 9 due
to the way we convert decimal integers to binary.
The expensive local actions will be separately gated to not overwhelm
the machine, and currently highly asynchronous actions are a dominant
part of our builds due to downloading cached artifacts. Without a high
concurrency, these are downloaded roughly 2-at-a-time currently.
I've verified that on a small machine without a good cache this doesn't
seem to generate huge amounts of work and local build and test actions
are successfully gated on the local flags.
There is already a Bazel issue tracking this limitation:
https://github.com/bazelbuild/bazel/issues/6394
Continuing with #3070. Just a dir and file rename (only prefix change is
lexer_file_test). Everything in the lex dir should be marked as a move.
Note, I think this closes#3070. There may still be further cleanup
later, but the organizational changes suggested there are being
completed.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
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).
Versus the namespace change in #3170, these felt like they may get more
nuanced review, so I'm splitting them out:
- Removing "Lexed" from LexedNumericLiteral and LexedStringLiteral
- Moving classes out of the TokenizedBuffer class, so that they're now
things like Lex::Token instead of Lex::TokenizedBuffer::Token (i.e.,
much shorter to type, more consistent with things like Parse::Node).
Continuing with #3070, this creates a `Lex` namespace for lexer. This is
probably the last namespace addition right now, and will be followed by
file moves, although I'll also share a PR separately moving member
classes out of TokenizedBuffer.
This is necessary for a clean split of the SemIR and Check namespaces.
My design intent with check.h is that check/check.h provides the factory
functions necessary to construct a SemIR, which is the main reason I
have the MakeBuiltins wrapper there.
I don't think File's constructors are great, but it felt good enough to
me in that context. I considered factory functions, or something like an
enum as discriminator, but it felt like more burden than improvement.
Specify the behavior of function calls and the type and behavior of the
entity
introduced by a function declaration.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Jon Ross-Perkins
josh11bandJon Ross-Perkins
Richard Smith
Prabhat SachdevaandGeoff Romer
Ikko Eltociear Ashimine
Chandler Carruth
m3ndax