-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes: Rust supports overriding traits #5948
base: master
Are you sure you want to change the base?
Conversation
Fixed support for hashing and equality for traits overrides Support for static functions and methods
… feat-rust-support-overriding-trait
# Conflicts: # Source/DafnyCore/Backends/Dafny/AST.dfy # Source/DafnyCore/Backends/Dafny/DafnyCodeGenerator.cs # Source/DafnyCore/Backends/Rust/Dafny-compiler-rust-definitions.dfy # Source/DafnyCore/Backends/Rust/Dafny-compiler-rust-rast.dfy # Source/DafnyCore/Backends/Rust/Dafny-compiler-rust.dfy # Source/DafnyCore/Backends/SinglePassCodeGenerator/SinglePassCodeGenerator.cs # Source/DafnyCore/GeneratedFromDafny/DAST.cs # Source/DafnyCore/GeneratedFromDafny/DCOMP.cs # Source/DafnyCore/GeneratedFromDafny/Defs.cs # Source/DafnyCore/GeneratedFromDafny/RAST.cs # Source/DafnyRuntime/DafnyRuntimeRust/src/lib.rs # Source/DafnyRuntime/DafnyRuntimeRust/src/tests/mod.rs # Source/IntegrationTests/TestFiles/LitTests/LitTest/comp/rust/traits-datatypes.dfy # Source/IntegrationTests/TestFiles/LitTests/LitTest/comp/rust/traits-datatypes.dfy.expect
# Conflicts: # Source/DafnyCore.Test/GeneratedFromDafny/DefsCoverage.cs # Source/DafnyCore/Backends/Rust/Dafny-compiler-rust-rast.dfy # Source/DafnyCore/Backends/Rust/Dafny-compiler-rust.dfy # Source/DafnyCore/GeneratedFromDafny/DCOMP.cs # Source/DafnyCore/GeneratedFromDafny/Defs.cs # Source/DafnyCore/GeneratedFromDafny/RAST.cs
# Conflicts: # Source/DafnyCore/Backends/Dafny/DafnyCodeGenerator.cs # Source/DafnyCore/Backends/Rust/Dafny-compiler-rust.dfy # Source/DafnyCore/Backends/SinglePassCodeGenerator/SinglePassCodeGenerator.cs # Source/DafnyCore/GeneratedFromDafny/DCOMP.cs # Source/DafnyCore/Resolver/ModuleResolver.cs # Source/DafnyCore/Resolver/TypeCharacteristicChecker.cs
Caching of compilation name. Don't copy target file if same path
# Conflicts: # Source/DafnyCore/Backends/Dafny/DafnyCodeGenerator.cs # Source/DafnyCore/Backends/SinglePassCodeGenerator/SinglePassCodeGenerator.cs
Some high level comments: I'm mainly just concerned about the soundness and implementation of trait downcasting. It looks like you do this in phases:
I don't understand why the latter can't be done earlier - is there not a subtype check method in the type system refresh? |
Thank you very much for reading the PR and trying to understand the underlying logic. That's already a lot of work.
You're right that there is an issue here. The problem is that in other backends, it's fine to do something like this:
This is because other back-ends keep run-time information of the type and types are behind single references. The only sensible way to do trait downcast is to add special methods to the trait X {} so that we can test if it's a Y {} for some T. However, again, because of monomorphization, Rust prevents type parameters on functions that are in a trait with a function. Hence, the only way to do a downcast test and instantiation is if the type parameters of the subtrait can be obtained from the type parameters of X. This is what I am computing in This means indeed that users who try to downcast One problem with doing it earlier is that the subtype check depends on whether the instantiated type parameters support |
I see, you add the coercion methods at code generation time, which is why Approved |
@@ -2114,6 +2114,12 @@ void ResolveParentTraitTypes(TopLevelDeclWithMembers cl, Graph<TopLevelDeclWithM | |||
$"{cl.WhatKind} '{cl.Name}' is in a different module than trait '{trait.FullName}'. A {cl.WhatKind} may only extend a trait " + | |||
$"in the same module, unless the parent trait is annotated with {{:termination false}} or the {cl.WhatKind} with @AssumeCrossModuleTermination."); | |||
} | |||
|
|||
if (cl is TraitDecl td) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the child trait contains abstract members not in the parent trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's ok, because if the underlying struct declares it can be downcasted to the trait by implementing the Downcast trait itself and returning true to the method _is, it also has to return an instance of the child trait in the method _as, and because it has nothing else than itself to return, it also needs to implement the abstract methods of the child, so at the end it's sound.
The structs that don't implement the child trait will return false on _is and panic on _as.
@@ -49,6 +49,13 @@ private static void InferEqualitySupport(List<TopLevelDecl> declarations, ErrorR | |||
} | |||
} | |||
} | |||
|
|||
foreach (var parentType in dt.ParentTraits) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but InferEqualitySupport
would be easier to read in functional style (you wouldn't have to exit a doubly-nested loop). No changes needed.
/// trait Sub<A> extends Parent<Int> where trait Parent<X> | ||
/// ==> false and null | ||
/// </summary> | ||
private static bool CanDowncast(Type parentTrait, TraitDecl subTrait, out Type downcastType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK: now I see the connection to the population of the new field in TraitDecl. Why can't this computation be done at that time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there not an existing mechanism to do a subtype check on two types/traits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For your first question:
This method has the side-effect of not just checking if the traits can be downcasted, but also given a type of the parent trait (with instantiated type arguments), it gives the type of the sub-trait (with instantiated arguments using the type parameters of the parent trait). This can't be done at the time of gathering trait dependencies because type parameters are not known. We could, however and at resolution time, determine if that construction is possible and store a function that, given the instantiated type of the parent, returns the instantiated type for each sub-trait that is possible, using a map.
Would you prefer me to do this instead?
For your second question
For Rust (the main consumer of the DafnyCodeGenerator now), references have a virtual dispatch table pointer if they represent a trait. Since everything must be known at compile-time, the comment of CanDowncast explains exactly what is necessary for us to be able to even mention that a trait is downcastable to another trait; that is, they need to share type arguments so that the type of the sub-trait must be expressible with the arguments of the upper trait. That's a slight restriction, and I should probably move it to a resolution error if we know that the target compiler is Rust.
In practice, this restriction has been satisfied in the large scale projects that I was helping translate to Rust, that is, there is usually no downcast by adding a type parameter that was not present in the parent trait.
Would you prefer that than having a resolution error instead as described?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I would prefer to have that (1) / a function that returns a delayed computation on the parent's concrete type into the concrete type for each child trait if it's not too difficult.
OTOH for (2) I think what you're doing currently is fine. Special casing the resolver depending on the backend seems morally incorrect
@@ -2114,6 +2114,12 @@ void ResolveParentTraitTypes(TopLevelDeclWithMembers cl, Graph<TopLevelDeclWithM | |||
$"{cl.WhatKind} '{cl.Name}' is in a different module than trait '{trait.FullName}'. A {cl.WhatKind} may only extend a trait " + | |||
$"in the same module, unless the parent trait is annotated with {{:termination false}} or the {cl.WhatKind} with @AssumeCrossModuleTermination."); | |||
} | |||
|
|||
if (cl is TraitDecl td) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's ok, because if the underlying struct declares it can be downcasted to the trait by implementing the Downcast trait itself and returning true to the method _is, it also has to return an instance of the child trait in the method _as, and because it has nothing else than itself to return, it also needs to implement the abstract methods of the child, so at the end it's sound.
The structs that don't implement the child trait will return false on _is and panic on _as.
Description
The PR #5940 defined a preliminary support for value traits.
This PR fixes everything that encompasses value traits and newtypes. Here is the list of fixes.
Also some enhancements
{:extern}
on types requiring a hint when it can simply take the Dafny name, like for modules and classes.DafnyCodeGenerator.GenType
now caches its result to avoid exponential computationsI also updated the documentation regarding the use of externs
How has this been tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.