-
Notifications
You must be signed in to change notification settings - Fork 271
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: Fill in missing type substitutions #2984
Changes from 7 commits
443732f
aef58cf
1c90050
dfa8ad6
1baa9bf
8b69cbf
685a806
7e67cd6
a74bae0
aa41e2f
e6fadca
c8d660d
8344691
cb857c6
b98fba5
c6a91ee
b0d0eaf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,13 +38,19 @@ public virtual Expression Substitute(Expression expr) { | |
return new StaticReceiverExpr(e.tok, ty, e.IsImplicit) { Type = ty }; | ||
} else if (expr is LiteralExpr literalExpr) { | ||
if (literalExpr.Value == null) { | ||
return new LiteralExpr(literalExpr.tok) { Type = Resolver.SubstType(literalExpr.Type, typeMap) }; | ||
var ty = Resolver.SubstType(literalExpr.Type, typeMap); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the effect of this change? I see you're changing the object pointer less often, but does that have an effect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a (possibly misguided) optimization in the } else if (expr is MultiSetFormingExpr) {
var e = (MultiSetFormingExpr)expr;
var se = Substitute(e.E);
if (se != e.E) {
newExpr = new MultiSetFormingExpr(expr.tok, se);
} This optimization is error prone. For The optimization is intended to save memory, but this may not make any difference, and the optimization makes the code significantly more complicated. If you suggest, I'd be happy to remove this optimization from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I built a local version with a simplified There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Without a measurement about the performance difference I'd prefer to leave it like it is, unless you're confident the optimisation doesn't have a significant effect. I'm not sure whether comparing the difference in the test-suite runtime is a good test. I'm hoping we can at some point generate the code for various behaviors such as Substitutor, Cloner, SubExpressions, SubStatements, and likely some others. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I'll leave it as is. And yes, I agree that generating code for these sorts of routines would be far better than the manual upkeep we have to do today. |
||
if (ty != literalExpr.Type) { | ||
return new LiteralExpr(literalExpr.tok) { Type = ty }; | ||
} | ||
} else { | ||
// nothing to substitute | ||
} | ||
} else if (expr is Translator.BoogieWrapper) { | ||
var e = (Translator.BoogieWrapper)expr; | ||
return new Translator.BoogieWrapper(e.Expr, Resolver.SubstType(e.Type, typeMap)); | ||
var ty = Resolver.SubstType(e.Type, typeMap); | ||
if (ty != e.Type) { | ||
return new Translator.BoogieWrapper(e.Expr, ty); | ||
} | ||
} else if (expr is WildcardExpr) { | ||
// nothing to substitute | ||
} else if (expr is ThisExpr) { | ||
|
@@ -74,7 +80,7 @@ public virtual Expression Substitute(Expression expr) { | |
} else if (expr is DisplayExpression) { | ||
DisplayExpression e = (DisplayExpression)expr; | ||
List<Expression> newElements = SubstituteExprList(e.Elements); | ||
if (newElements != e.Elements || !Resolver.SubstType(e.Type, typeMap).Equals(e.Type)) { | ||
if (newElements != e.Elements || Resolver.SubstType(e.Type, typeMap) != e.Type) { | ||
if (expr is SetDisplayExpr) { | ||
newExpr = new SetDisplayExpr(expr.tok, ((SetDisplayExpr)expr).Finite, newElements); | ||
} else if (expr is MultiSetDisplayExpr) { | ||
|
@@ -96,18 +102,25 @@ public virtual Expression Substitute(Expression expr) { | |
} | ||
} | ||
var ty = Resolver.SubstType(e.Type, typeMap); | ||
if (anyChanges || !ty.Equals(e.Type)) { | ||
if (anyChanges || ty != e.Type) { | ||
newExpr = new MapDisplayExpr(expr.tok, e.Finite, elmts); | ||
} | ||
} else if (expr is MemberSelectExpr) { | ||
var mse = (MemberSelectExpr)expr; | ||
Expression substE = Substitute(mse.Obj); | ||
MemberSelectExpr fseNew = new MemberSelectExpr(mse.tok, substE, mse.MemberName); | ||
fseNew.Member = mse.Member; | ||
fseNew.TypeApplication_AtEnclosingClass = mse.TypeApplication_AtEnclosingClass.ConvertAll(t => Resolver.SubstType(t, typeMap)); | ||
fseNew.TypeApplication_JustMember = mse.TypeApplication_JustMember.ConvertAll(t => Resolver.SubstType(t, typeMap)); | ||
fseNew.AtLabel = mse.AtLabel ?? oldHeapLabel; | ||
newExpr = fseNew; | ||
var newObj = Substitute(mse.Obj); | ||
var newTypeApplicationAtEnclosingClass = SubstituteTypeList(mse.TypeApplication_AtEnclosingClass); | ||
var newTypeApplicationJustMember = SubstituteTypeList(mse.TypeApplication_JustMember); | ||
if (newObj != mse.Obj || | ||
newTypeApplicationAtEnclosingClass != mse.TypeApplication_AtEnclosingClass || | ||
newTypeApplicationJustMember != mse.TypeApplication_JustMember) { | ||
var fseNew = new MemberSelectExpr(mse.tok, newObj, mse.MemberName) { | ||
Member = mse.Member, | ||
TypeApplication_AtEnclosingClass = newTypeApplicationAtEnclosingClass, | ||
TypeApplication_JustMember = newTypeApplicationJustMember, | ||
AtLabel = mse.AtLabel ?? oldHeapLabel | ||
}; | ||
newExpr = fseNew; | ||
} | ||
} else if (expr is SeqSelectExpr) { | ||
SeqSelectExpr sse = (SeqSelectExpr)expr; | ||
Expression seq = Substitute(sse.Seq); | ||
|
@@ -159,13 +172,14 @@ public virtual Expression Substitute(Expression expr) { | |
newExpr = new ApplyExpr(e.tok, fn, args, e.CloseParen); | ||
|
||
} else if (expr is DatatypeValue) { | ||
DatatypeValue dtv = (DatatypeValue)expr; | ||
var dtv = (DatatypeValue)expr; | ||
var newArguments = SubstituteExprList(dtv.Bindings.Arguments); // substitute into the expressions, but drop any binding names (since those are no longer needed) | ||
if (newArguments != dtv.Bindings.Arguments) { | ||
DatatypeValue newDtv = new DatatypeValue(dtv.tok, dtv.DatatypeName, dtv.MemberName, newArguments); | ||
newDtv.Ctor = dtv.Ctor; // resolve on the fly (and set newDtv.Type below, at end) | ||
newDtv.InferredTypeArgs = Util.Map(dtv.InferredTypeArgs, tt => Resolver.SubstType(tt, typeMap)); | ||
// ^ Set the correct type arguments to the constructor | ||
var newTypeArgs = SubstituteTypeList(dtv.InferredTypeArgs); | ||
if (newArguments != dtv.Bindings.Arguments || newTypeArgs != dtv.InferredTypeArgs) { | ||
var newDtv = new DatatypeValue(dtv.tok, dtv.DatatypeName, dtv.MemberName, newArguments) { | ||
Ctor = dtv.Ctor, | ||
InferredTypeArgs = newTypeArgs | ||
}; | ||
newExpr = newDtv; | ||
} | ||
|
||
|
@@ -209,29 +223,39 @@ public virtual Expression Substitute(Expression expr) { | |
} else if (expr is BoxingCastExpr) { | ||
var e = (BoxingCastExpr)expr; | ||
var se = Substitute(e.E); | ||
if (se != e.E) { | ||
newExpr = new BoxingCastExpr(se, e.FromType, e.ToType); | ||
var fromType = Resolver.SubstType(e.FromType, typeMap); | ||
var toType = Resolver.SubstType(e.ToType, typeMap); | ||
if (se != e.E || fromType != e.FromType || toType != e.ToType) { | ||
newExpr = new BoxingCastExpr(se, fromType, toType); | ||
} | ||
} else if (expr is UnaryExpr) { | ||
var e = (UnaryExpr)expr; | ||
Expression se = Substitute(e.E); | ||
if (se != e.E) { | ||
var se = Substitute(e.E); | ||
if (e is TypeUnaryExpr typeUnaryExpr) { | ||
var toType = Resolver.SubstType(typeUnaryExpr.ToType, typeMap); | ||
if (se != e.E || toType != typeUnaryExpr.ToType) { | ||
if (e is ConversionExpr) { | ||
var ee = (ConversionExpr)e; | ||
newExpr = new ConversionExpr(expr.tok, se, toType); | ||
} else if (e is TypeTestExpr) { | ||
var ee = (TypeTestExpr)e; | ||
newExpr = new TypeTestExpr(expr.tok, se, toType); | ||
} else { | ||
Contract.Assert(false); // unexpected UnaryExpr subtype | ||
} | ||
} | ||
} else if (se != e.E) { | ||
if (e is FreshExpr) { | ||
var ee = (FreshExpr)e; | ||
newExpr = new FreshExpr(expr.tok, se, ee.At) { AtLabel = ee.AtLabel ?? oldHeapLabel }; | ||
} else if (e is UnaryOpExpr) { | ||
var ee = (UnaryOpExpr)e; | ||
newExpr = new UnaryOpExpr(expr.tok, ee.Op, se); | ||
} else if (e is ConversionExpr) { | ||
var ee = (ConversionExpr)e; | ||
newExpr = new ConversionExpr(expr.tok, se, ee.ToType); | ||
} else if (e is TypeTestExpr) { | ||
var ee = (TypeTestExpr)e; | ||
newExpr = new TypeTestExpr(expr.tok, se, ee.ToType); | ||
} else { | ||
Contract.Assert(false); // unexpected UnaryExpr subtype | ||
} | ||
} | ||
|
||
} else if (expr is BinaryExpr) { | ||
BinaryExpr e = (BinaryExpr)expr; | ||
Expression e0 = Substitute(e.E0); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
// RUN: %dafny_0 /compile:0 "%s" > "%t" | ||
// RUN: %diff "%s.expect" "%t" | ||
|
||
codatatype Stream<X> = Nil | Cons(head: X, tail: Stream<X>) | ||
|
||
least predicate Finite<Y>(s: Stream<Y>) { | ||
// The following once generated malformed Boogie, because of a missing | ||
// type substitution in the datatype value Nil | ||
s == Nil || Finite(s.tail) | ||
} | ||
|
||
least predicate F0<Y>(s: Stream<Y>) { | ||
var nil := Nil; | ||
s == nil || F0(s.tail) | ||
} | ||
|
||
least predicate F1<Y>(s: Stream<Y>) { | ||
s.Nil? || F1(s.tail) | ||
} | ||
|
||
least predicate G0<Y>(s: Stream<Y>) { | ||
s is Stream<Y> | ||
} | ||
|
||
least predicate G1<Y>(s: Stream<Y>) { | ||
s == Identity(s) | ||
} | ||
|
||
least predicate G2<Y>(s: Stream<Y>) { | ||
s == Identity<Stream<Y>>(s) | ||
} | ||
|
||
function Identity<W>(w: W): W { w } | ||
|
||
least lemma About<Z>(s: Stream<Z>) | ||
requires s == Nil | ||
requires s.Nil? | ||
requires var nil := Nil; s == nil | ||
requires s is Stream<Z> | ||
requires s == Identity(s) | ||
requires s == Identity<Stream<Z>>(s) | ||
requires Finite(s) | ||
requires F0(s) | ||
requires F1(s) | ||
requires G0(s) | ||
requires G1(s) | ||
requires G2(s) | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
|
||
Dafny program verifier finished with 3 verified, 0 errors |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix malformed Boogie generated for extreme predicates |
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.
I'd like to have each language construct be as independent as possible. For extreme lemmas and predicates this would also mean they have mostly outwards references. Would it be possible to achieve this?
Would it be possible to translate extremes to their prefix versions and then forget about the originals? Or maybe create the prefix versions next to the originals?
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.
We need both. A program needs the extremes, since that's what you'll ultimately will use for the property you want. The prefix predicates/lemmas give a way to prove properties about the extremes, so they are also needed.
Actually, the more surprising thing (to me) is that
AllCallables
(which excludes the prefix versions) is used so many times.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.
Sure, but can we place both side by side instead of introducing
AllCallablesIncludingPrefixDeclarations
?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.
I don't understand. The
AllCallablesIncludingPrefixDeclarations
andAllCallables
properties are declared side by side. Are you perhaps suggesting that the prefix predicates/lemmas be placed in the list of the enclosing module's top-level-decls (rather than being linked off the corresponding extreme predicate/lemma)? (And if so, do you think that will improve the organization for other parts of the implementation?)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.
Yes that's what I meant.
I think so. If I understand extreme lemma's correctly, they behave similarly to as if you'd written the prefix lemma manually next to them. Also, looking at usages of
ExtremeLemma.PrefixLemma
gives the impression there's a handful of snippets that handle the PrefixLemma as if it was a top level method.Code like this:
or this:
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.
I looked into this in detail for
.PrefixLemma
and.PrefixPredicate
. In places where the code looked like your first example, I changeAllCallables
toAllCallablesIncludingPrefixDeclarations
. I also improved code in the various other visitors, making use of newer C# constructs. And I improved the iteration over declarations inIsEssentiallyEmptyModuleBody()
, to be more exact than before.In the code base, what happens with extreme lemmas/predicates and prefix lemmas/predicates is quite varied. In some places, the extreme declaration is used and the prefix version ignored. In other places, it's the other way around. And in some places, the two are handled in different ways. Therefore, I suggest that any further improvements be done separately from this PR.