-
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
Resolve match before compile #2734
Resolve match before compile #2734
Conversation
This is great! This will make many things better. For example, it will help my new type inference and will aid in producing good linter warnings about the user's given syntax. |
…r/dafny into resolveMatchBeforeCompile
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.
Just a few cleanup changes and questions. Otherwise I think this looks good, and makes the treatment of nested matches easier to understand.
|
||
namespace Microsoft.Dafny; | ||
|
||
class ResolverBottomUpVisitor : BottomUpVisitor { |
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 looks like this class still also exists inside Resolver.cs
. Was that intentional?
public override IEnumerable<Statement> SubStatements => | ||
ResolvedStatement == null ? Cases.SelectMany(c => c.Body) : base.SubStatements; | ||
public override IEnumerable<Statement> SubStatements { | ||
get { |
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.
Why make this use a get
block instead of =>
like before?
} | ||
} | ||
|
||
public abstract record LinkedList<T> : IEnumerable<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.
Why can't you use System.Collections.Generic.LinkedList
?
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 a doubly linked list and this is a single one, which allows easily grabbing just the tail. I will rename this one to make it more clear.
@@ -8558,6 +7362,14 @@ public CollectFriendlyCallsInSpec_Visitor(Resolver resolver, ISet<Expression> fr | |||
} | |||
} | |||
|
|||
abstract class ResolverTopDownVisitor<T> : TopDownVisitor<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.
This class appears to be duplicated.
/// it gets "replaced" by the statement in "ResolvedStatement". | ||
/// Adapted from ConcreteSyntaxStatement | ||
/// </summary> | ||
public abstract class ConcreteSyntaxStatement : Statement { |
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 happy to see this go away!
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 but NestedMatchStmt.Flattened
came in its place so I'm not sure if it's an improvement 😅 I considered changing ConcreteSyntaxStatement
and ConcreteSyntaxExpression
so they would have not only a resolvedX
but also a compiledX
field, instead of the current Flattened
property. That would reduce the code added to Translator.cs
and SinglePassCompiler.cs
, and add opportunities for other post resolution compilation passes.
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 point. But one advantage to NestedMatchStmt.Flattened
is that it's clear that it's used for one thing, whereas ConcreteSyntaxStatement
sounds like it could cover many instances of concrete syntax, even if it didn't, in practice.
* 4 - An IdPattern at datatype type with no arguments representing a bound variable | ||
*/ | ||
public void CheckLinearExtendedPattern(Type type, ResolutionContext resolutionContext, Resolver resolver) { | ||
if (type == null) { |
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.
Under what circumstances might this happen?
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 doesn't seem possible to me. Note this is an existing check, this method was moved from Resolver.cs
. In a follow-up PR, I will merge CheckLinearExtendedPattern
and ExtendedPattern.Resolve
and maybe this can go then.
|
||
// Get the datatype of the matchee | ||
var currMatcheeType = currMatchee.Type | ||
// resolver.PartiallyResolveTypeForMemberSelection(currMatchee.tok, currMatchee.Type) |
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.
Perhaps remove this commented-out code?
.NormalizeExpand(); | ||
// if (currMatcheeType is TypeProxy) { | ||
// resolver.PartiallySolveTypeConstraints(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.
Remove this, as well?
if (dtd == null) { | ||
ctors = null; | ||
} else { | ||
ctors = dtd.Ctors.ToDictionary(c => c.Name, c => c); //resolver.datatypeCtors[dtd]; |
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.
Obsolete comment?
} | ||
} | ||
|
||
// TODO use a record. |
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.
Is there any reason not to do this right away?
…r/dafny into resolveMatchBeforeCompile
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.
Looks great!
Fixes #3333 The actual fix was already accidentally made in #2734, this PR is just adding a test case and a release note, and removing some obsolete code. <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>
During PR dafny-lang#2734, a refactoring of the cloner mistakenly omitted a clone call for one sub-expression of TypeRHS, leading to issue dafny-lang#3343. This PR reinstates that clone. Fixes dafny-lang#3343.
During PR dafny-lang#2734, a refactoring of the cloner mistakenly omitted a clone call for one sub-expression of TypeRHS, leading to issue dafny-lang#3343. This PR reinstates that clone. Fixes dafny-lang#3343.
During PR dafny-lang#2734, a refactoring of the cloner mistakenly omitted a clone call for one sub-expression of TypeRHS, leading to issue dafny-lang#3343. This PR reinstates that clone. Fixes dafny-lang#3343.
Changes
A nested match is one where a single case may match on multiple destructors, organised in a tree. A nested match case may also match on a constant value. A simple match case may only match on a single constructor. The process of compiling/denesting/flattening nested matches, transforms nested matches into
if statements
and simple matches.Previously the flattening was done during resolution, but this PR moves it further back in the pipeline so the resolved AST does not have the flattening applied to it. The flattening is still computed during resolution, but it is stored in a new field instead of being applied to the resolved AST. That way, the IDE can derive code navigation from the unflattened AST, but still emit warnings produced by the flattening. Using the flattened results is done in verification and compilation by traversing the new field.
I think more idiomatically would be not to have the newly added field
flattened
insideNestedMatchExpr/Stmt
, and instead fully replace theNestedMatchExpr/Stmt
node, since that would prevent having to add code in the backends. However, we don't have a good mechanism for replacing AST nodes yet. Also, doing that would require being able to set up a transformation that runs after resolution, so it doesn't affect the IDE's code navigation, but before verification and code generation, so both of those can benefit from it, and we don't have a good mechanism for that either. I also wonder whether that would require cloning the AST right after resolution so it's preserved for code navigation. However, I think such mechanisms could benefit code generators that produce warnings after doing other transformations, so it's not a ton of work just for nested matches.Code related to the flattening of nested matches has been moved out of
Resolver.cs
and into a new fileMatchFlattener.cs
. Code related to resolving nested matches has been moved out of resolver and into the related AST nodes. Simple matches no longer have resolution code since they're instantiated already resolved. Resolution of nested matches is currently done in two phases, the 'Check' and 'Resolve' phase, that can be merged, but I'll leave that for a follow-up PR.The was a small transformation that was done during match flattening, which transforms explicit type annotations on bound case variables, by replacing them with a let statement around the body of the case, where the bound let variable carries the explicit type annotation. This transformation has been moved to the resolution phase of nested matches. Possibly we don't need this transformation, but I leave removing or moving it for a follow-up PR.
Fix a bug related to matches and ghost constructors. In the following program, D0 is non-ghost, but previously the match was still considered to be ghost because XY has constructors other than D0 that are ghost.
Testing
DefinitionTests.DatatypesAndMatches
have been uncommented, to test the additionally supported behavior.By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.