-
Notifications
You must be signed in to change notification settings - Fork 261
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
feat: Add support for disjunctive (“or”) patterns #2448
Changes from 2 commits
de0ab09
17a5a68
2ef4189
53808c6
afb8718
ff2869c
8973659
d326179
46b34e3
aa8d2b1
d6f9242
1c89a9d
cd7d728
a0c4848
b04bdf1
48161bd
5dbd839
b39ac54
85bdb90
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 |
---|---|---|
|
@@ -44,20 +44,11 @@ private bool VisibleInScope(Declaration d) { | |
return useCompileSignatures || d.IsVisibleInScope(moduleInfo.VisibilityScope); | ||
} | ||
|
||
FreshIdGenerator defaultTempVarIdGenerator; | ||
FreshIdGenerator defaultTempVarIdGenerator = new FreshIdGenerator(); | ||
|
||
string FreshTempVarName(string prefix, ICodeContext context) { | ||
var decl = context as Declaration; | ||
if (decl != null) { | ||
return decl.IdGenerator.FreshId(prefix); | ||
} | ||
|
||
// TODO(wuestholz): Is the following code ever needed? | ||
if (defaultTempVarIdGenerator == null) { | ||
defaultTempVarIdGenerator = new FreshIdGenerator(); | ||
} | ||
|
||
return defaultTempVarIdGenerator.FreshId(prefix); | ||
var gen = context is Declaration decl ? decl.IdGenerator : defaultTempVarIdGenerator; | ||
return gen.FreshId(prefix); | ||
} | ||
|
||
interface IAmbiguousThing<Thing> { | ||
|
@@ -11947,6 +11938,7 @@ private abstract class RBranch { | |
public List<ExtendedPattern> Patterns; | ||
|
||
public RBranch(IToken tok, int branchid, List<ExtendedPattern> patterns) { | ||
Contract.Requires(patterns.All(p => !(p is DisjunctivePattern))); | ||
this.Tok = tok; | ||
this.BranchID = branchid; | ||
this.Patterns = patterns; | ||
|
@@ -11964,6 +11956,7 @@ public RBranchStmt(IToken tok, int branchid, List<ExtendedPattern> patterns, Lis | |
} | ||
|
||
public RBranchStmt(int branchid, NestedMatchCaseStmt x, Attributes attrs = null) : base(x.Tok, branchid, new List<ExtendedPattern>()) { | ||
Contract.Requires(!(x.Pat is DisjunctivePattern)); // No nested or patterns | ||
this.Body = new List<Statement>(x.Body); // Resolving the body will insert new elements. | ||
this.Attributes = attrs; | ||
this.Patterns.Add(x.Pat); | ||
|
@@ -12190,7 +12183,7 @@ private void PrintRBranches(MatchingContext context, List<Expression> matchees, | |
* PairPB contains, for each branches, its head pattern and the rest of the branch. | ||
*/ | ||
private SyntaxContainer CompileRBranchConstant(MatchTempInfo mti, MatchingContext context, Expression currMatchee, List<Expression> matchees, List<Tuple<ExtendedPattern, RBranch>> pairPB) { | ||
// Decreate the count for each branch (increases back for each occurence later on) | ||
// Decrease the count for each branch (increases back for each occurence later on) | ||
foreach (var PB in pairPB) { | ||
mti.UpdateBranchID(PB.Item2.BranchID, -1); | ||
} | ||
|
@@ -12501,6 +12494,48 @@ private SyntaxContainer CompileRBranch(MatchTempInfo mti, MatchingContext contex | |
} | ||
} | ||
|
||
private ExtendedPattern RemoveDisjunctivePatterns(ExtendedPattern pat, bool allowVariables) { | ||
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. The name only captures half of the behavior, so maybe replace 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. Good point |
||
switch (pat) { | ||
case LitPattern: | ||
return pat; | ||
case IdPattern p: | ||
if (p.Arguments == null && !p.Id.StartsWith("_") && !allowVariables) { | ||
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 is the check 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. Wildcards ( 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. From the documentation:
Shouldn't it be Also, shouldn't there be a function Also, is wildcard a good name? Hole seems more fitting. 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.
No: internally, "_" is treated as a variable and gets renamed into a unique variable name starting with an underscore, so by the time we get here the holes are named something like A future refactoring should probably just add a
OK, I will do that. Even better, I can make it a property of the identifier pattern. 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. Done |
||
reporter.Error(MessageSource.Resolver, pat.Tok, "Or-patterns may not bind variables"); | ||
return new IdPattern(p.Tok, FreshTempVarName("_", null), null); | ||
} | ||
return new IdPattern(p.Tok, p.Id, p.Arguments?.ConvertAll(a => RemoveDisjunctivePatterns(a, allowVariables))); | ||
case DisjunctivePattern p: | ||
reporter.Error(MessageSource.Resolver, pat.Tok, "Or-patterns are not allowed inside other patterns"); | ||
return new IdPattern(p.Tok, FreshTempVarName("_", null), null); | ||
default: | ||
Contract.Assert(false); | ||
return null; | ||
} | ||
} | ||
|
||
private IEnumerable<ExtendedPattern> FlattenDisjunctivePatterns(ExtendedPattern pat) { | ||
// TODO: Once we rewrite the pattern-matching compiler, we'll handle disjunctive patterns in it, too. | ||
// For now, we handle top-level disjunctive patterns by duplicating the corresponding cases here, and disjunctive | ||
// sub-patterns are unsupported. | ||
return pat is DisjunctivePattern p | ||
? p.Alternatives.ConvertAll(a => RemoveDisjunctivePatterns(a, allowVariables: false)) | ||
: Enumerable.Repeat(RemoveDisjunctivePatterns(pat, allowVariables: true), 1); | ||
} | ||
|
||
private IEnumerable<NestedMatchCaseExpr> FlattenNestedMatchCaseExpr(NestedMatchCaseExpr c) { | ||
var cloner = new Cloner(); | ||
foreach (var pat in FlattenDisjunctivePatterns(c.Pat)) { | ||
yield return new NestedMatchCaseExpr(c.Tok, pat, cloner.CloneExpr(c.Body), cloner.CloneAttributes(c.Attributes)); | ||
} | ||
} | ||
|
||
private IEnumerable<NestedMatchCaseStmt> FlattenNestedMatchCaseStmt(NestedMatchCaseStmt c) { | ||
var cloner = new Cloner(); | ||
foreach (var pat in FlattenDisjunctivePatterns(c.Pat)) { | ||
yield return new NestedMatchCaseStmt(c.Tok, pat, c.Body.ConvertAll(cloner.CloneStmt), cloner.CloneAttributes(c.Attributes)); | ||
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. Why do you need to clone here instead of use the old body and attributes? Will you be using the old cases? 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. Yes, the old body and attributes are used in the linearity checker. 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. But still, what happens if you don't clone them? 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. Most likely Dafny will crash. My answer above wasn't complete: notice how I don't clone
to
If we don't clone the body we'll have multiple places in the AST sharing the same reference to the body (and typechecking it multiple times, among other things) 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. Makes sense, thanks! 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. It made sense, but I was wrong :/ The crash I had observed was not due to not cloning, and in fact not cloning is wrong here, because of #2334 (the key point being that the branches are already partly resolved, and hence cloning them isn't safe). Thanks for flagging this. |
||
} | ||
} | ||
|
||
private void CompileNestedMatchExpr(NestedMatchExpr e, ResolveOpts opts) { | ||
if (e.ResolvedExpression != null) { | ||
//post-resolve, skip | ||
|
@@ -12510,12 +12545,13 @@ private void CompileNestedMatchExpr(NestedMatchExpr e, ResolveOpts opts) { | |
Console.WriteLine("DEBUG: CompileNestedMatchExpr for match at line {0}", e.tok.line); | ||
} | ||
|
||
MatchTempInfo mti = new MatchTempInfo(e.tok, e.Cases.Count, opts.codeContext, DafnyOptions.O.MatchCompilerDebug); | ||
var cases = e.Cases.SelectMany(FlattenNestedMatchCaseExpr).ToList(); | ||
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 is the right time for flattening the disjunctive patterns? Can't that be done before resolution as a separate rewriter? 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. Possibly, but I'm not sure it's what we want: I wrote it this way to give other code access to the disjunctive patterns. I think of this flattening as an implementation detail of the pattern compiler. 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.
Didn't we want to use many small lowering transformations when possible, to make each individual transformation simpler? I think it would be a big benefit if we can avoid adding code to Also, in what scenario do you envision someone needs access to disjunctive patterns? And if they need it, why couldn't they sit before the lowering transformation? 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.
Indeed, and we have that here, right? There's just a handful of calls in the resolver to new code that pre-processes the pattern.
I can move it to a new file, but all of the other pattern matching compilation code is there, so… ? Originally, the pattern matching compiler was implemented all in the parser. It was moved to the resolver, and in fact it should really be moved to the compiler. In fact, in the long run, disjunctive patterns should be supported at any level of nesting, so the nice separation we have right now won't survive. But for now it's good to have the separated implementation. 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 meant moving the code to a separate file and not calling it from Resolver.cs, except for possibly adding a
I don't know the pattern matching transformations well enough to comment. 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.
Possibly? I think it might work, because this pass doesn't use type information. The problem is that pattern matching compilation happens in the middle of resolution, and it uses some type info. Also, do we have the architecture in place for writing such a pass? I need something that traverses all statements and expressions in a program. Do we have a visitor that does that? 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. Editing to add this following our conversation: it would be nice to add such a visitor, but one issue with rewriting this systematically would be that clients would lose access to the unresolved AST containing the full match structure. |
||
MatchTempInfo mti = new MatchTempInfo(e.tok, cases.Count, opts.codeContext, DafnyOptions.O.MatchCompilerDebug); | ||
|
||
// create Rbranches from MatchCaseExpr and set the branch tokens in mti | ||
List<RBranch> branches = new List<RBranch>(); | ||
for (int id = 0; id < e.Cases.Count; id++) { | ||
var branch = e.Cases.ElementAt(id); | ||
for (int id = 0; id < cases.Count; id++) { | ||
var branch = cases.ElementAt(id); | ||
branches.Add(new RBranchExpr(id, branch, branch.Attributes)); | ||
mti.BranchTok[id] = branch.Tok; | ||
} | ||
|
@@ -12534,8 +12570,8 @@ private void CompileNestedMatchExpr(NestedMatchExpr e, ResolveOpts opts) { | |
if (mti.BranchIDCount[id] <= 0) { | ||
reporter.Warning(MessageSource.Resolver, mti.BranchTok[id], "this branch is redundant "); | ||
scope.PushMarker(); | ||
CheckLinearNestedMatchCase(e.Source.Type, e.Cases.ElementAt(id), opts); | ||
ResolveExpression(e.Cases.ElementAt(id).Body, opts); | ||
CheckLinearNestedMatchCase(e.Source.Type, cases.ElementAt(id), opts); | ||
ResolveExpression(cases.ElementAt(id).Body, opts); | ||
scope.PopMarker(); | ||
} | ||
} | ||
|
@@ -12564,13 +12600,14 @@ private void CompileNestedMatchStmt(NestedMatchStmt s, ResolveOpts opts) { | |
Console.WriteLine("DEBUG: CompileNestedMatchStmt for match at line {0}", s.Tok.line); | ||
} | ||
|
||
var cases = s.Cases.SelectMany(FlattenNestedMatchCaseStmt).ToList(); | ||
// initialize the MatchTempInfo to record position and duplication information about each branch | ||
MatchTempInfo mti = new MatchTempInfo(s.Tok, s.EndTok, s.Cases.Count, codeContext, DafnyOptions.O.MatchCompilerDebug, s.Attributes); | ||
MatchTempInfo mti = new MatchTempInfo(s.Tok, s.EndTok, cases.Count, codeContext, DafnyOptions.O.MatchCompilerDebug, s.Attributes); | ||
|
||
// create Rbranches from NestedMatchCaseStmt and set the branch tokens in mti | ||
List<RBranch> branches = new List<RBranch>(); | ||
for (int id = 0; id < s.Cases.Count; id++) { | ||
var branch = s.Cases.ElementAt(id); | ||
for (int id = 0; id < cases.Count; id++) { | ||
var branch = cases.ElementAt(id); | ||
branches.Add(new RBranchStmt(id, branch, branch.Attributes)); | ||
mti.BranchTok[id] = branch.Tok; | ||
} | ||
|
@@ -12588,8 +12625,8 @@ private void CompileNestedMatchStmt(NestedMatchStmt s, ResolveOpts opts) { | |
if (mti.BranchIDCount[id] <= 0) { | ||
reporter.Warning(MessageSource.Resolver, mti.BranchTok[id], "this branch is redundant"); | ||
scope.PushMarker(); | ||
CheckLinearNestedMatchCase(s.Source.Type, s.Cases.ElementAt(id), opts); | ||
s.Cases.ElementAt(id).Body.ForEach(s => ResolveStatement(s, codeContext)); | ||
CheckLinearNestedMatchCase(s.Source.Type, cases.ElementAt(id), opts); | ||
cases.ElementAt(id).Body.ForEach(s => ResolveStatement(s, codeContext)); | ||
scope.PopMarker(); | ||
} | ||
} | ||
|
@@ -12652,6 +12689,7 @@ private void CheckLinearVarPattern(Type type, IdPattern pat, ResolveOpts opts) { | |
} | ||
|
||
// pat could be | ||
// 0 - A DisjunctivePattern | ||
// 1 - An IdPattern (without argument) at base type | ||
// 2 - A LitPattern at base type | ||
// 3* - An IdPattern at tuple type representing a tuple | ||
|
@@ -12662,7 +12700,17 @@ private void CheckLinearExtendedPattern(Type type, ExtendedPattern pat, ResolveO | |
return; | ||
} | ||
|
||
if (!type.IsDatatype) { // Neither tuple nor datatype | ||
if (pat is DisjunctivePattern dp) { | ||
foreach (var alt in dp.Alternatives) { | ||
// Pushing a scope silences the “duplicate parameter” error in | ||
// `CheckLinearVarPattern`. This is acceptable because disjunctive | ||
// patterns are not allowed to bind variables (the corresponding | ||
// error is raised in `RemoveDisjunctivePatterns`). | ||
scope.PushMarker(); | ||
CheckLinearExtendedPattern(type, alt, opts); | ||
scope.PopMarker(); | ||
} | ||
} else if (!type.IsDatatype) { // Neither tuple nor datatype | ||
if (pat is IdPattern id) { | ||
if (id.Arguments != null) { | ||
// pat is a tuple or constructor | ||
|
@@ -12715,6 +12763,7 @@ private void CheckLinearExtendedPattern(Type type, ExtendedPattern pat, ResolveO | |
return; | ||
} else { // matching a datatype value | ||
if (!(pat is IdPattern)) { | ||
Contract.Assert(pat is LitPattern); | ||
reporter.Error(MessageSource.Resolver, pat.Tok, "Constant pattern used in place of datatype"); | ||
} | ||
IdPattern idpat = (IdPattern)pat; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// RUN: %dafny_0 /compile:0 "%s" > "%t" | ||
// RUN: %diff "%s.expect" "%t" | ||
|
||
module SanityChecks { | ||
datatype T = A(int) | B(nat) | C(bool) | ||
|
||
method Variables(t: T) { | ||
match t | ||
case A(n) // Error: Or-patterns may not bind variables | ||
| B(n) => // Error: Or-patterns may not bind variables | ||
case _ => | ||
} | ||
|
||
method Nesting(t: T) { | ||
match t | ||
case A(1 | 2 | _) => // Error: Or-patterns are not allowed inside other patterns | ||
case B(0 | _) // Error: Or-patterns are not allowed inside other patterns | ||
| C(_ | _ | _) => // Error: Or-patterns are not allowed inside other patterns | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
// RUN: %dafny /compile:3 /print:"%t.print" /rprint:"%t.dprint" "%s" > "%t" | ||
// RUN: %diff "%s.expect" "%t" | ||
|
||
datatype Enum = One | Two | Three { | ||
predicate method Even() { | ||
this.Two? | ||
} | ||
|
||
predicate method Even'() { | ||
match this | ||
case One | Three => false | ||
case Two => true | ||
} | ||
|
||
predicate method Even''() { | ||
match this | ||
case Two => true | ||
case One | Three => false | ||
} | ||
|
||
lemma EvenOk() ensures Even() == Even'() == Even''() {} | ||
} | ||
|
||
module Lists { | ||
datatype List<T> = Nil | Cons(car: T, cdr: List<T>) { | ||
function {:fuel 5} Length(): nat { | ||
match this | ||
case Nil => 0 | ||
case Cons(_, t) => 1 + t.Length() | ||
} | ||
} | ||
|
||
predicate method ContainsOne(l: List<int>) | ||
requires l.Length() == 3 | ||
{ | ||
l.car == 1 || l.cdr.car == 1 || l.cdr.cdr.car == 1 | ||
} | ||
|
||
predicate method ContainsOne'(l: List<int>) | ||
requires l.Length() == 3 | ||
{ | ||
match l | ||
case Cons(1, Cons(_, Cons(_, Nil))) | ||
| Cons(_, Cons(1, Cons(_, Nil))) | ||
| Cons(_, Cons(_, Cons(1, Nil))) => | ||
true | ||
case Cons(_, Cons(_, Cons(_, Nil))) => | ||
false | ||
} | ||
|
||
lemma ContainsOneOK(l: List<int>) | ||
requires l.Length() == 3 | ||
ensures ContainsOne(l) == ContainsOne'(l) | ||
{} | ||
} | ||
|
||
import opened Lists | ||
|
||
method Main() { | ||
expect One.Even() == One.Even'() == One.Even''() == false; | ||
expect Two.Even() == Two.Even'() == Two.Even''() == true; | ||
expect Three.Even() == Three.Even'() == Three.Even''() == false; | ||
|
||
var a0 := Cons(0, Cons(0, Cons(0, Nil))); | ||
expect ContainsOne(a0) == ContainsOne'(a0) == false; | ||
var a1 := Cons(1, Cons(0, Cons(0, Nil))); | ||
expect ContainsOne(a1) == ContainsOne'(a1) == true; | ||
var a2 := Cons(0, Cons(1, Cons(0, Nil))); | ||
expect ContainsOne(a2) == ContainsOne'(a2) == true; | ||
var a3 := Cons(0, Cons(0, Cons(1, Nil))); | ||
expect ContainsOne(a3) == ContainsOne'(a3) == true; | ||
|
||
print "OK\n"; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
|
||
Dafny program verifier finished with 7 verified, 0 errors | ||
Running... | ||
|
||
OK |
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.
Seems like we have a difference in style preference. I always write names out in full, so here it would be
pattern
ordisjunctivePattern
andpatternSeparator
. Maybe we can agree on that we should optimise for reading? Would that change your preference?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.
In general I find the shorter names more readable:
In the snippet you highlighted (where there's a single variable) I don't have strong preferences; I just followed the style of the code right above.
For externally visible parameters (like function arguments or class fields) I prefer long names.
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 think your example is more fair if you also include the legend for the small variable names, and add line breaks:
vs