- Add the `extern` keyword for forward declarations in libraries that
don't
provide the definition.
- Treat repeated forward declarations as redundant.
- Allow them when they prevent a dependence on an imported name.
- Clarify rules for when modifier keywords should be on forward
declarations
and definitions.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
For now, a builtin function is defined by specifying a string literal
initializer in a function declaration:
```carbon
fn MyBuiltin(a: i32) -> i32 = "builtin.name";
```
End-to-end support is included for a sample `"int.add"` builtin
performing integer addition, covering constant evaluation and code
generation.
The implementation here needs substantial refactoring before we'll be
ready to start adding more builtins. That refactoring work will be
coming next. This change is aiming to checkpoint some incremental
progress.
This is to make it easier to locate various diagnostic types. I was
thinking to separately rename DiagnosticLocationTranslator to
DiagnosticTranslator, because it's now handling some type translations.
This is mostly keeping classes in one place, but FormatFn handling is
moved from DiagnosticMessage to DiagnosticEmitter for a cleaner
dependency graph where DiagnosticTypeForArg is concerned.
Fix a collection of issues that were preventing lowering for overloaded
operators from working.
Instead of creating `import_ref` instructions during name lookup in the
current block, whatever that might be, we now create them in the `file`
block always. This avoids inserting them into blocks that might not be
intended to contain them, such as functions, and avoids the IR generated
for a function depending on which names we happen to have looked up
first.
When importing a class, function, or interface, import its enclosing
scope ID. This is necessary to allow us to distinguish between functions
at interface scope, which shouldn't be lowered, and other functions, and
will also be used in future to provide qualified names for declarations
when printing types. In order to support this:
- Track the constant values of namespaces created during importing so
that we can find them when resolving an import ref. Use those constant
values to convert an enclosing scope ID from the imported IR into a
corresponding ID in the current IR.
- Change how we do two-pass import of classes and namespaces so that we
can do two-pass import even for non-defining declarations, so that we
can import the enclosing scope.
While working on the final point above, I reworked `TryResolveInst` to
return a flag indicating whether another pass is necessary instead of
implicitly encoding this in the `ConstantId`. This permits the handling
of classes to be simplified; now `import_ir_constant_values` is only
accessed in a single place.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Support is added for all overloaded operator interfaces in the current
design apart from `Assign`, which is going to require some more work to
properly handle, given that primitive assignment currently has a special
implementation for quite a few builtin types.
As we don't have support for generics yet -- in particular, generic
interfaces -- there is no support for `*With` interfaces, but homogenous
interfaces such as `Add` are supported instead.
Factor out building of call expressions so that overloaded operators can
generate calls.
Switch a few places from using specific kinds of NodeId to a general
NodeId. Because overloaded operators and other things like implicit
conversions can result in member access and function calls, those
operations can't require a specific kind of NodeId.
Add import support for associated entities, and fix import support for
interfaces and symbolic bindings. We now import interfaces in two steps,
first importing a forward declaration then a definition, just like we do
for classes. For symbolic bindings, we ensure that each BindSymbolicName
is imported only once, because its ID is used as its symbolic identity.
This is necessary because we (only) support operator interfaces that are
defined in an imported Carbon package for now.
The entire contents of `check/operator.cpp` should probably be
rethought. In particular, doing a lot of name lookups on each operator
is likely to be bad for performance. But this gets us to the point where
overloaded operators are basically working, which seems like a good
place to iterate from.
For now, the tests that the individual operators map to the right
interfaces are mostly generated by a script, but that's just because I'm
expecting a fair bit of churn in how we define the prelude and the
`impl`s -- in particular, when we add support for `AddWith`, we'll need
to update all the tests. The plan is to remove the script once things
settle down.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Still needs more merge/redeclaration logic for import semantics, but
this felt like a reasonable point to send a PR.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This change implements the check behavior for the arrow operator.
`ptr->Foo()` is rewritten as `(*ptr).Foo()` and `ptr->(X.y)` is
rewritten as `(*ptr).(X.y)`
I wanted to choose one or the other. I think some code has been using
each from early on. We're predominately using `filename`, so
consolidating on that. This conveniently matches the [Google dev doc
style guide](https://developers.google.com/style/word-list#filename)
(which we use for docs) which says "filename: Not file name".
In toolchain:
```
╚╡git grep file_name . | wc -l
35
╚╡git grep filename . | wc -l
489
```
On the parsing side, we treat `a.(b)` as a member access whose second
operand is a `ParenExpr` rather than a `MemberName`. A new node category
is added for the union of `MemberName` and `ParenExpr` to support this.
Checking is mostly reusing the same pieces we already have for simple
member access. Compound member access is in most ways a simplified form
of simple member access because it doesn't need to do any lookup.
Note, I'm annotating the lookup partly so that the reason the conflict
comes up is clear, partly so that there's actually a diagnostic line
associated with the root cause as more tests get packed into a single
file.
Add a general substitution mechanism to support substituting symbolic
bindings with their values throughout symbolic constants and, more
specifically, types. This is done by decomposing the constant
instruction into its operands, substituting into the operands, and then
rebuilding the constant value by invoking the constant evaluator.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Provide a general mechanism for determining the kind of the args of an
instruction. Use this to simplify instruction profiling a little. The
intent is to also use this mechanism as the basis of a substitution
mechanism, which will be part of a future patch.
Note that this causes us to do three table lookups and two indirect
calls in inst_profile per instruction, instead of one table lookup and
one indirect call. We can revisit this if it shows up in profiles.
I'd added these trying to catch cases where ImportRefUnused might be
returned, but I believe these are incorrect. Mistakes should still be
caught because an ImportRefUnused will lead to errors in other handling.
Removing the TryResolveImportRefUnused should clarify the semantics of
when an ImportRefUnused is expected.
This revamps the support for cross-package imports, making them look
more like a namespace. The planned model is mentioned on
[#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1217586076022210670).
This does not implement name lookup into the new namespace structure.
A few key changes in this PR (it's a little sprawling) are:
- Moves logic for adding package imports from context.* to import.*
- Remove SemIR::Import, which was the prior model. This is instead now a
SemIR::Namespace with the NameScope getting a new import_ir_scopes
field.
- Allow SemIR::Namespace to use Parse::ImportDirectiveId in addition to
the prior Parse::NamespaceId
- The import_ir_scopes field includes a NameScopeId so that as we
traverse to child namespaces, we can directly perform name lookup in the
other IR.
- is_closed_import now tracks whether a namespace comes from a different
package. This has a diagnostic implemented in decl_name_stack.
When a member access names an interface member, perform impl lookup to
find the impl and its corresponding member.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
A couple of minor functional changes here:
- We now always create a `name_ref` for the name referred to by the
right-hand operand of member access. Previously we skipped creating this
instruction if the referenced name was a field, and just created the
field access instruction. This makes our processing of member accesses
and our SemIR representation a bit more uniform.
- We now perform lookup into the type of the left-hand operand if it's
any type with a scope, not just for classes. This means we do lookup
into interface types. However, doing so isn't really useful yet because
it always finds an associated entity that isn't usable by itself. This
changes the diagnostic in
`toolchain/check/testdata/interface/fail_todo_facet_lookup.carbon`.
Note this builds on #3772 (I was partly cleaning up because I was
looking at this again).
This stops printing "Ignore missing" for ignored includes because it was
feeling noisy. This has been bugging me for a bit, and now I'm here. To
get an idea of what I mean, here it is masking a fix:
```
Querying bazel for Carbon targets...
Querying bazel for external targets...
Building header map...
Building generated file list...
Parsing headers from source files...
Ignored missing '#include "testing/fuzzing/carbon.pb.h"' in 'explorer/fuzzing/ast_to_proto_main.cpp'
Ignored missing '#include "testing/fuzzing/carbon.pb.h"' in 'explorer/fuzzing/ast_to_proto.h'
Ignored missing '#include "testing/fuzzing/carbon.pb.h"' in 'explorer/fuzzing/fuzzer_util.h'
Fixing include format in 'testing/file_test/file_test_base.h': '#include "gtest/gtest.h"' to '#include <gtest/gtest.h>'
Ignored missing '#include "testing/fuzzing/carbon.pb.h"' in 'testing/fuzzing/proto_to_carbon.cpp'
Ignored missing '#include "testing/fuzzing/carbon.pb.h"' in 'testing/fuzzing/proto_to_carbon.h'
Ignored missing '#include "testing/fuzzing/carbon.pb.h"' in 'testing/fuzzing/proto_to_carbon_test.cpp'
Done!
```
To help make this work, I'm also adjusting the gtest handling to use the
same EXTERNAL_REPO logic as the rest. I don't recall if there'd been
some other reason for the special casing, but AFAICT it works fine
(auto-adds a missing gtest dependency) this way.
I'd kept these in to separate the bazel module update from the BUILD
file changes, then forgot about it. I think all of these can be cleanly
removed now. I think it's something we should clean up for consistency
with the bazel central repository names; I think it's best to reduce
that divergence.
llvm_zlib and llvm_zstd remain because of how llvm depends on the
particular names.
The member access logic is fairly large, and will be growing with the
addition of impl lookup. Factor it out to separate the logic for dealing
with handling the parse node and updating the node stack from the logic
that checks and builds the member access expression.
This check is adding a lot of noise to the clang-tidy output for
construction of `SemIR::Inst` types, and we don't want to switch to
using designated initialization for them at this time.
The patches to bazel_clang_tidy were adopted upstream and are no longer
necessary, so I think we can simplify to git_override.
This fixes an existing issue where bazel was complaining that some rules
indirectly requested rules_python 0.29.0.
Note I'm not updating protobuf or com_google_libprotobuf_mutator. This
is because I get build errors with a protobuf -> abseil dependency if
updating.
Co-authored-by: josh11b <github-llvm@technomagi.com>
On macOS, Homebrew recommends `pipx` to install Python binaries, or
installing `pre-commit` directly, since it is available.
Here is the message in full.
```console
❯ pip3 install pre-commit
error: externally-managed-environment
× This environment is externally managed
╰─> To install Python packages system-wide, try brew install
xyz, where xyz is the package you are trying to
install.
If you wish to install a non-brew-packaged Python package,
create a virtual environment using python3 -m venv path/to/venv.
Then use path/to/venv/bin/python and path/to/venv/bin/pip.
If you wish to install a non-brew packaged Python application,
it may be easiest to use pipx install xyz, which will manage a
virtual environment for you. Make sure you have pipx installed.
```
~~Also, unless there is a compelling reason to install a particular 3.x
version of Python, it may be better to let Homebrew use whatever version
people may already have installed (or let it automatically install the
version they've tested).~~
Here are the details of the pre-commit Homebrew package:
```
❯ brew info pre-commit
==> pre-commit: stable 3.6.2 (bottled), HEAD
Framework for managing multi-language pre-commit hooks
https://pre-commit.com/
/opt/homebrew/Cellar/pre-commit/3.6.2 (679 files, 9.9MB) *
Poured from bottle using the formulae.brew.sh API on 2024-03-08 at 22:22:11
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/p/pre-commit.rb
License: MIT
==> Dependencies
Required: libyaml ✔, python@3.12 ✔
```
Installing directly with Homebrew is a supported option in the
pre-commit documentation. https://pre-commit.com/#install
This was previously discussed at
https://discord.com/channels/655572317891461132/655578254970716160/1209975051588210729.
I'm initiating this mainly because we typically use "id" suffixes to
indicate an `IdBase` being passed around and the non-id suffix of
`parse_node` suggests at it carrying more data than it actually does.
There used to be more reason for avoiding `node_id` because
`SemIR::InstId` used to be named `NodeId`, but that's no longer
necessary. As a consequence, I'd like to rename `parse_node` to more
precisely reflect its type.
In full, this is doing:
```
parse_node_kind -> node_kind
parse_node -> node_id
ParseNodeCategory -> NodeCategory
ParseNodeKind -> NodeKind
ParseNode -> NodeId
```
This is primarily in check and sem_ir, but with some `parse_node_kind`
references in parse too.
Pluralization is consistent with name forms on both sides, so that
wasn't part of my replacements.
Check that functions in an `impl` are declared properly. For now, simply
reuse the redeclaration checking logic. Longer-term, we need to check
that the signatures match in a more approximate way, after substituting
the `Self` type into the signature in the `interface`.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This handles toolchain failures per-file. The intent is to allow placing
both "success" and "fail" tests in the same file, using splits. However,
this PR only adds support and updates existing tests to continue
passing.
This doesn't add full support. I'm separating it out to make the effects
of the modifier changes clearer for review. I'm restructuring a little
with the expectation that we'll have some more categories of modifier
keywords in the future (similar to `extern`, these may not be in a "set"
such as access), and thus easily scaling up to a few more would be
useful.
Add an instruction to hold the witness table, along with a corresponding
type to keep things simpler. Add `check/impl.{h,cpp}` to house the new
logic. No checking of impls against interfaces is performed yet.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This can't be done directly from a `pull_request` action, because that's
run without write privileges. This is done for security reasons, because
it runs in the context of the pull request branch. So instead, we
perform this in two steps:
- The `pull_request` action runs `pre-commit` and uploads an artifact
containing the diffs and the event information (which is only used to
extract the pull request number).
- A separate `workflow_run` action is triggered when the `pre-commit`
action finishes. This action is privileged, and should be able to
download the artifact and create corresponding suggestions.
Unfortunately, due to the permissions model in play here, the second
half of this appears to only be testable live in production.
For now, we're splitting the pre-commit action into two actions -- one
to run on PRs and one to run when actually merging commits -- so that
the suggestions are only triggered in the former case. It might be
possible to recombine these using data in the `workflow_run` invocation
to tell them apart, but the documentation here isn't very good so I've
made this PR dump out that event information so that we can look at it
and see if it contains the relevant information.
#3740 added another similar call. This may be temporary, maybe we'll
stop using and remove later, but it's a small simplification right now
and we might also retain a similar inst -> constant -> type flow.
`Self` is modeled as a `bind_symbolic_name` with no corresponding value,
for now at least. In the future it might make sense to model it as a new
kind of instruction, or as a `bind_symbolic_name` whose value is a
`param`, but for now we just want it to introduce a symbolic constant.
In order to convert a value like the `Self` of an interface to a type, a
new instruction `facet_type_access` is introduced. This notionally
accesses the "type" field within a facet, converting it from a pair of
(type, witness) into just the type.
This detects more cases of incorrect matches between declaration and
definition. It also factors the logic out to a separate file for easier
sharing, particularly when it comes to merging imports (I'm not sure if
this exact API will be reshared, but the core logic should apply).
Jon Ross-PerkinsandChandler Carruth
Richard Smith
josh11bandJosh L
CJ Johnson
Nathan Youngman
