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).
Update the compiler-rt patch (maybe this could be upstreamed, or try out
https://github.com/google/fuzztest?) and handle the ThreadPool ->
DefaultThreadPool rename.
Offers a more precise apples-to-apples language comparison. Replaces the
non-portable `M_PI` and removes the extraneous arg parameters. It does
use C++23, so I can understand if that's unacceptable for the time
being.
Rather than just adding `Self` to the lexical scope, add it to the
class's name scope so that it is visible in later lexical scopes for the
same class -- in particular, for out-of-line definitions of members.
Also switch some tests in `check/testdata/class` over to making
idiomatic use of `Self` both inside a class and out-of-line, now that it
works more consistently.
Note that this does not permit using `Class.Self`, but only because we
don't yet support keyword names after `.` at all. If that changed, one
could use `Class.Self` to redundantly refer to `Class`. Whether we allow
that is left to a future decision.
Previously we used the default inst block formatting, which writes out a
parenthesized list of references, which would always be 'unexpected
instref's because nothing else prints the instructions in the decl
block.
In addition, track the decl block for function declarations like we do
for other kinds of declaration. This means that the parameter
declarations for a function are now properly rendered into the formatted
IR. Note that this adds a lot of verbosity to `function_decl`, but it
does accurately reflect the IR, and we'll probably want this information
to be printed once we start supporting more complex generic function
declarations.
This removes almost all the 'unexpected instref's in our formatted
output. There are remaining cases when a declarative scope contains
multiple blocks, where we only track one of those blocks. That happens
when there is control flow within declarative scopes, and for error
recovery when a class or interface or similar is defined more than once.
As requested in #3730.
When declaring an associated entity in an interface -- just associated
functions for now -- create an associated entity value and corresponding
type to represent a "slot in a witness table". Also track the list of
associated entities on the interface so that we will eventually be able
to check impls against them.
Associated entities are represented as the integer index of their slot
in a witness table.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Follow the existing support for classes.
None of this is especially useful until other features land: we don't
yet have any use for defining methods of an interface out of line,
because we don't support `default` or `final` interface methods, and we
don't have impl lookup, so referring to an interface member is also not
especially useful. But this is a nice piece to factor out that's a
prerequisite for effectively testing other interface features.
In `Class` we have:
```
// The following members always have values, and do not change throughout the
// lifetime of the class.
// The following members are set at the `{` of the class definition.
// The following members are accumulated throughout the class definition.
// The following members are set at the `}` of the class definition.
```
`Interface` has similar, minus the "accumulated" members. I'm echoing
this, except `Function` has no `}` members; at present, nothing
differentiates between "started definition" and "completed definition",
unlike the other two.
Also removing a slightly inconsistent default value for decl_id.
Interface support is pretty skeletal so this may need additions later,
but I think it's still worthwhile to fill in the necessary bits now.
With this change, the expectation is then that everything we have right
now which _can_ be imported, is supported for import (at least for the
"current package, no overlap" case).
I'm proposing a different split, along the line of "what does this
relate to". I view impl.h as having started down this route. Moving the
inst store stuff to inst.h feels odd to me given how much else is there
right now, but maybe it's still the best approach. Some files only
contain a store, no structured class, but I felt the consistency in file
naming (without _store suffixes) might help.
I noticed that there are some empty `__init__.py` files in the repo,
with no immediately clear reason to have them. I asked [in
Discord](https://discord.com/channels/655572317891461132/655578254970716160/1211088334894534686)
and it looks like these are likely artifacts from when `lit` was used
for testing, now left over and obsolete from the migration. To keep the
repo tidy, this PR deletes the files.
By adding a constant to ClassDecl/InterfaceDecl, we're able to remove
name reference special-casing. Use TryEvalInst on the Decl to generate
the Type. For ClassDecl, then use the generated constant for
self_type_id.
To make the implementation simpler, make `PopWithParseNodeIf` return
`pair<NodeId, optional<value>>` rather than `optional<pair<NodeId,
value>>`. While wrapping the whole result in `optional` seems more
principled, it's significantly harder to work with.
I believe this PR is sufficient to pull in all current class features,
including the current bits of inheritance which have been implemented.
Because a class declaration can reference its own type, this creates an
incomplete type prior to constant loading.
Right now, the object representation is imported proactively, but
individual fields are left as ImportRefUnused. This means that member
functions and similar will only be imported if called.
This also adjusts how function parameters are being handled, to match
the expectations of Self param structure.
When formatting, I'm starting to look into constants. Otherwise we get
"unexpected instref".
Overall, there are a few things that may be worth further discussion:
- The lack of a constant corresponding to the ClassType on ClassDecl is
inconvenient -- I'd like to see how zygoloid feels about trying to
restructure this. i.e., I'm setting a constant in order to be able to
track things down later, it'd be nice if the normal IR did this simply
for consistency, or if we were able to combine these rather than having
separate instructions.
- Should we shift the parse node tracking further, and go with a setup
wherein imports can embed import references into that? e.g., negative
values go to another array which includes a ImportIRId for printing
diagnostics, replacing the invalid NodeId.
- Can the formatter switch to a more general scan of instructions for
naming, to eliminate the ImportRef constant approach added here?
- GetExprValueForLookupResult special-casing instructions felt
surprising, I might see if there's a way to restructure to avoid that.
But I think these issues are things that can be separated out.
The style guide doesn't permit private inheritance, so switch to a
member instead. Keep storing the callable to ensure it lives long
enough, but type-erase it using `function_ref` instead of a hand-rolled
mechanism.
This undoes parts of #3515 in order to allow PushGlobalInit to be called
when the initializer is called, instead of at the end of the binding
pattern. The current approach is fragile because supported patterns will
become more complex. We also will likely want similar support in `let`,
which puts the initializer first, so this offers a consistent approach
for both.
[Looking
back](https://discord.com/channels/655572317891461132/655578254970716160/1184237511766179840),
this is more or less the second option in that message, but using the
PeekNextIs to avoid vagueness about what's being popped first.
Note I'm putting in PeekNextIs for what I'm hoping will be a pretty
narrow use-case. I could've added depth arguments to the Peek functions,
but that would've rippled through a number of APIs and it's not clear to
me that this has generic utility. I mean, right now it could just be
PeekNextIsVariableInitializer, since it's only optional in that case.
This works by creating a faux FunctionDecl in the context of the current
IR, which seems to be working for function calls. Deduced params are
there, but won't really be tested until classes are up and running. Also
I may need to look further at return_slot_id to ensure it's working. But
the basics, I think, are here.
Reorganizes some other ImportRef work from `has_unresolved` that'd
relied on manual calls to a more detection-based `HasUnresolved`
approach that doesn't require as much checking.
Unqualified names don't handle scopes the way that typical names do, so
a name conflict with a namespace needs to be handled specially. I'm
still favoring keeping code close as much as possible, particularly
since long-term this syntax will probably shift to be more consistent.
For now I'm just flagging when we shouldn't push scopes, so that
MakeUnqualifiedName doesn't need to clean up.
Note, a different approach would basically be:
```
PushScopeAndStartName
ApplyNameQualifierTo
result = decl_name_stack_.back();
decl_name_stack_.back().state = NameContext::State::Finished;
PopScope
return result;
```
But that approach feels worse to me, due to the additional stack
manipulations and the need to duplicate some of the FinishName logic
just to be able to pop the scope that didn't really need to be added.
Adds `BindAlias` with a hybrid of `BindName` and `NameRef` semantics. I
think it's slightly closer to `BindName` because it introduces a name,
so I'm going more in that direction. This also matches the need for
`bind_name_id` with imports on enclosing scopes.
Note, only things that look like a name reference are being allowed on
the RHS of `alias`. This includes builtins that look like name
references, such as `bool`, but not ones that turn into values
underneath, such as `false`.
Similar to #3705, we actually have a mix of `Make` and `Create` in
factory functions too, so this PR is normalizing on `Make`. It's
intended to be consistent with the naming choice for Carbon factory
functions.
Note, MakeSyntheticBlock is the only one I feel a little weird about
because llvm's own APIs use Create, and this is essentially wrapping
LLVM calls. But the flipside is it also feels like a vague line to draw,
when we also differ from LLVM coding style in other ways.
- File::StringifyTypeExpr now has a case that hits the ImportRefUsed
TODO, so implementing that. I think the `static` approach will be
helpful in ensuring there aren't access bugs, particularly when future
support is added.
- `let` wasn't adding to exports because it doesn't use
`decl_name_stack` the way `var` does. This now adds to exports, but we
might want to unify logic for issues such as this.
- BuildImportRefUsedValueRepr is now called for another inst kind, and
it seemed like calling back to BuildValueRepr was the best way to
resolve this. I don't *think* that's going to cause recursion.
When we always had a single file in the test file, we could run the
driver over it to see output. This adds the ability to do something
similar for multi-file tests. So for example in test output, there's
now:
```
[ RUN ] ToolchainFileTest.toolchain/check/testdata/class/fail_todo_import.carbon
To test this file alone, run:
bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_import.carbon
To view output, run:
bazel run //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_import.carbon --test_arg=--dump_output
[ OK ] ToolchainFileTest.toolchain/check/testdata/class/fail_todo_import.carbon (64 ms)
```
And here's what a run looks like:
```
$ bazel run //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_import.carbon --test_arg=--dump_output
(elided bazel output)
Executing tests from //toolchain/testing:file_test
-----------------------------------------------------------------------------
===============================================================================
= toolchain/check/testdata/class/fail_todo_import.carbon
===============================================================================
= stderr
===============================================================================
b.carbon: ERROR: Semantics TODO: `TryResolveImportRefUnused on ClassDecl`.
b.carbon: ERROR: Semantics TODO: `TryResolveImportRefUnused on ClassDecl`.
b.carbon: ERROR: Semantics TODO: `TryResolveInst on ClassType`.
b.carbon: ERROR: Semantics TODO: `TryResolveInst on ClassType`.
b.carbon:18:29: ERROR: Name `d_ref` not found.
var d: (ForwardDeclared,) = d_ref;
^~~~~
===============================================================================
= stdout
===============================================================================
--- a.carbon
(eliding semir output)
}
===============================================================================
= Exit with success: false
===============================================================================
Done!
```
This also switches from PrettyStackTraceFormat to
PrettyStackTraceString. This is partly so that we can share the produced
command (avoiding two different format strings corresponding to the same
value), but also it's in my mind to step away from
https://github.com/llvm/llvm-project/pull/77351.
Jon Ross-Perkins
Richard Smith
Patrick Johnston
cui fliter
Calvin
CJ Johnson
josh11bandJosh L
czapiga