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: Use correct type for map update expression #3060

Merged
merged 9 commits into from
Nov 18, 2022
3 changes: 2 additions & 1 deletion Source/DafnyCore/Resolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15074,7 +15074,8 @@ public void ResolveExpressionX(Expression expr, ResolutionContext resolutionCont
ResolveExpression(e.Index, resolutionContext);
ResolveExpression(e.Value, resolutionContext);
AddXConstraint(expr.tok, "SeqUpdatable", e.Seq.Type, e.Index, e.Value, "update requires a sequence, map, or multiset (got {0})");
expr.Type = e.Seq.Type;
var ty = PartiallyResolveTypeForMemberSelection(expr.tok, e.Seq.Type);
expr.Type = ty;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var ty = PartiallyResolveTypeForMemberSelection(expr.tok, e.Seq.Type);
expr.Type = ty;
expr.Type = PartiallyResolveTypeForMemberSelection(expr.tok, e.Seq.Type);

Copy link
Member

Choose a reason for hiding this comment

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

This does not look right to me — there's no member selection here. Instead, shouldn't we just revert to the base type? (e.Seq.Type with the constraints removed?)

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't because at this point the type are not resolved. We first need to resolve identifier names (such as "foo"), hence the need to "partially resolve")


} else if (expr is DatatypeUpdateExpr) {
var e = (DatatypeUpdateExpr)expr;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
namespace Microsoft.Dafny;

public class NonZeroExitCodeIfErrorsOption : BooleanOption {
public static readonly NonZeroExitCodeIfErrorsOption Instance = new();
public override object DefaultValue => true;
public override string LongName => "nonzero-exit-code-if-errors";
public override string ShortName => null;
public override string ArgumentName => "boolean";

public override string Description => @"
if false then always exit with a 0 exit code, regardless of whether errors are found.
If true (default) then use the appropriate exit code.".TrimStart();
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit uncommon to have such a flag. Shouldn't we find another solution in the test suite itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already have this option in a substantial portion of our 1400+ tests, whenever you have %dafny_0, it means you have this flag.
Since %baredafny verify only takes new arguments, we need to expose this option to make it work.
I don't know of another simple way.

What we could do is hide this option to users, but I don't know how to do that. @keyboardDrummer ?

Copy link
Member

Choose a reason for hiding this comment

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

But the description of that flag says "deprecated" in the old CLI:

  /countVerificationErrors:<n>
      (deprecated)
      0 - Set exit code to 0 regardless of the presence of any other errors.
      1 (default) - Emit usual exit code (cf. beginning of the help message).

Copy link
Member

Choose a reason for hiding this comment

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

I think for this test we should just use the old CLI. And we can then (in a separate PR) enhance LitTests.cs to accept || true at the end of a command line, which is the "right" way to ignore an exit code. WDYT?


public override string PostProcess(DafnyOptions options) {
options.CountVerificationErrors = Get(options);
return null;
}
}
1 change: 1 addition & 0 deletions Source/DafnyDriver/Commands/CommandRegistry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ static class CommandRegistry {
public static IReadOnlyList<IOptionSpec> CommonOptions = new List<IOptionSpec>(new IOptionSpec[] {
CoresOption.Instance,
VerificationTimeLimitOption.Instance,
NonZeroExitCodeIfErrorsOption.Instance,
LibraryOption.Instance,
ShowSnippetsOption.Instance,
PluginOption.Instance,
Expand Down
5 changes: 5 additions & 0 deletions Source/DafnyDriver/DafnyDriver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ static DafnyDriver() {
"--verification-time-limit=300"
};

public static readonly string[] NewDefaultArgumentsForTestingWithErrors =
NewDefaultArgumentsForTesting.Concat(new[] {
"--nonzero-exit-code-if-errors=false"
}).ToArray();

public static int Main(string[] args) {
int ret = 0;
var thread = new System.Threading.Thread(
Expand Down
1 change: 1 addition & 0 deletions Source/IntegrationTests/LitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ IEnumerable<string> AddExtraArgs(IEnumerable<string> args, IEnumerable<string> l
throw new Exception($"Unsupported OS: {RuntimeInformation.OSDescription}");
}

substitutions["%args_0"] = DafnyDriver.NewDefaultArgumentsForTestingWithErrors;
substitutions["%args"] = DafnyDriver.NewDefaultArgumentsForTesting;

var dafnyReleaseDir = Environment.GetEnvironmentVariable("DAFNY_RELEASE");
Expand Down
28 changes: 28 additions & 0 deletions Test/git-issues/git-issue-3059.dfy
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// RUN: %baredafny verify %args_0 "%s" > "%t"
Copy link
Member Author

Choose a reason for hiding this comment

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

@keyboardDrummer yay ! we can use the new CLI for tests now.

// RUN: %diff "%s.expect" "%t"

type foo = m: map<int, int> | forall n <- m.Keys :: m[n] < 5

function addToFoo(m: foo): foo
ensures false
{
m[1 := 7]
}

type seq0 = s: seq<int> | forall n <- s :: n == 0

function ReplaceInSeq0(s: seq0): seq0
requires |s| > 0
ensures false
{
s[0 := 1]
}

type map0 = m: map<int, int> | forall k <- m :: m[k] == 0

function ReplaceInMap0(m: map0): map0
requires 0 in m
ensures false
{
m[0 := 1]
}
7 changes: 7 additions & 0 deletions Test/git-issues/git-issue-3059.dfy.expect
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
git-issue-3059.dfy(9,3): Error: value does not satisfy the subset constraints of 'foo'
git-issue-3059.dfy(14,9): Error: A postcondition might not hold on this return path.
git-issue-3059.dfy(16,10): Related location: This is the postcondition that might not hold.
git-issue-3059.dfy(18,3): Error: value does not satisfy the subset constraints of 'seq0'
git-issue-3059.dfy(27,3): Error: value does not satisfy the subset constraints of 'map0'

Dafny program verifier finished with 3 verified, 4 errors
1 change: 1 addition & 0 deletions docs/dev/news/3059.fix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Use correct type for map update expression