Skip to content

Commit

Permalink
Fix static analysis and code actions (#5966)
Browse files Browse the repository at this point in the history
Fixes #5962

### Description
This
[commit](bf1aac6)
removed one optimization that was gathering declaration and assignment
in a single assignment in the Dafny intermediate representation. That
is, before that commit, this piece:
```dafny
[ DeclareVar(name, typ, None), Assign(name, rhs)]
```
was combined into a single
```dafny
DeclareVar(name, typ, Some(rhs))`
```
before being translated to Rust.
The commit above removed that optimization because it can detect that
"name" is going to be immediately assigned, and give it a type without
Maybeplacebo tracking (that tracks whether the variable was assigned at
all). And when optimizing the resulting Rust code, a declaration
followed by an assignment is combined into a single statement.

It seems for some reason that the Rust optimization was not applied in
the problem reported in #5962. I could not reproduce it, but because I
recognized that the optimization was not applied, I decided to reinstate
the optimization on the Dafny AST level itself.
Note that the splitting between the declaration and the assignment is an
artefact from the code generator. Dafny declare and assign are stored in
two different statements, which prevents shadowing variables. Indeed,
the following two pieces of code are not equivalent. The first one is
what Dafny resolves to, the second one is what users write:
```dafny
var name: int;
name := name + 2;
```
and
```dafny
var name: int := name + 2;
```
In the second case, the name refers to the variable name before itself,
whereas in the first case, the name is not assigned.
All other backends than Dafny-based backends add counters to variables
to disambiguate them. However, the Rust code generator has decent
shadowing rules so it makes sense to compile the Dafny
If the rhs contained "name", in Dafny it referred to a previous
eponymous variable. In Rust as well. However, in the intermediate
representation, since it does not add unique counters to variables, the
second one is translatable in Rust whereas the first one is not:
```rs
let mut name: DafnyInt := name + int!(2);
```
By reinstating this optimization on the Dafny intermediate
representation, it should make the code generation more robust in the
future.

The second error
[commit](bf1aac6)
introduced was that the static analyzer was faulty in the case of
if-then-else statements, as it would forget the part after the
if-then-else block and wrongly infer that a variable was not assigned
when it was actually unknown.
I fixed the analyzer the following way:
- I wrote a test case that was failing
- I fixed the analyzer.

Then, in an effort to make everything more robust,
- I created a simple "by method" part of the function so that it would
not need to extract subsequences and also could compute the function
lazily, only when it needs more information
- Once the by method passed, I modified the specification part so that I
would clearly separate 1) the part that analyzes the current statement,
if it assigns the variable and/or if it's a stop statement, and 2) the
continuation of the analysis on the rest of the sequence;

The function still matched the method body so I'm now pretty much more
confident of the analysis.

This PR also adds two more fixes in code actions I needed to figure out
termination
- The key for storing code action was not the same as the key for
retrieving it in a file system where one folder contained a space. The
first one was correct, but the second one was based on an URI converted
to JSON without going through string, so it contained a %20. This means
that no code actions could work when working with such files. By
ensuring the key is the same for both the cache storage and the client,
the code actions work again
- The insertion point of a code action was wrong. I reproduced the issue
on a small test case, and fixed it.

### How has this been tested?
- For the first issue, I temporarily deactivated the expression
optimization to reproduce the exact issue on an existing test case
(datatypes.dfy). I fixed it and verified it worked, and then put again
the expression optimization.
- I added a test in DefsCoverage for the second issue
- I added an intermediate variable for the key so that we ensure it's
the same in both places.
- I added a code action test for the fourth issue

<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
  • Loading branch information
MikaelMayer authored Dec 5, 2024
1 parent 81b8216 commit 5666571
Show file tree
Hide file tree
Showing 9 changed files with 671 additions and 513 deletions.
2 changes: 2 additions & 0 deletions Source/DafnyCore.Test/GeneratedFromDafny/DefsCoverage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ public static void Tests()
DefsCoverage.__default.Expect(object.Equals(Defs.__default.DetectAssignmentStatus(Dafny.Sequence<DAST._IStatement>.Concat(Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_Print(_2_z)), _3_assigns__x), _0_x), Defs.AssignmentStatus.create_SurelyAssigned()));
DefsCoverage.__default.Expect(object.Equals(Defs.__default.DetectAssignmentStatus(Dafny.Sequence<DAST._IStatement>.Concat(Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_Print(_2_z)), _3_assigns__x), _0_x), Defs.AssignmentStatus.create_SurelyAssigned()));
DefsCoverage.__default.Expect(object.Equals(Defs.__default.DetectAssignmentStatus(Dafny.Sequence<DAST._IStatement>.Concat(Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_While(_2_z, Dafny.Sequence<DAST._IStatement>.FromElements())), _3_assigns__x), _0_x), Defs.AssignmentStatus.create_Unknown()));
DefsCoverage.__default.Expect(object.Equals(Defs.__default.DetectAssignmentStatus(Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_If(_5_cond, _3_assigns__x, Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_If(_5_cond, _3_assigns__x, _4_assigns__y)))), _0_x), Defs.AssignmentStatus.create_Unknown()));
DefsCoverage.__default.Expect(object.Equals(Defs.__default.DetectAssignmentStatus(Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_If(_5_cond, Dafny.Sequence<DAST._IStatement>.Concat(Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_If(_5_cond, Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_Return(_2_z)), Dafny.Sequence<DAST._IStatement>.FromElements())), _3_assigns__x), Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_If(_5_cond, Dafny.Sequence<DAST._IStatement>.Concat(Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_If(_5_cond, Dafny.Sequence<DAST._IStatement>.FromElements(DAST.Statement.create_Return(_2_z)), Dafny.Sequence<DAST._IStatement>.FromElements())), _3_assigns__x), _4_assigns__y)))), _0_x), Defs.AssignmentStatus.create_Unknown()));
}
public static Dafny.ISequence<Dafny.Rune> IND { get {
return RAST.__default.IND;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,15 @@ module {:extern "DefsCoverage"} DafnyToRustCompilerDefinitionsCoverage {
Expect(DetectAssignmentStatus([Print(z)] + assigns_x, x) == SurelyAssigned);
Expect(DetectAssignmentStatus([Print(z)] + assigns_x, x) == SurelyAssigned);
Expect(DetectAssignmentStatus([While(z, [])] + assigns_x, x) == Unknown);
Expect(DetectAssignmentStatus([If(cond, assigns_x, [If(cond, assigns_x, assigns_y)])], x) == Unknown);
Expect(
DetectAssignmentStatus(
[ If(cond,
[If(cond, [Return(z)], [])] +
assigns_x,
[If(cond,
[If(cond, [Return(z)], [])] +
assigns_x,
assigns_y)])], x) == Unknown);
}
}
65 changes: 47 additions & 18 deletions Source/DafnyCore/Backends/Rust/Dafny-compiler-rust-definitions.dfy
Original file line number Diff line number Diff line change
Expand Up @@ -778,37 +778,66 @@ module {:extern "Defs"} DafnyToRustCompilerDefinitions {
if SurelyAssigned? then SurelyAssigned
else if NotAssigned? then other
else Unknown // It's not as simple. If there are are two paths leading to one being assigned, the other not,
// Rust won't be albe to figure out the rules
// Rust won't be able to figure out the rules
}
}
// What could be problematic is the presence of branches in the assignment
// and one branch where a value is not assigned at all.
// If return false, we don't know if it's assigned or not
/** Detects if a given variable can be detected to be surely assigned or surely unassigned by the Rust compiler */
function DetectAssignmentStatus(stmts_remainder: seq<Statement>, dafny_name: VarName): AssignmentStatus {
if |stmts_remainder| == 0 then NotAssigned else
var stmt := stmts_remainder[0];
match stmt {
var tailAssigned := DetectAssignmentStatus(stmts_remainder[1..], dafny_name);
var stop := stmt.Return? || stmt.EarlyReturn? || stmt.JumpTailCallStart? || (stmt.DeclareVar? && stmt.name == dafny_name);
var thisAssign := match stmt {
case Assign(Ident(assign_name), _) =>
if assign_name == dafny_name then SurelyAssigned else
DetectAssignmentStatus(stmts_remainder[1..], dafny_name)
if assign_name == dafny_name then SurelyAssigned else NotAssigned
case If(cond, thn, els) =>
DetectAssignmentStatus(thn, dafny_name).Join(DetectAssignmentStatus(els, dafny_name))
DetectAssignmentStatus(thn, dafny_name)
.Join(DetectAssignmentStatus(els, dafny_name))
case Call(on, callName, typeArgs, args, outs) =>
if outs.Some? && dafny_name in outs.value then
SurelyAssigned
else
DetectAssignmentStatus(stmts_remainder[1..], dafny_name)
if outs.Some? && dafny_name in outs.value then SurelyAssigned else NotAssigned
case Labeled(_, stmts) =>
DetectAssignmentStatus(stmts, dafny_name).Then(DetectAssignmentStatus(stmts_remainder[1..], dafny_name))
DetectAssignmentStatus(stmts, dafny_name)
case DeclareVar(name, _, _) =>
if name == dafny_name then
NotAssigned // Shadowed
else
DetectAssignmentStatus(stmts_remainder[1..], dafny_name)
NotAssigned // If it's the same name, it's shadowed
case Return(_) | EarlyReturn() | JumpTailCallStart() => NotAssigned
case Print(_) => DetectAssignmentStatus(stmts_remainder[1..], dafny_name)
case Print(_) => NotAssigned
case _ => Unknown
};
if stop then thisAssign else thisAssign.Then(tailAssigned)
} by method {
for i := 0 to |stmts_remainder|
invariant DetectAssignmentStatus(stmts_remainder, dafny_name) == DetectAssignmentStatus(stmts_remainder[i..], dafny_name)
{
assert stmts_remainder[i..][0] == stmts_remainder[i];
var stmt := stmts_remainder[i];
match stmt {
case Assign(Ident(assign_name), _) =>
if assign_name == dafny_name {
return SurelyAssigned;
}
case If(cond, thn, els) =>
var rec := DetectAssignmentStatus(thn, dafny_name);
if rec == Unknown { return Unknown; }
var rec2 := DetectAssignmentStatus(els, dafny_name);
if rec2 == Unknown { return Unknown; }
if rec != rec2 { return Unknown; }
if rec.SurelyAssigned? { return SurelyAssigned; }
case Call(on, callName, typeArgs, args, outs) =>
if outs.Some? && dafny_name in outs.value { return SurelyAssigned; }
case Labeled(_, stmts) =>
var rec := DetectAssignmentStatus(stmts, dafny_name);
if !rec.NotAssigned? { return rec; }
case DeclareVar(name, _, _) =>
if name == dafny_name { return NotAssigned; /* Shadowed */ }
case Return(_) | EarlyReturn() | JumpTailCallStart() =>
return NotAssigned;
case Print(_) =>
case _ =>
return Unknown;
}
assert DetectAssignmentStatus(stmts_remainder[i..][1..], dafny_name) == DetectAssignmentStatus(stmts_remainder[i+1..], dafny_name);
}
return NotAssigned;
}
}
117 changes: 71 additions & 46 deletions Source/DafnyCore/Backends/Rust/Dafny-compiler-rust.dfy
Original file line number Diff line number Diff line change
Expand Up @@ -1736,43 +1736,80 @@ module {:extern "DCOMP"} DafnyToRustCompiler {
readIdents := {};
var i := 0;
newEnv := env;
ghost var oldStmts := stmts;
var stmts := stmts; // Make it mutable
while i < |stmts| {
var stmt := stmts[i];
// Avoid maybe placebo wrapping if not necessary
match stmt {
case DeclareVar(name, optType, None) =>
var laterAssignmentStatus := DetectAssignmentStatus(stmts[i + 1..], name);
newEnv := newEnv.AddAssignmentStatus(escapeVar(name), laterAssignmentStatus);
case DeclareVar(name, optType, Some(InitializationValue(typ))) =>
var tpe := GenType(typ, GenTypeContext.default());
var varName := escapeVar(name);
var laterAssignmentStatus := DetectAssignmentStatus(stmts[i + 1..], name);
newEnv := newEnv.AddAssignmentStatus(varName, laterAssignmentStatus);
case _ =>

}
assume {:axiom} stmt in oldStmts;
var stmtExpr, recIdents, newEnv2 := GenStmt(stmt, selfIdent, newEnv, isLast && (i == |stmts| - 1), earlyReturn);
newEnv := newEnv2;
// Special case "var x: T; x = ...". We need to merge them
// because normally x can read a variable named x, and with two statements
// this is not possible.
if stmt.DeclareVar? && stmt.maybeValue.None? &&
i + 1 < |stmts| &&
stmts[i + 1].Assign? &&
stmts[i + 1].lhs.Ident? &&
stmts[i + 1].lhs.ident == stmt.name {
var name := stmt.name;
var typ := stmt.typ;
var stmtExpr, recIdents, newEnv2 := GenDeclareVarAssign(name, typ, stmts[i + 1].value, selfIdent, newEnv);
newEnv := newEnv2;
readIdents := readIdents + (recIdents - declarations);
declarations := declarations + {escapeVar(name)};
generated := generated.Then(stmtExpr);
i := i + 2;
} else {
// Avoid maybe placebo wrapping if not necessary
match stmt {
case DeclareVar(name, optType, None) =>
var laterAssignmentStatus := DetectAssignmentStatus(stmts[i + 1..], name);
newEnv := newEnv.AddAssignmentStatus(escapeVar(name), laterAssignmentStatus);
case DeclareVar(name, optType, Some(InitializationValue(typ))) =>
var tpe := GenType(typ, GenTypeContext.default());
var varName := escapeVar(name);
var laterAssignmentStatus := DetectAssignmentStatus(stmts[i + 1..], name);
newEnv := newEnv.AddAssignmentStatus(varName, laterAssignmentStatus);
case _ =>
}
var stmtExpr, recIdents, newEnv2 := GenStmt(stmt, selfIdent, newEnv, isLast && (i == |stmts| - 1), earlyReturn);
newEnv := newEnv2;

match stmt {
case DeclareVar(name, _, _) => {
declarations := declarations + {escapeVar(name)};
match stmt {
case DeclareVar(name, _, _) => {
declarations := declarations + {escapeVar(name)};
}
case _ => {}
}
readIdents := readIdents + (recIdents - declarations);
generated := generated.Then(stmtExpr);
if stmtExpr.Return? { // The rest of statements is unreachable
break;
}
case _ => {}
}
readIdents := readIdents + (recIdents - declarations);
generated := generated.Then(stmtExpr);

i := i + 1;
if stmtExpr.Return? { // The rest of statements is unreachable
break;
i := i + 1;
}
}
}

method GenDeclareVarAssign(name: VarName, typ: Type, rhs: Expression, selfIdent: SelfInfo, env: Environment)
returns (generated: R.Expr, readIdents: set<string>, newEnv: Environment)
decreases rhs
modifies this
{
var tpe := GenType(typ, GenTypeContext.default());
var varName := escapeVar(name);
var exprRhs;
if rhs.InitializationValue? &&
tpe.IsObjectOrPointer() {
readIdents := {};
tpe := if rhs.NewUninitArray? then tpe.TypeAtInitialization() else tpe;
exprRhs := tpe.ToNullExpr();
} else {
var expr, exprOwnership;
expr, exprOwnership, readIdents := GenExpr(rhs, selfIdent, env, OwnershipOwned);
tpe := if rhs.NewUninitArray? then tpe.TypeAtInitialization() else tpe;
exprRhs := expr;
}
generated := R.DeclareVar(R.MUT, varName, Some(tpe), Some(exprRhs));
newEnv := env.AddAssigned(varName, tpe);
}

method GenAssignLhs(lhs: AssignLhs, rhs: R.Expr, selfIdent: SelfInfo, env: Environment) returns (generated: R.Expr, needsIIFE: bool, readIdents: set<string>, newEnv: Environment)
decreases lhs, 1
modifies this
Expand Down Expand Up @@ -1962,30 +1999,18 @@ module {:extern "DCOMP"} DafnyToRustCompiler {
var varName := escapeVar(name);
var hasCopySemantics := tpe.CanReadWithoutClone();
if expression.InitializationValue? && !hasCopySemantics {
// We might be able to just let the variable uninitialized.
if env.IsAssignmentStatusKnown(varName) {
var tpe := GenType(typ, GenTypeContext.default());
generated := R.DeclareVar(R.MUT, varName, Some(tpe), None);
readIdents := {};
newEnv := env.AddAssigned(varName, tpe);
} else {
generated := R.DeclareVar(R.MUT, varName, None, Some(R.MaybePlaceboPath.AsExpr().ApplyType1(tpe).FSel("new").Apply0()));
readIdents := {};
newEnv := env.AddAssigned(varName, R.MaybePlaceboType(tpe));
tpe := R.MaybePlaceboType(tpe);
}
} else {
var expr, recIdents;
if expression.InitializationValue? &&
tpe.IsObjectOrPointer() {
expr := tpe.ToNullExpr();
recIdents := {};
} else {
var exprOwnership;
expr, exprOwnership, recIdents := GenExpr(expression, selfIdent, env, OwnershipOwned);
}
readIdents := recIdents;
tpe := if expression.NewUninitArray? then tpe.TypeAtInitialization() else tpe;
generated := R.DeclareVar(R.MUT, varName, Some(tpe), Some(expr));
readIdents := {};
newEnv := env.AddAssigned(varName, tpe);
} else {
generated, readIdents, newEnv := GenDeclareVarAssign(name, typ, expression, selfIdent, env);
return;
}
}
case DeclareVar(name, typ, None) => {
Expand Down
Loading

0 comments on commit 5666571

Please sign in to comment.