Most of these are places where we failed to include a header file and
simply never got an error about this. The fix is to include the header
file.
Most other cases are functions that should have been marked `static` but
were not. Finding all of these was a main motivation for me enabling the
warning despite how much work it is.
One complicating factor was that we weren't including the `handle.h` for
all the state-based handler functions. While this isn't a tiny amount of
code, it is just declarations and doesn't add any extra dependencies. It
also lets us have the checking for which functions need to be `static`
and which don't. For the `parse` library I had to add the `handle.h`
header as well, I tried to match the design of it in `check`.
I have also had to work around a bug in the warning, but given the value
it seems to be providing, that seems reasonable. I've filed the bug
upstream: https://github.com/llvm/llvm-project/issues/94138
I also had to use some hacks to work around limitations of Bazel rules
that wrap `cc_library` rules and don't expose `copts`. I filed a bug for
`cc_proto_library` specifically:
~https://github.com/bazelbuild/bazel/issues/22610~https://github.com/bazelbuild/bazel/issues/4446
Trying to conform with #4009. Changes SemIR::LocIdAndInst construction
to root out struct init cases with AddInst and related functions. I'm
using templating of AddInst functions in order to avoid `AddInst(loc_id,
InstName{...})` and instead have `AddInst<InstName>(loc_id, {...})` with
I think similar readability results. There are a couple cases where inst
construction is templated and so designated initializers couldn't be
used, so this may be better for those in particular due to the extra
type enforcement.
This probably doesn't clean up every last case, but I was trying to get
the bulk at once without bleeding over into less related changes.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Previously we did some of this in decl_name_stack and some of it in the
callers of decl_name_stack. Factor out a single place to pop a name and
its optional following parameters.
Part of making this behavior consistent is that we now track whether an
implicit parameter list was present or absent rather than mapping an
absent list to `InstBlockId::Empty`. This improves our redeclaration
checking and the precision of some diagnostics.
Parse the name of a declaration as a sequence of `NameQualifier`s --
which have a name, possibly parameters, and a trailing period --
followed by a name and possibly parameters. This prepares us for parsing
declarations of members of generic classes and similar cases, but
actually supporting such member redeclarations is left to a future
change.
We previously required functions to have parameters, but no longer do,
following the direction of #3848. Cases like namespaces that can't
actually have parameters are now diagnosed in check instead of in parse.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.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)`
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.
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 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.
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.
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.
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.
Note we may also want to do this with NameId, maybe some other things,
but the TypeId use is pretty broad and repetitive -- I thought I'd start
with it first.
Consume the components of the `impl` declaration, and set up scopes for
the child elements. We don't yet build a representation for the impl
itself.
Also, add an interface type value. This is necessary so that we have a
value for the expression on the right-hand side of `as` in an `impl`.
The parse nodes are still tracked as part of the same value store
interface in order to ensure parity, but they're split out from Inst
itself in order to reduce the size of Inst -- the expectation is that
they don't need to be passed around quite as much.
This change doesn't actually reduce the passing very much, although
there are hints of it: AddInstAndPush doesn't typically need a separate
parse node from the one on the Inst itself, for example. In a couple
spots I changed code to rely a little more on the InstId until the
ParseNode is needed, but it's very low hanging fruit where done. I think
convert could do more to not eagerly fetch the parse node before its
use, but more cleanup felt it would be easier to handle separately. I'm
currently viewing this as making such cleanup _possible_ rather than
executing on it up-front.
But also, I want to make sure there's a consensus to head in this
direction before pulling the trigger. We speculated that this would
result in the parse node being passed around less, and I do think that's
the case, although it's a bit fuzzy in the change.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This is accomplished by tracking an extra bit on the ID we store in the
constant values table, and propagating that from subexpressions to the
enclosing expression. This extra bit is not yet computed correctly for
types; that will be addressed in later PRs.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Form a side table with constant values for each instruction. Evaluation
is only supported for a few very simple kinds of instruction for now.
This is not observable outside of the SemIR output, because nothing
depends on expressions having a constant value phase yet.
This sets things up to use `bazel` to run `clang-tidy` using
https://github.com/erenon/bazel_clang_tidy.
I'm fixing issues outside of explorer, and disabling clang-tidy for
targets in explorer that have legacy issues. I was going to disable
clang-tidy for targets in explorer such as interpreter anyways, because
they're slow to parse, and just extended that to the currently failing
targets.
Goal is to increase type safety, though more work needs to be done (see
added TODOs).
Note that, after this change, check handlers corresponding to deleted
parse node kinds will no longer compile.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
The goal here is to make the representation more uniform so that we can
start adding different kinds of binding -- checked generic bindings and
template bindings -- across both function parameters and local `let`
declarations.
With this change, the entry in the parameter list for the function is
the name binding, not the Param itself, which has some ripple effects on
consumers of that list that want to access the parameter rather than the
binding. This is expected to change again when we start adding more of
the pattern matching SemIR, but this seems good enough for now.
This builds out a little infrastructure for one name scope to `extend`
another. We'll need more refinement here to cover other cases, but this
should provide some foundation for that future work.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Most of the calls to `StringifyType` already passed `true` for
`in_type_context`. Checking the rest, I found that every one of them was
already sufficiently clear that they were printing a type, or could be
made so with a very small change to the diagnostic text.
This creates a namespace for `package` scope.
It looks like names of class_decls in namespaces lead to an unexpected
instref. This is already true, as best as I can tell. I'm not sure if
there's a preferred approach to address that, so I've left a TODO for
now.
This gets to a lifetime subtlety, particularly with things like the
sorting diagnostic consumer that delay output. In order to reduce the
chance of accidental references, disallow StringRef in the diagnostics.
For example:
```
./toolchain/diagnostics/diagnostic_emitter.h:162:5: error: static_assert failed due to requirement '!std::is_same_v<llvm::StringRef, llvm::StringRef>' "Use std::string or llvm::StringLiteral for diagnostic lifetimes."
static_assert(
^
toolchain/check/convert.cpp:477:11: note: in instantiation of member function 'Carbon::Internal::DiagnosticBase<std::string, std::string, llvm::StringRef>::DiagnosticBase' requested here
CARBON_DIAGNOSTIC(StructInitMissingFieldInConversion, Error,
^
./toolchain/diagnostics/diagnostic_emitter.h:47:7: note: expanded from macro 'CARBON_DIAGNOSTIC'
::Carbon::Internal::DiagnosticBase<__VA_ARGS__>( \
^
```
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Add a `NameId` that is effectively just a wrapper around a `StringId`,
with
some additional predefined values for names that don't correspond to
strings, such as the name of `self` or the function's return slot.
When declaring a name such as `fn Ns.Class.F() { ... }`, enter the
scopes of `Ns` and `Ns.Class` as we form the name, and remain in those
non-lexical scopes until the end of the declaration.
When performing an unqualified lookup, look in any enclosing non-lexical
scopes in addition to looking into the lexical name table.
We now track a scope index with each lookup result in the lexical name
lookup table. This is used to determine whether a lexical or non-lexcial
result is the innermost result and whether a declared name is in the
same scope as some previous introduction of that name or in a nested
scope. For now, this could just be the index into the scope_stack, but
the intent is to also use this to detect names being declared after they
are first looked up, which requires the indexes to outlive their scopes,
so we use a persistent numbering of all scopes instead. The persistent
numbering also permits more invariant checking.
Following up on discussion yesterday regarding this split.
Note, I'm expecting #3341 to do IdentifierId -> NameId in SemIR. It
might be worth adding NameId creation directly to StringStore if you're
content with this setup though.
Per the design, field access into a class value expression is a value
expression, even though we could produce an ephemeral reference
expression instead and avoid performing a value binding. This slightly
pessimizes class member access in some cases, but we should be able to
restore the old generated code by deferring actually performing the
value binding until a value expression is needed.
The value in the name lookup table is the class declaration. Map it to
the class type when it's found by lookup in an expression. This differs
from the behavior in a declaration name, where we want to find the class
declaration itself.
Adds a `BoundMethod` SemIR node to represent an `x.F` bound method, with
a new builtin type `BoundMethodType`. Reorganized conversion of call
expression arguments to also check and convert a `self` parameter in the
implicit parameters list.
In passing, improved diagnostics and error recovery for bad call
expressions. We now build a `call` node with the appropriate type and
value category, but with invalid arguments, if the argument conversion
failed, and diagnose calls to non-callable expressions.
`addr self` methods don't work properly yet; the `addr` is ignored for
now.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Add support for member access into classes, for both non-instance
members and for fields.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Track the fields in a class, and generate a corresponding struct type as
the object representation for the class. For now, we always use a
pointer as the value representation for a class.
Chandler Carruth
Jon Ross-PerkinsandRichard Smith
CJ Johnson
josh11bandChandler Carruth