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

fix: Verify that all compilable expressions are compilable #2641

Merged
merged 11 commits into from
Aug 26, 2022

Conversation

RustanLeino
Copy link
Collaborator

This PR updates resolution bookkeeping information that tells the verifier to check certain expressions to be compilable. This information was previously missing, except in the case of assignment statements.

Fixes #2640

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

Copy link
Member

@MikaelMayer MikaelMayer left a comment

Choose a reason for hiding this comment

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

Great work ! That surely fixes latent soundness issues.
A few remarks to enhance this PR, I'm happy to merge it afterwards.

Source/Dafny/Util.cs Outdated Show resolved Hide resolved
Source/Dafny/Util.cs Outdated Show resolved Hide resolved
@@ -2,9 +2,9 @@ ArrayElementInitResolution.dfy(16,26): Error: array-allocation initialization ex
ArrayElementInitResolution.dfy(23,23): Error: array-allocation initialization expression expected to have type 'nat ~> D' (instead got 'D') (perhaps write '_ =>' in front of the expression you gave in order to make it an arrow type)
ArrayElementInitResolution.dfy(38,33): Error: array-allocation initialization expression expected to have type '(nat, nat, nat) ~> char' (instead got 'int -> char')
ArrayElementInitResolution.dfy(55,31): Error: array-allocation initialization expression expected to have type '(nat, nat, nat, nat, nat) ~> D' (instead got 'D') (perhaps write '(_, _, _, _, _) =>' in front of the expression you gave in order to make it an arrow type)
ArrayElementInitResolution.dfy(64,21): Error: ghost fields are allowed only in specification contexts
ArrayElementInitResolution.dfy(64,21): Error: a ghost function is allowed only in specification contexts
Copy link
Member

Choose a reason for hiding this comment

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

Nice improvement of error reporting :-)

/*
module Types {
datatype XY =
| {:hello} D0(x: int)
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of these annotations?

module Types {
datatype XY =
| {:hello} D0(x: int)
| ghost {:bye} G0(y: bool)
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow I did not know you could have ghost variants.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't. Yet. Sorry, my cherry picking must have included these by accident. I didn't intend to include this file in this PR.

s := 3;
}

if xy1.D1? { // error: xy1 might be of a ghost variant
Copy link
Member

Choose a reason for hiding this comment

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

Excellent error catching.

}

method Bad() returns (r: int) {
if C? { // error: cannot ask about C? since ghost variant A? is a possibility
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure of this one. How come ghost variant A? is a possibility if we are in compiled code?

@@ -0,0 +1,252 @@
// RUN: %dafny_0 /compile:0 "%s" > "%t"
// RUN: %diff "%s.expect" "%t"
/*
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of commenting the entire test? Any plans to remove the comments?

}
}

// TODO: resolve and verify update expressions
Copy link
Member

Choose a reason for hiding this comment

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

TODO remaining here.

return false;
}

} else if (expr is MemberSelectExpr selectExpr) {
if (selectExpr.Member != null && selectExpr.Member.IsGhost) {
reporter?.Error(MessageSource.Resolver, selectExpr, "ghost fields are allowed only in specification contexts");
string what;
switch (selectExpr.Member) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please create a new field for all members, named like WhatKindWithExplicitGhostness or something similar and preferably shorter, that will return what you compute in the variable "what"?
I bet this will be useful for reporting other errors, so it might be worth it moving it there..

Copy link
Member

@MikaelMayer MikaelMayer left a comment

Choose a reason for hiding this comment

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

Great, thanks for fixing these soundness issues !

@RustanLeino RustanLeino enabled auto-merge (squash) August 26, 2022 19:27
@RustanLeino RustanLeino merged commit c1e3d5c into dafny-lang:master Aug 26, 2022
@RustanLeino RustanLeino deleted the issue-2640 branch August 26, 2022 20:55
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.

Restrictions for compilable expressions not enforced
2 participants