-
Notifications
You must be signed in to change notification settings - Fork 266
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: Domains and ranges for quantified variables #2195
feat: Domains and ranges for quantified variables #2195
Conversation
…to new-quantifier-syntax
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.
A few minor things, but the feature introduced in this PR looks great, very well documented and tested, congratulations for the big effort.
// thus needs to capture the value of "s" into another variable, which these tests check on. | ||
var s := [0, 1, 1, 2]; | ||
s := s + [3, 5, 8, 13]; | ||
print forall x <- s :: x < 20, " "; // true |
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'm so happy to see the reduction in the duplication of the variable x! Great job!
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.
Oh, I didn't realize that this would be supported in forall
/exists
! I thought it was just for set
/seq
/multiset
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.
Might as well! They all have the same underlying quantifier domain semantics, so I like the simplicity of aligning the syntax.
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.
Still worth checking a bit more broadly, maybe ^^
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'm afraid I'm not following, checking what exactly? This was the intention in the RFC all along at least.
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.
It was unexpected to me, despite having read the proposals, so I'm guessing it might be unexpected to other people who provided feedback as well; that's it :)
Test/comp/ForallNewSyntax.dfy
Outdated
@@ -0,0 +1,356 @@ | |||
// RUN: %dafny /quantifierSyntax:4 /compile:3 /spillTargetCode:2 /compileTarget:cs "%s" > "%t" |
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.
Could you verify once, and then use /compile:4
here as well?
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.
Good call, this is just what the original test file was doing.
[ <- Expression(allowLemma, allowLambda) ] | ||
{ Attribute } | ||
[ | Expression(allowLemma, allowLambda) ] | ||
```` |
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.
Only three backticks are necessary.
Also, I think we all agreed on that and all the documentation needs to be reformatted accordingly, but could you perhaps start the effort by moving this grammar to the end of this section and replace it with a few examples? That would be less frightening.
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 did have trouble finding the best place for this content. It's shared syntax between multiple features but it's not an Expression or Statement or Type or anything else independent like that. But it's also substantially less "low-level" than the other productions.
Are you suggesting moving it to the end of Section 2, as a 2.7?
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.
Are you suggesting moving it to the end of Section 2, as a 2.7?
No I'm not suggesting changing numbering or the order of sections and sub-sections.
I'm only suggesting to not place the grammar at the beginning of this section, only at the end (until it will be moved to another place). The beginning of the section should consist of introductory text to the features and example covering most use cases.
So, instead of:
### 2.6.5. Quantifier Domains {#sec-quantifier-domains}
<frightening and unreadable grammar definitions>
<introduction text>
<examples>
<more text>
I strongly suggest you write:
### 2.6.5. Quantifier Domains {#sec-quantifier-domains}
<introduction text>
<examples>
<more text>
<frightening and unreadable grammar definitions>
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.
Aha thanks for clarifying. I will gladly start setting a better example. :)
Co-authored-by: Mikaël Mayer <MikaelMayer@users.noreply.github.com>
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.
This is very nice! All of the changes I requested are tiny little things (and one question).
} | ||
(. QuantifiedVar.ExtractSingleRange(qvars, out bvars, out range); .) |
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.
Do I understand correctly that future uses of this new syntax won't call ExtractSingleRange
, but will keep the components separate? That seems like it would be necessary for the expected operational semantics of foreach
, for example (once you get to that!).
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.
That's right, the next PR will add domains and ranges to BoundVar
instead, and ExtractSingleRange
will likely be removed. But this PR will establish a ton of test cases that still have to work after that refactoring. :)
@@ -992,6 +1010,16 @@ is printed. | |||
experimentalPredicateAlwaysGhost - Compiled functions are written `function`. | |||
Ghost functions are written `ghost function`. Predicates are always ghost | |||
and are written `predicate`. | |||
/quantifierSyntax:<version> |
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.
Would be nice if we can reuse Options.txt
here.
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 had that thought too, perhaps in the future we can have a common Options.md that we strip the markdown from when outputting it as part of /help
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.
Love the relatively small scope of this PR :-) makes the review manageable.
Quality of code, documentation and tests looks excellent!
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.
Nice work! I'm looking forward to the point when we can start using these new features. :)
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.
Great job with the documentation, I like it much much better!
A few tweaks to make documentation more friendly.
- `x <- integerSet` | ||
- `x: nat <- integerSet` | ||
- `x: nat <- integerSet | x % 2 == 0` | ||
- `x: nat, y: nat | x < 2 && y < 2` |
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.
- `x: nat, y: nat | x < 2 && y < 2` | |
- `x: nat, y: nat | x < 2 && y < 2` | |
- `x: nat | x < 2, y: nat | y < x` |
It would be good to have a simpler example just before the next one, because the next one uses vertical bars for a different meaning.
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.
Yeah I like this, more of a gradual onramp of complexity :)
docs/DafnyRef/Grammar.md
Outdated
which restricts the bindings to values that satisfy this expression. | ||
In the example above `x <= 5` is the range attached to the `x` variable declaration. | ||
|
||
3. The optional syntax `<- C` attaches a collection expression `C` as a *quantified variable domain*. |
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.
Could you move this one before 2, so that it's the order in which we parse things?
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 was on the edge of doing this already and you have tipped me over :)
Co-authored-by: Mikaël Mayer <MikaelMayer@users.noreply.github.com>
Resolves #2187. First phase of implementing dafny-lang/rfcs#10.
This change adds two new features on several uses of quantified variables in the language:
Quantified variable domains, specified with the syntax
x <- C
, where C is an expression with a collection type (i.e. set, multiset, sequence, or map). Besides offering a more compact syntax for a common pattern (i.e. replacingmyLovelyButLongVariableName | myLovelyButLongVariableName in someSet
withmyLovelyButLongVariableName <- someSet
), this is forwards-compatible with the notion of ordered quantification, which the two features designed in the linked RFC depend on, and potentially others in the future.Quantified variable ranges, specified with the syntax
x | E
, where E is a boolean expression. These replace the existing concept of a single| E
range after all quantified variables (which is now bound to the last declared variable, with equivalent semantics). These are helpful for restricting the values of one quantified variable so that it can be used in the domain expression of another variable.These features apply to the quantifier domains used in
forall
andexists
expressions,set
andmap
comprehensions, andforall
statements, and the parsing for quantifier domains is now shared between these features. As a small consequence, set comprehensions now no longer require range expressions, and will default to| true
.This new syntax is not fully backwards-compatible: the statement
print set x | 0 <= x < 10, y
, for example, was previously parsed asprint (set x | 0 <= x < 10), (y)
but will now be parsed asprint (set x | 0 <= x < 10, y)
and rejected. The/quantifierSyntax
option is used to help migrate this otherwise breaking change:/quantifierSyntax:3
keeps the old behaviour where a| <Range>
always terminates the list of quantified variables./quantifierSyntax:4
instead attempts to parse additional quantified variables.Like
/functionSyntax
, this option will default to4
instead in Dafny 4.0.This is implemented with pure desugaring for now: the
QuantifiedDomain
production parses a list ofQuantifiedVar
objects, but then immediately rewrites a snippet such asx <- C | P(x), y <- D(x) | Q(x, y)
into the equivalent (for now)x | x in C && P(x) && y in D(x) && Q(x, y)
. Token wrappers are used to ensure error messages can still refer to the original syntax. This will not be equivalent once features that depend on the ordering of quantification are added, though, so a subsequent change will add the new fields toBoundVar
instead and plumb these extra components through the whole Dafny pipeline.On testing: I duplicated several existing test files and modified them to use the new syntax features, to get good white-box test coverage even though the implementation is very simple for now.
NOTE: I still have one more commit to add changes to the reference manual and a few more tests (especially of the/quantifierSyntax
option), but a review of the rest is welcome in the meantime!Also resolves #2038 (and the same issue for Java) since I hit it when converting test cases, and it was dead easy to fix.
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.