- 
                Notifications
    
You must be signed in to change notification settings  - Fork 124
 
Add support for Opaque types defined by function returns #1785
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
base: main
Are you sure you want to change the base?
Conversation
| 
           Passing all the tests now. I'm looking at all the usages of   | 
    
| 
           @tjhance fixed most of the things. Would you take another look? TY!  | 
    
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.
This mostly looks good to me; with respect to the code, I only have some minor nitpicks.
The last main thing I would like to see is a few more tests regarding interacting features:
- A function marked 
external_bodywhich returns an opaque type. - An 
assume_specificationfor a function that returns an opaque type. - A function with a 
returnsclause and which returns an opaque type. - A function with a 
returnsclause and which returns an opaque type, and which is markedallow_in_spec 
It's fine if some of these are "not implemented" errors, I mostly want to get tests in so it's clear what the expected behavior is in these cases.
| 
           @tjhance  | 
    
| 
           @tjhance ping in case the email notification didn't work  | 
    
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.
Sorry for the delay. This looks good to me.
        
          
                source/vir/src/ast_visitor.rs
              
                Outdated
          
        
      | TypX::ConstInt(_) => R::ret(|| typ.clone()), | ||
| TypX::ConstBool(_) => R::ret(|| typ.clone()), | ||
| TypX::Air(_) => R::ret(|| typ.clone()), | ||
| TypX::Opaque { .. } => R::ret(|| typ.clone()), | 
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.
This needs to visit the args, like the TypX::Datatype and TypX::Primitive cases.
        
          
                source/vir/src/ast.rs
              
                Outdated
          
        
      | pub typ_bounds: GenericBounds, | ||
| } | ||
| 
               | 
          ||
| pub type Opaquetype = Arc<Spanned<OpaquetypeX>>; | 
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.
Minor comment: OpaqueType would be better than Opaquetype, since it's two words (whereas "datatype" is often just one word).
| 
           @FeizaiYiHao Is this ready to be merged?  | 
    
          
 Wow sorry. Somehow the notification didn't work... Will quickly address the comments from @Chris-Hawblitzel and resolve the conflicts and then it should be good to be merged.  | 
    
| 
           Great, thanks!  | 
    
… in opaque function
| 
           @parno Somehow the comments from Chirs were addressed a long time ago but the they poped out again in the threads. But anyways, this is ready to be merged. Sorry for the delay  | 
    
| 
           I found an unhandled case: for nested opaque types, we are missing the equality statements in AIR generation. Let me quickly fix and push again. Sorry. Example:  | 
    
| 
           OK. fixed. Ready to be merged  | 
    
This PR adds basic support for opaque types defined by function returns. The changes include:
external.rs, all opaque type definitions are visited, and only opaque types underexternalfunctions are marked asVerifOrExternal::External, since the opaque type is part of the function declaration.TypX::Opaque{def_path:Path,args:Typs}is added, in which thedef_pathis the path where the opaque type is defined. Andargsare the type parameters used to instantiate this instance of the opaque type.OpaquetypeX{pub name: Path, pub typ_params: Typs, pub typ_bounds: GenericBounds,}is added for each opaque type definition. AKrateXcarries a list ofOpaquetypeXthat are used in the crate.OpaquetypeXgenerates two functionsDCR%pathandTYPE%path(or consts if takes no args) that return aDcrandTypefor given args. And also generates a range of axioms to encode the trait bounds for the instantiated opaque types.args, we instantiate the opaque type by calling theDCR%path argsandTYPE%path argsfunctions.If in
opaque.rswe have the following Rust codeProduces the following z3 code
This seems to also enable returning
impl Fn().TODO
OpaqueTypost-analysistonon_body_analysisto stop normalizing opaque types based on the body of the functions that define the opaque types. I'm not sure if I have changed all the right places.By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.