Change impls from `<interface>.impl` to `<self>.as.<interface>.impl`,
and *member* functions to `<parent scope>.<fn>` (non-member functions
exclude their parent scope). Stop special-casing builtin functions,
given the new naming scheme.
The purpose of this is to make it clearer when a member function is
being accessed and, if so, which member function. In particular, we
often access interface `Op` functions. The builtin function
special-casing was intended to help with that, but we still have lots of
`Op` functions. This particular approach should make the interactions
clearer.
This changes up queueing of block IDs a little because, in particular,
we need to process bodies of entities only after constants finish
processing. But, it should also result in less memory usage during
processing because it means we have less on the insts stack at any given
time, since we track a block rather than all instructions contained by
the block.
This switches from the `CollectNamesInBlock` approach for entities, to
instead traversing entities as they're encountered. For example, when
traversing constants, when a type is found, the entity will have its
block queued for processing.
This leads to a change in the traversal order, which affects
disambiguation done by numeric sequencing (since that's just showing the
traversal order).
This will allow for simpler "name based on name" logic. This is
something I plan to use for:
- impls: `<type>.as.<interface>.impl`
- functions: `<entity>.<member function>`
- Note an impl may be used as the entity for a bound function.
By naming the entities as they're encountered, I'll be able to rely on
the generated names rather than recalculating them.
To assist this, I'm also differentiating between the ambiguous and
disambiguated name. Otherwise, we could end up with things like
`<function>.<disambiguator>.<call>.<other disambiguator>`, where the
repeated disambiguator may not be necessary in order to get full
disambiguation. It's also a smaller delta from the current output.
Note, changing `Name` to a class felt appropriate given its shape. I was
also noticing that parts of its API were unused, and the class helps
detect unused private members.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
In `BuildUnaryOperator`, `GetOperatorOpFunction` is treated as
desugaring, but `PerformCompoundMemberAccess` and `PerformCall` are not.
This treats all of destruction as desugaring.
This leads to some instructions being elided, because of `GetOrAddInst`
behaviors:
> // If the instruction has a desugared location and a constant value,
returns
> // the constant value's instruction ID. Otherwise, same as AddInst.
This changes instructions that previously had a non-desugared location
to instead have a desugared location, so if they also have a constant
value then the constant value can be used directly.
This just catches uses equivalent to `return;` and `return <expr>;`.
Note it's just extending the current implicit return logic, not really
adding much unique here.
I'm still delaying break and continue because those require partial
destruction, which is more work and I want to be careful to get it
right.
This changes `Destroy` to use an interface for its implementation.
Note that this change includes a lot of test updates. Even when
`Destroy` is a no-op, it still causes code generation as part of
determining that.
Originally I was trying to use ranges to cut down the scope of this, and
to a degree I think they have. But a flipside here is that cases where
no destructors should be generated -- particularly globals -- would be
needed to completely remove destructor calls. Even for ranges, the range
can often include the destructor placement. So I've shifted
frame-of-thought a little: accept a bunch of destructor churn, because
destructors are needed and will be prevalent. The verbosity is a feature
of the design to make desugaring apparent in IR, not a bug.
This drops file_test runtime from about 3s to 2.5s on my machine, which
is now ~30% faster than before #5653 slowed things down by adding a lot
of stuff to the production prelude.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Replace the binary `Operation` interfaces with the `OperationWith(T:!
type)` interfaces described in the design, and add a `Result` associated
type for both unary and binary operations. Update the `impl`s in the
prelude for integer types to use the new form, including supporting
implicit conversion of either operand.
I've tried to split this PR up into commits focused on distinct changes
for review convenience. It may be simplest to review it one commit at a
time.
Previously we walked the global variables defined by the current file
and emitted an LLVM global variable definition for each of them. Now
instead, when emitting a constant reference to a global variable, we
emit an LLVM global variable declaration, and we then subsequently walk
the global variables defined by the current file and convert each of
them from a declaration to a definition.
In order to make import of names of global variables work, add support
for import of `var`, as well as support for importing `tuple_access` and
`tuple_pattern` in the case where the `var` has a tuple pattern in its
declaration. Also treat `bind_name`s that are reference bindings to
`var`s as having the same constant reference value as their `var` so
that we can properly import and lower them.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Where `--no-dump-sem-ir` is used, change to `--dump-sem-ir-ranges=only`.
Otherwise, add `--dump-sem-ir-ranges=if-present` with a TODO to change
to `only`.
Note, SemIR is affected just because the extra comments change line
numbers in files where splits aren't in use.
- Track the `VarPattern` instruction on the `VarStorage` instruction so
that it's available for name mangling.
- Mangle global variables based on the first binding name within their
pattern.
- Give global variables external rather than internal linkage, except if
they have no bindings whatsoever in their pattern.
- To support lowering references to bindings nested within a global var,
such as for `var (x: i32, b: i32)`, add some basic initial support for
reference constant expressions. Treat a global `var` as a reference
constant, and treat an aggregate access into a reference constant as a
reference constant.
This is making two inter-related changes:
- Change `file` to reuse the formatter logic of `constants` and
`imports`, meaning empty `file` scopes will be omitted
- Mark `<elided>` sections in blocks (not in non-block scopes, because
they're not as sequential)
Instead of building an eval block as a separate pass at the end of a
generic, build the eval block incrementally.
The larger change here is that asking for the type or constant value of
an instruction now always returns an unattached type or constant value,
in order to preserve the behavior that we previously achieved by doing
the rewrite to attached types and constant values at the end of handling
the generic.
This also incidentally fixes some subtle issues where attached types and
constant values would leak out into check and cause it to get confused
about differences between attached and unattached values. Check should
no longer see attached values except where it explicitly asks for them.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Don't import `ImplWitnessTable` into the `constants` block, because we
generally don't put `Unique` constants there. This matches the handling
of the other kinds of `Unique` constants. In order to keep the
instruction visible in formatted SemIR, add it to the `imports` block
instead.
Also fix a bug in the instruction formatter that resulted in
instructions in the `imports` block being omitted from the output if
they were only referenced by earlier instructions in the `imports` block
and by instructions in the `constants` block. This was already resulting
in some referenced instructions being omitted from the output, but also
occurred frequently for `impl_witness_table` instructions after this
change because it is common for the only reference to those instructions
to be from `impl_witness` instructions in the `constants` block.
This was originally needed to support constant evaluation of name
expressions, but that's now done in a different way.
This is actually a step toward treating all patterns as constants. The
upcoming change will do so in a slightly different way, and so it will
simplify the review to start from a baseline where patterns are never
constant.
Right now, return_scope_stack is being used to determine whether logic
is in a function scope. However, we need to handle nested entities
inside function scopes. For example where this crashes right now:
```
base class C(B:! bool) {}
fn F() {
class B {
extend base: C(true or false);
}
}
```
This is doing a few things to make this kind of code not crash:
- Split `scope_stack().Push` into `PushForDeclName`, `PushForEntity`,
`PushForExpr`, and `PushForFunction` so that better decisions can be
made about behaviors.
- Hide `return_scope_stack` in the API, instead using interfaces to get
at the underlying data.
- Also using `PushForFunction` to update it similar to the other stacks
that `ScopeStack` manages.
- Add `IsInFunctionScope` as the best way to determine presence in
function scope.
- Remove `PeekIsLexicalScope` since destruction really wants function
scope information anyways.
- Clean up `destroy_id_stack` handling to be for function scopes rather
than lexical scopes.
- Return after related `context.TODO`s in a couple more spots, so that
code doesn't proceed to add control flow in spite of the lack of
support.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Provide builtins for compound assignments instead of defining them in
the prelude as a use of a binary operator and an assignment. This allows
us to lower compound assignment directly to LLVM operations instead of
producing a function call. In the short term this also allows us to
define a type-generic compound assignment in the prelude.
This allows us to import the table for a given impl only once, while we
can import many ImplWitness instructions with different specifics for a
generic impl.
For example in convert_facet_value_to_narrowed_facet_type.carbon we see
that a single witness table is imported for the BitAnd interface, with
multiple witnesses (for different specifics) imported and sharing the
same table.
The ImplWitnessTable now contains a back-link to the Impl the witness is
for, allowing inst namer to name that interface in the textual semir,
and allowing the interface to be found when debugging from a witness.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
In preparation for shifting from `TypeId`s potentially representing
attached types to always representing unattached types, using
[terminology suggested on
Discord](https://discord.com/channels/655572317891461132/963846118964350976/1359286326779973712).
This change causes us to track slightly more type spelling information
through SemIR.
One change that has significant impact on the SemIR output is that we
now build a `struct_type` instruction in each class representing the
types of the fields, including the spelling used for those types. This
is now no longer always identical to the corresponding canonical
`struct_type` for the object representation, so it's built separately
and owned by the class.
Also remove `TypeBlock` support entirely, as its only use was
representing `TupleType`s, which now use an `InstBlock`.
Instead of using None, use an explicit ImplWitnessTablePlaceholder in
the witness table for entries that have not yet been populated, to aid
debugging. This would ensure they would show up very clearly in the
SemIR. This uncovered some `<invalid>` in the SemIR under erroneous
conditions that have now been turned into `<error>`.
Add the ImplWitnessAssociatedConstant instruction which wraps the
canonical instruction found from the constant value of the rewrite
constraint. This ensures that we have an instruction inside the eval
block for a generic impl declaration for each rewrite constraint's
value, which allows Subst to be performed to rewrite the symbolic
constant of the ImplWitnessAssociatedConstant instruction to associate
it with the generic. This will prevent the otherwise orphaned symbolic
constant of the rewrite's value from being used which can not have a
specific applied to them.
While applying the new insts in InitialFacetTypeImplWitness(), rearrange
the function to use less nesting. And avoid using entity names from
imported instructions (as we found is not effective in deduce.cpp) and
use a local instruction by going through the constant value.
This PR is part of the effort to allow a rewrite to name a generic
parameter, such as `impl forall [T:! type] T as Z where .X = T`, however
tests for this involve a final impl so that we can typecheck that the .X
value is a specific T, so the tests will come with that work. This piece
is split off because introducing new instructions causes a lot of SemIR
churn, and I wanted to get that done separately.
For each kind of instruction, specify whether its constant evaluation
needs an `InstId` or not. If it does, ensure that all constant
evaluation of that instruction provides one. Otherwise, allow calling
into the evaluator without providing an `InstId`.
This allows us to reliably use the `InstId` in evaluation steps that
either need a location or need to look at the original operands of the
instruction prior to evaluation, and also to support `TryEvalInst` calls
safely for instructions whose evaluation does not need an `InstId`.
Each of these types takes another type as an operand. Instead of storing
that other type as a `TypeId`, store it as an `InstId` so that we can
track how it was written, not only its canonical form.
The canonical constant values of these types continue to store the
canonical constant values of their operands, as normal.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
When transforming instructions with symbolic constant values into the
eval block, we previously special-cased `bind_symbolic_name` (and
`symbolic_binding_pattern`) because they are places where symbolicness
is introduced, rather than propagated from operands, and just copied
them into the eval block. However, `bind_symbolic_name` can be dependent
on other symbolic constants, because it can have a type that is
dependent. In this case, the copy in the eval block would not have its
type properly adjusted to refer to the type within the eval block.
Fix this by performing substitution into `bind_symbolic_name` rather
than copying it directly, and instead, detect cases where substitution
determined that the instruction was unchanged despite having a symbolic
constant value, and force it to be rebuilt in that case.
I've not found any way that the previous behavior actually caused
problems, or affected the observable behavior of the toolchain. The type
of these instructions in the eval block doesn't make much difference to
anything because they get immediately replaced by their corresponding
argument values when we run the eval block. But this came up and caused
some test output churn when I was making a different change, and it
seems like a fix to our representation even if it's not changing
behavior, so I'm splitting it out so it can be handled separately.
Currently this test fails with trying to access a comptime function with
runtime values:
```
fn F() {
let a: J = {} as J;
let b: J = {} as J;
// CHECK:STDERR: fail_bit_and_values_no_impl.carbon:[[@LINE+7]]:3: error: non-constant call to compile-time-only function [NonConstantCallToCompTimeOnlyFunction]
// CHECK:STDERR: a & b;
// CHECK:STDERR: ^~~~~
// CHECK:STDERR: core/prelude/operators/bitwise.carbon:96:3: note: compile-time-only function declared here [CompTimeOnlyFunctionHere]
// CHECK:STDERR: fn Op[self: Self](other: Self) -> Self = "type.and";
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
a & b;
}
```
The issue is that the BitAnd impl for facet types is matching on any
value of any type:
```
impl forall [T:! type] T as BitAnd
```
What we really want is for it to match on facet types (which are type
values), which is written as:
```
impl type as BitAnd
```
After this change, the error makes more sense in the above test:
```
fn F() {
let a: J = {} as J;
let b: J = {} as J;
// CHECK:STDERR: fail_bit_and_values_no_impl.carbon:[[@LINE+4]]:3: error: cannot access member of interface `Core.BitAnd` in type `J` that does not implement that interface [MissingImplInMemberAccess]
// CHECK:STDERR: a & b;
// CHECK:STDERR: ^~~~~
// CHECK:STDERR:
a & b;
}
```
A query facet type may contain multiple required interfaces, in which
case impl lookup should return an ImplWitness for an impl that is used
for each interface in the query. We bundle these together into an
instruction block and return that from impl lookup. The witnesses are in
the same order as the interfaces in the
`CompleteFacetType::required_interfaces`. This allows walking the
`required_interfaces` to find an interface to give an index that can
also be used to grab a witness from this set, or from FacetValue.
FacetValue now has an InstBlockId for the set of witnesses of the
FacetType, instead of a single ImplWitness instruction id.
FacetAccessWitness includes the index of the witness (determined from
the position in `required_interfaces`) of the witness it's accessing
from the FacetType.
The
toolchain/check/testdata/facet/no_prelude/fail_todo_call_combined_impl_witness.carbon
test demonstrates the fix in the resulting SemIR. We can see the calls
to methods on a multi-interface FacetType result in a FacetAccessWitness
with an index of the correct interface, and this results in a witness
that leads to the correct impl's function.
There is a TODO in member access, where it does not have a
`CompleteFacetType` yet, so it uses the index in
`FacetTypeInfo::impls_constraints` instead, but this can be incorrect in
the presence of named constraints, which when completed can add more
interfaces to the `CompleteFacetType` and which are sorted into an
arbitrary order with the rest there.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
During SemIR, when identifying a specific in a specific context, we'd
have to either look through a specific or through a bound method.
Canonicalize which one to look through first, by having the BoundMethod
created around a SpecificFunction instead.
This changes a lot of check tests.
TODO: As the SemIr does not currently allow removal (access to insts()
is intentionally const), the bound instruction created prior to finding
the specific is not removed from the instructions.
Options: (1) leave as is, (2) add a way to remove the previous bound,
(3) rethink how/when the BoundMethod inst is created.
- Explicitly document that `*Param` and `*ParamPattern` insts represent
`Call` parameters.
- Stop wrapping compile-time parameter patterns in `ValueParamPattern`
insts (because they aren't `Call` parameters).
- Document how `MatchContext::results_` relates to the `Call`
parameters, and be more consistent about when it's written to.
- Remove `RuntimeParamIndex::Unknown`: we no longer need to distinguish
"this `Param`'s runtime index is unknown" from "this `Param` isn't a
runtime param", because we no longer use `Param`s at all in the latter
case.
- Rename `RuntimeParamIndex` to `CallParamIndex`.
As a side effect of removing the `ValueParamPattern` insts, this fixes a
minor diagnostic bug where `NoteInitializingParam` didn't identify the
specific parameter that led to a deduction failure, because it expects
generic parameters to only be represented by `SymbolicBindingPattern`s,
but before this change they could be wrapped in `ValueParamPattern`s.
Doing so results in TODOs in the resulting semir, since we don't handle
combining the facet types together properly or doing lookup into them.
There's a test added demonstrating this, which will be made to work in
followups.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
In line with the proposal in #4682, this changes the array syntax to be
array(T, N). `array` is a builtin keyword which must be followed by
parens containing two expressions and a separating comma.
The array type expression is still fully builtin, it does not forward to
a Core.Array library type yet. It merely adds the `ArrayType`
instruction, as was done with the previous syntax.
Followup work will change the instruction to reference to Core.Array,
once the library type exists and can be used directly.
---------
Co-authored-by: zygoloid <richard@metafoo.co.uk>
* Add `RequireCompleteFacetType` and `ResolveFacetTypeImplWitness` to
`check::Context`. Goal was to move code from `impl.cpp` (mostly) without
functional changes.
* Complete type information is cached with the facet type, and is stored
in a `complete_facet_types()` table.
* Main functional change is to diagnose attempts to use a rewrite
constraint on an associated function. Some existing diagnostics have
been updated.
* Remove `check::Context::RequireDefinedType`:
* For class types, use `RequireCompleteType`
* For facet types, use `RequireCompleteFacetType`
* Introduce a `SemIR::SpecificInterface` to hold an interface and
specific id pair.
* Keep the specific interface ids in the impl object.
* Avoid some extra copies in `Dump` functions.
* Future work missing from this PR:
* Resolving for member access or actions that require impl lookup.
* Resolving rewrites constraints that refer to non-concrete values.
* Any support for adding implied constraints that result from a `where`
clause (though TODOs have been added).
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Dana Jansens <danakj@orodu.net>
For an expression such as `(Type as Interface).AssocFn()`, track the
`Self` type `Type` in the result of the member access so that it's
available when checking the function call.
This introduces a new kind of type, `ImplFunctionType`, that represents
the type of a function that is expected within an impl, modeled as the
type of the function within the interface plus a value to use as `Self`.
Calls to values of this type behave like calls to the underlying
function except that the `Self` parameter is pre-bound to the self type
from the facet.
In order to support this, fix an issue where the imported list of
generic bindings lost their association with their enclosing generic.
This adds a little complexity to `import_ref`, including a new recursive
cycle that I intend to address in a follow-up PR.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
High level, replacing `Id::Invalid` with `Id::None` and `Id::is_valid`
with `Id::has_value` for clarity, as discussed
[here](https://discord.com/channels/655572317891461132/655578254970716160/1331664574545395794).
The `IntId` refactoring is needed together with `AnyIdBase` because it's
also used with `ValueStore`.
Note, trying to be careful not to rewrite `EnumBase::InvalidIndex`, or
`is_valid` in general (e.g., `IdKind::is_valid`).
I've tried to sequence commits here:
1. Automatic replacements:
- `((?:Id|Index)(?: |::|\(|Base(?:\(|::)))Invalid((?:Index)?\W)` ->
`$1None$2`
- `<invalid>` -> `<none>`
- `InvalidNodeId` -> `NoneNodeId`
- `/\*invalid\*/` -> `/*none*/`
- `id((?:_|\(\))(?:\.|->))is_valid` -> `id$1has_value`
2. Manual edits:
- In `int.h` and `int_test.cpp`
- `IntT` has `is_value`, which I'm renaming to `is_embedded_value`.
- Manual edits to comments in this file.
- `AnyIdBase` and `IdBase`
- Declaration of `is_valid` -> `has_value`, `InvalidIndex` ->
`NoneIndex`.
- In `ids.h` and `ids.cpp`
- `is_valid` -> `has_value`
- `// An explicitly invalid ID.` -> `// An ID with no value.`; similar
for index
- Various math on `InvalidIndex` -> `NoneIndex`
- Various mentions of "valid" in comments
- In `value_store.h`, for `IdT::Invalid`, plus one comment
- In `impl.h` and `tokenized_buffer.h`, we had different initialization
of `::None` values (versus `ids.h` syntax) that I fixed manually.
- Spot checks to compile
- Particularly where `is_valid` replacements didn't catch spots due to
different naming.
3. Autoupdate tests
4. verbose.carbon (NOAUTOUPDATE)
5. Comment spot checks
Note there are probably other mentions of "Invalid" that should be swept
up, but I'd like to argue for merging and separating out remaining
cleanup since this is so sweeping (and likely to hit merge conflicts
from churn). We'll probably have lingering mentions of "invalid" for a
bit regardless, just because there are uses of "invalid" in non-Id APIs.
Changes the name of SemIR `import_ref`s to use the format
`<package>.<entity>`.
<table>
<tr><th>Before</th><th>After</th></tr>
<tr>
<td><code>%import_ref.05a: type</code></td>
<td><code>%Main.D: type</code></td>
</tr>
<tr>
<td><code>%import_ref.8f2: <witness></code></td>
<td><code>%Main.import_ref.8f2: <witness></code></td>
</tr>
</table>
* [Discord discussion in
#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1330253540999827577)
* Closes#4769
This removes some churn when adding new diagnostic cases to test files
(where previous to this change the newly added newline would cause the
previous diagnostic CHECKs to be updated including changes to the line
number because the CHECK for the blank line meant an extra line between
CHECK and source line).
A few alternatives discussed here:
https://discord.com/channels/655572317891461132/655578254970716160/1329573358475673723
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Add a Vtable typed inst with a type_id (of the type this vtable applies
to) and list of virtual function decls (or import refs to function
object constants).
This doesn't add lowering/emission of the vtable, or usage when
initializing objects of the type.
Some questions in case they're interesting to discuss:
* is it right/worth having the type_id in the vtable? (probably makes it
easier to emit - using the type to get the class name to figure out the
mangled name for the vtable) perhaps it should be a ClassId?
* I'm thinking the logic in CheckCompleteClassType could be the place we
handle diagnostics for mismatched keywords (virtual/abstract for a
function that's already virtual/abstract, maybe checking for non-virtual
functions with the same name in a base class, or derived class functions
without `impl`, etc) - but we could move some of that to the moment we
walk the function decl, and record our findings in the function decl
(record the base function it overrides, or the index of the vtable to
slot to use when building the vtable at the end of the class)
* the Vtable typed inst has `constant_kind = InstConstantKind::Always`
and `is_lowered = false`, I think I added that in to workaround/address
some failures in lowering. And seems correct for this intermediate step
- I'll add lowering in a follow-up patch. But the constant_kind - what
should this be? We can just say all vtables are of VtableType (in which
case the `Always` constant kind sounds right to me) or we could have
them introduce a type with each virtual function as a named member,
even?
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
When printing a Class, the complete_type_witness was printed last but
this gave a somewhat misleading representation as it appeared to be part
of the !members label. Move it above the label so that the label more
clearly refers to everything below it.
Include the index rather than the name in the fingerprint of a symbolic
binding. While both the index and the name contribute to the canonical
identity, using either one of them in the fingerprint is sufficient to
ensure that distinct entities get different fingerprints. Changing the
name of a symbolic binding should ideally not result in fingerprint
changes, so exclude the name from the fingerprint when we have an index.
Use the canonical type and constraint when fingerprinting an impl, so
that uses of names in `name_ref` instructions aren't considered, only
the entity the name resolves to, and different ways of spelling the same
type have the same fingerprint. This similarly allows compatible changes
to be made to impls without changing the fingerprint.
Exclude the declaration block when determining the fingerprint of a
declaration. The declaration block contains the declarations of
parameters of the declaration, which do affect whether two declarations
are identical, but not whether they denote the same entity, because it
would be invalid to have different declaration blocks for declarations
with the same name in the same scope. Therefore changes to the
declaration block are compatible, and it's useful for such changes to
not affect the fingerprint.
This is not easy to test in isolation with our current testing
machinery. However, a follow-on PR will change the name of a parameter
in the prelude, and with this in place, will not cause any changes to
occur elsewhere in the toolchain tests.
This change deliberately breaks away from the line/column ordering, and
instead focuses on a last byte offset corresponding to the final token
processed as part of producing the message. Where that's equal, this
maintains stable ordering in order to reflect the order that diagnostics
were produced.
The intent of this approach is that lex, parse, and check diagnostics
are interleaved based on where they are produced, but that
subexpressions still have diagnostics emitted prior to containing
expressions. In particular, the prior line/column sort essentially
sorted on the _start_ of where a diagnostic was associated, and this is
closer to sorting based on the _end_. As a consequence, something like
`F(1 2)` will have the error for `1 2` emitted _before_ a diagnostic for
`F(1 2)` not matching parameters, instead of _after_.
In check, we track the last handled node. This provides a
last_byte_offset _separate_ from where a diagnostic is associated. The
intent is that this creates an ordering of diagnostics which may be
associated with earlier code, to cause the diagnostics to be emitted
later. An example consequence of this is the change in ordering of
modifier diagnostics: we are diagnosing those from the same place, but
they have the same last_byte_offset, so we print them out in the order
produced.
I've added similar tracking to parse, but cannot identify any test which
is affected by it (note the separate commit, I thought about this late).
I'm not sure whether we have good out-of-order errors we could produce
for this.
A significant number of tests have reordered diagnostics as a
consequence of this change, so this change does not add further testing.
Use it in the instruction namer to make instruction names more stable
across unrelated changes to the toolchain or the prelude.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Jon Ross-Perkins
Dana JansensandJon Ross-Perkins
Richard Smith
Alina Sbirlea
Calvin
David Blaikie