This PR is making two main changes to the Explorer and Toolchain:
- Replace the `is` keyword in `where SomeType is SomeInterface` with `impls`, so it is `where SomeType impls SomeInterface`
- Rewrite uses of the "impls" to something else to avoid, frequently "`impl` declarations" or "implementations", to avoid confusion with the `impls` keyword.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
The strategy that we use for now to support template instantiation is to check the impl declaration as if it were a generic, but to defer all checking of the impl definition until we see a use in which all template parameters have arguments. At that point, we clone the impl definition and type-check the whole thing, with constant values set on the template parameters corresponding to the given arguments.
No caching of template instantiations is performed yet; each time we form a reference to a template instantiation, we instantiate it afresh. We also don't implement the name lookup rule from #949 yet; lookups during template instantiation look only in the actual type and not in the constraint.
Depends on #2699
### Features
* Add virtual `destructor`s support (Closes#2521)
* Check virtual override for virtual destructors
* Error if attempting to `Delete` a class that does not have virtual destructors from a base class pointer
### Implementation
* Update parser to support virtual override introducers for destructors
* Check virtual override for class destructor and add to class vtable if necessary
* Add corresponding tests
### Notes
Contrary to initial implementation, this implementation leverages the `Address` structure and implements a new `Address::DowncastedAddress()` method to get address from child most class from a base class address. This avoids the need to use `GetAllocationId` and its issues when it comes to having multiple values for an `AllocationId`.
### Next work
Following this PR, we need to:
* Check when using `Delete` that the class was allocated with `New` (WIP)
* Drop the old `GetAllocationId(Value*)` in favor of a better system (WIP)
This is intended to be used for template instantiation, but for now takes no stance as to what it's cloning.
This is tested by parsing and cloning all of our test files, and checking that the result of converting each AST to proto is the same as the result of cloning and then converting each AST to proto.
This is in preparation for template instantiation being triggered during substitution, and being able to fail.
Fix rule-of-three violation (missing assignment operator) in `Error` that got in the way of using it to hold an error temporarily in a failed transformation.
As described in [the generics design](https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/generics/details.md#type-structure-of-an-impl-declaration), `impl` declarations are prioritized by type structure. Given two `impl` declarations that match a `type as interface` query, the one that describes the longest prefix of the query without using placeholders is preferred.
We implement this by putting all impls in a total order, first by type structure equivalence classes and then by lexical order. When matching an impl, we walk this total order, and stop once we find a match and reach the end of its equivalence class.
Equivalence classes are determined by finding the locations of the "holes" (the positions where deduced parameters appear) within the type structure, viewed as a tree. Two impls are in the same equivalence class if their holes are in the same place, and equivalence classes are ordered based on a reverse lexicographical ordering of their holes.
Explorer doesn't keep the `Bindings` list for a parameterized type in any particular order, but the type structure rule requires that we consider them in lexical order. In order to support this, we now track an index on the declared parameters of each generic. This is a simple numbering of enclosing generic parameters, both on that generic and on all lexically enclosing generics.
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This was bugging me after I saw all the strings; it feels like this is why we have EnumBase on the toolchain side.
I've included the move of EnumBase to //common because I figured it's reasonable to evaluate together; if we don't want EnumBase in this case, it doesn't make sense to move.
These are used by the AST in lots of ways, and this resolves various layering issues.
This means that `AllocationId` also lives in ast/, but is managed by interpreter/. A better layering here would be desirable, but this seems good enough for the time being.
These functions are dangerous, as they check whether conversions are possible without actually performing the conversions. In each case where they were used, explorer would crash in some cases if a user-defined conversion is required.
This change moves us more towards implicit conversions being handled by a regular function call on an interface and away from them being magical builtins. Unfortunately, this exposes a pre-existing bug that a call of the form `x.(ImplicitAs(T).Convert)()` compiles even if `x` only has an explicit conversion to `T`. That's worked around here for now, but will need a proper fix later.
google-readability-function-size and readability-function-size were _both_ triggering on TypeCheckExp. It looks like the Google version may be a subset of the general version, so I've disabled the Google version while keeping the general version and adding a NOLINT for it.
I manually removed the `const` in cases like `Nonnull<const VTable* const>` based on the readability-const-return-type warning. i.e., where a return type is a pointer, the `const` isn't meaningful and the tidy check was warning about that.
Added a NOLINT for misc-definitions-in-headers on IsRecursivelyTransformable. I think that's the right choice for the `constexpr`, the warning didn't feel accurate and may be getting confused by the templating.
I changed the structure of `carbon_files` in the fuzzer because the `new` was causing a warning about exceptions. However, also disabling bugprone-exception-escape because it's what was flagging this and it's not really a helpful warning.
Other changes were automated.
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Right now //explorer/fuzzing:explorer_fuzzer takes my machine 80s to run, just because of the corpus size. The corpus is actually pretty small compared to the toolchain fuzzer, so reducing the corpus size doesn't feel quite right.
This adds support for sharding fuzz tests, and with 8 shards each is closer to 10s. This should put it closer to the noise of other explorer tests in terms of runtime.
Unfortunately I'm not seeing a sharding flag in the llvm library, which seems fair. However, that's why I'm working around it by creating separate test targets per shard, then a suite to merge them back together. The use of `shard_count` for this is idiomatic for bazel rules; I'm using it so that switching implementations should be low-impact if that's ever needed.
Co-authored-by: Adrien Leravat <Pixep@users.noreply.github.com>
This fixes some bugs in each, where the fixes had only been made on one
side of the switch or the other. Also don't forget to instantiate
deduced generic arguments in a call when we read them out of the AST.
This is intended to address currently flaky timeouts that are likely caused by the size of the prelude. I'm addressing a performance bottleneck in AnalyzeProgram with trace output. Trying to omit prelude traces reduces most trace output significantly, and I think it'll scale better as the prelude size increases.
The basic mechanics here are:
- In order to consistently track whether tracing is on, I've added a TraceStream class, explorer/interpreter/trace_stream.h.
- The AST now has a num_prelude_declarations field, so that it's provided where the boundary is.
- In order to mark where we try to skip prelude output, I've added calls to set_in_prelude in type_checker.
- In exec_program, I just use num_prelude_declarations directly to skip over.
- Everywhere checks TraceStream::is_enabled before printing, similar to the std::optional check that was previously used.
This does add some timing output in order to better diagnose where slowness is coming from, when tracing. It also adds "verbose" targets to make it easier to get the trace output.
So for example, here's a timing for zero.carbon:
```
Timings:
- Parse: 13ms
- AddPrelude: 25ms
- AnalyzeProgram: 116ms
- ExecProgram: 12ms
```
If I make a small change to just not set skipping_prelude (essentially getting back to current output):
```
- Parse: 13ms
- AddPrelude: 25ms
- AnalyzeProgram: 2359ms
- ExecProgram: 57ms
```
Thus in this trivial example, I'm eliminating about 95% of the execution time.
Note this approach could still be refined in a few ways:
- We could add a flag to allow overriding in_prelude. It should be a small amount of work after this change. But it's a little consistent with how parser_debug works, that it won't print prelude output by default (unless there's an error).
- Execution could skip messages involving initialization of globals declared in the prelude. This is a little noisy right now, but I don't think it's significant for performance because ExecProgram is tiny.
- Once files are more separated, we should be able to change the num_prelude_declarations/set_in_prelude approach.
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
The intent here is that changes to prelude.carbon shouldn't break every test that expects some error from prelude.carbon; that would be too fragile. As a consequence, this effectively ignores the line number in prelude.carbon.
This is a little complex because we don't know which line in the original source file is actually causing the error, just that there is an error. Also, the previous look-behind approach required a fixed-with prefix, whereas we want a little more than that in order to capture the filename for comparison.
This would be hard to do with extra_check_replacement because the path to bazel.runfiles is complex to calculate. As a consequence, this is basically all new code.
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Both cases need a label for each level of struct and tuple type appearing in the impl query, because we can have an unbounded number of such levels with the types otherwise being the same.
Prior to this, both added testcases exhibit unbounded recursion.
Detect when evaluating an impl recursively tries to evaluate the same impl for the same or a more complex set of parameters.
In order to perform the check after we have tested that the type structure matches and before we check that constraints are recursively satisfied, argument deduction is extended to check structural matching properties earlier.
This requires us to separate match failures into two kinds: hard failures that produce errors that should never be swallowed, and soft failures such as a missing impl that lead us to merely discard an impl as a candidate. A flag has been added to `ImplScope` and `ArgumentDeduction` to specify whether soft failures should produce an error message or not.
The design of choice types expects the declaration of an alternative to match the usage: if an alternative is declared as `None`, then it should be used as `None` not `None()`, and if it is declared as `None()` then it should be used as `None()` not `None`. Update explorer to match.
Also clean up the handling of choice types and alternative values a little in general, by moving away from identifying choice types and alternatives as strings and towards identifying them symbolically.
Closes#2422
Return Integer overflow Runtime error for bit shift if The second operand is less than zero or bigger than or equal bit width of the first operand in this case (32 bit for int)
Issue #2595
Support for global variables is still missing; they're a bit more tricky because they use a pattern to introduce their name.
Prior to this change, explorer heavily relied on name comparisons to determine whether two declarations declare the same entity. Some of those instances are fixed in this PR, but more remain to be fixed, and some TODOs are added for some harder-to-fix instances.
Name lookup into namespaces needs to be resolved early, as part of name resolution, so that we can properly diagnose references to entities before they are fully declared. Make name resolution set a target `value_node` on simple member accesses that name namespace members, and in type-checking rewrite those member accesses into `IdentifierExpression`s that directly reference the namespace member.
First step towards permitting declarations within namespaces. Supports only functions within namespaces for now, with no way to call those functions except from within other such functions. Unqualified lookups within a function in a namespace look in that namespace first.
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
As per the `TODO` comment in the file, the `CallableDeclaration::name` property was refactored into `FunctionDeclaration`, which is currently the only namable callable declaration kind. I was able to rely on the existing `GetName` function to replace call sites of the accessor function with nearly identical swaps.
Fixes#2531
Move deallocation logic to `StepCleanUp` to have all necessary cleanup actions in the same place. Currently this means `DestroyAction` and `heap_.Deallocate`.
Fix variables not being properly cleaned up if declared before an unformed variable.
### Details
Currently the following example / test
```
package ExplorerTest api;
class A {
var i: i32;
destructor[self: Self] {
Print("Destructor A {0}", self.i);
}
}
fn Main() -> i32 {
var a0: A;
var a1: A = {.i = 1};
var a2: A;
var a3: A = {.i = 3};
return 0;
}
```
prints
```
Destructor A 3
```
instead of
```
Destructor A 3
Destructor A 1
```
This PR fixes the issue in the `CleanUp` logic.
Add support for user-defined assignment, as well as compound assignment and increment, following the design direction in pending proposal #2511.
Some of this isn't fully testable yet: because explorer doesn't properly support `impl` specialization, the blanket `impl`s in the prelude prevent types from customizing assignment.
Naming this ToString within the Carbon namespace encourages incidental overloading of the function, and that doesn't seem to be intentional; this feels questionable [under overloading style](https://google.github.io/styleguide/cppguide.html#Function_Overloading). OperatorToString seems helpful in that it makes it easy to see at a glance where it's being used.
Allow explicit conversion from a tuple of `T1`, `T2`, ... to a tuple of `U1`, `U2`, ... if each element has an explicit conversion.
Also add a comment to existing `ImplicitAs` impl for tuples explaining its purpose.
Add a check to prevent creating a class from a struct with a missing parent.
This is already type-checked for carbon / user code, but not when using `Convert` manually.
Add a blanket `ImplicitAs` implementation to perform the conversions that explorer can perform as built-in conversions. This allows those conversions to be detected by constraints and to be used as part of other user-defined conversions. For now, a single monolithic conversion is exposed. I intend to split this up into multiple smaller conversion kinds for each kind of conversion in a follow-up change.
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Add basic support for `match_first` declarations to explorer, as described in https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/generics/details.md#prioritization-rule. The concrete syntax here is not yet approved, so `match_first` is renamed to `__match_first` for now, but the semantics are necessary to implement other approved features in explorer so we're intentionally getting a little ahead of the design here.
Improves the error produced when 'Main' is declared with parameters. Resolves a TODO in the typechecker. Explorer only.
PR also adds a test for this error message.
Jon Ross-Perkins
josh11bandGeoff Romer
Richard Smith
Adrien Leravat
Amr Hesham
Calvin
Carson Radtke