Skip to content
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

WIP: Enumerator<T> trait #37

Draft
wants to merge 72 commits into
base: master
Choose a base branch
from

Conversation

robin-aws
Copy link
Member

This is an early draft of a design for an Enumerable<T> trait, which I developed in the process of writing up an RFC for adding some kind of enumeration support to Dafny: dafny-lang/dafny#1753

I do NOT expect this to be approved even once it is more polished, since it has the same issue with needing the {:termination false} attribute that my other PR (#22) is blocked by. I will likely tackle at least a partial solution to that issue (dafny-lang/dafny#1588) before promoting this PR out of draft. I nevertheless wanted to create this PR for early feedback and as evidence that all the common enumeration use cases therein do in fact verify, and therefore to support the arguments in the RFC I'll be posting to dafny-lang/rfcs shortly.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

examples/Enumerators/Datatypes.dfy Outdated Show resolved Hide resolved
examples/Enumerators/Enumerators.dfy Outdated Show resolved Hide resolved
examples/Enumerators/IteratorAdaptor.dfy Outdated Show resolved Hide resolved
examples/Enumerators/IteratorAdaptor.dfy Show resolved Hide resolved
examples/Enumerators/IteratorAdaptor.dfy Outdated Show resolved Hide resolved
reads Repr
requires Valid()
decreases Repr, 2
ensures HasNext() ==> Decreases() > 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For an enumerator that correctly implements Next(), L58 will have to hold of HasNext(). However, I don't see that any client of HasNext() needs to know this, so I suggest dropping L58.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this is redundant since it follows from the decreasing post-condition of Next() itself. We need that "extraneous specification" linter :)

src/Enumerators/Enumerators.dfy Outdated Show resolved Hide resolved
Comment on lines 48 to 49
// Next() is invoked. This avoids forcing the type parameter for any
// enumerator to satisfy the Auto-initializable type characteristic (0),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything in this trait that would require T to be an auto-init type. There is such a requirement on the yields parameters of an iterator, but I don't see that that also has to apply here.

It is quite possible that "knowing ahead of time if [an enumerator] is able to produce a value" requires the enumerator's constructor and Next() method to have to go compute the next value. Once that is computed, it needs to be stored in some field (call it next), so that the subsequent Next() has the precomputed value to return. If the pre-computation determines that there is no next value, then next still has to have a value. This is where it may look like an auto-init type is helpful. However, this problem is better solved by declaring next to have type Option<T>, just as class FilteredEnumerator is doing below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are exactly the details I had in my head when writing that. I'll either expand this or not bother mentioning it at this point.

Comment on lines 45 to 47
// Pre-condition for Next(). Making this a pure predicate means that
// enumerators have to at least know ahead of time if they are able to
// produce a value, even if they do not know what value it is until
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this is the best model for enumerators. Always having to compute a value ahead of requests makes the code more complicated. And it may also be confusing, because any side effects of computing a value take place when the previous value is in the process of being returned.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree on those downsides. My strongest reason for landing on this version so far is that it was FAR easier to make all this code verify. Now that I've reached this baseline, though, I'll make an alternate version of all of this where Next() returns an Option<T> instead and see how it compares on both the implementation and consuming sides.

Comment on lines 124 to 125
// TODO: The examples above work because Dafny is aware of the concrete
// types of the various enumerator values, and hence knows the additional post-conditions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this needed? For all new allocations in this Demo module, I gave the trait as an explicit type of the receiving variable and everything still verifies. For example, I changed the declarations of e1, e2, and e in Example2() to

    var e1: Enumerator := new SeqEnumerator(first);
    var e2: Enumerator := new SeqEnumerator(second);
    var e: Enumerator := new ConcatEnumerator(e1, e2);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example2 is just fine with that because it doesn't assert anything about the behavior of the enumerators. But Concatenate has a post-condition that isn't verified if I remove the explicit type declaration on L119. That's because it needs the extra post-condition implied by !ConcatEnumerator.HasNext(), so Dafny doesn't have enough information if I don't guide it to recognize the concrete type of that enumerator. I'm trying to figure out if there's a better solution there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah don't spend any further thought on this, I have a solution that seems to work nicely. Will push something tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants