Skip to content
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

Refactoring to allow for new verification back ends #1329

Merged
merged 8 commits into from
Feb 27, 2023

Conversation

JakuJ
Copy link
Contributor

@JakuJ JakuJ commented Feb 19, 2023

Related to #1321 – this PR aims to abstract away the notion of a verification back end behind an enum, in order to facilitate implementation of new back ends.

Had to make env on viper::Verifier public to do this. I cannot put the call to program.to_viper() in viper since that would be a cyclic dependency to prusti-common, and returning an AstFactory from some method would cause E0502 on call to viper.verify().

@vakaras vakaras requested a review from fpoli February 22, 2023 14:19
Copy link
Contributor

@vakaras vakaras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move all the Java things into Backend::Viper? For example, the construction of the Viper object, build Viper program closure, etc.

@fpoli
Copy link
Member

fpoli commented Feb 22, 2023

the construction of the Viper object

We just discussed this with Vytautas: it should be enough to store Viper inside an Arc<Lazy<..>> instead of inside Arc<..> as it is now. There us no need to move the local variable somewhere else. See the Lazy documentation.

@fpoli
Copy link
Member

fpoli commented Feb 22, 2023

If you remove the verification_context: &'v VerificationContext<'t> argument and make it a field of `

enum Backend<'v> {
    Viper { verifier: viper::Verifier<'v>, verification_context: Rc<VerificationContext<'v>> }
}

then making the env field public should no longer be needed. Let us know if this suggestion causes weird lifetime errors.

@fpoli
Copy link
Member

fpoli commented Feb 22, 2023

etc

The ast_utils.with_local_frame(16, || { .. }) wrapper can be moved too inside the Backend::verify method, so that it's used only for the Viper verifiers.

@JakuJ
Copy link
Contributor Author

JakuJ commented Feb 23, 2023

I made it so that the viper object is now initialized lazily, both on the server as well as in the prusti-viper package. Reorganized code around the backend call. Let me know how it looks now.

Copy link
Contributor

@vakaras vakaras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@fpoli Could you please check that Lazy is used as you intended?

Copy link
Member

@fpoli fpoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lazy is perfect, thanks! The dump disappeared and the log messages need to be updated, then it's good to be merged.

prusti-server/src/backend.rs Outdated Show resolved Hide resolved
prusti-server/src/process_verification.rs Show resolved Hide resolved
prusti-server/src/backend.rs Outdated Show resolved Hide resolved
prusti-server/src/backend.rs Show resolved Hide resolved
@JakuJ JakuJ requested a review from fpoli February 24, 2023 20:40
prusti-server/src/process_verification.rs Outdated Show resolved Hide resolved
prusti-server/src/backend.rs Show resolved Hide resolved
@fpoli
Copy link
Member

fpoli commented Feb 27, 2023

I rebased on master.

@vakaras
Copy link
Contributor

vakaras commented Feb 27, 2023

The CI failure seems to be unrelated to this PR. @fpoli If you agree, please merge.

@fpoli fpoli merged commit e4896c0 into viperproject:master Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants