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

fix: Add missing type characteristics #154

Merged
merged 11 commits into from
Jan 26, 2024

Conversation

RustanLeino
Copy link
Collaborator

@RustanLeino RustanLeino commented Jan 3, 2024

This PR primarily adds missing (!new) type characteristics in function declarations. These had gone undetected, because of a latent bug that has now been fixed. The bug fix requires these fixes in the libraries.

This PR also cleans up some library files: removing deprecated semi-colons, removing unnecessary parentheses, stylizing order of specification clauses, and removing explicit triggers that are saying the same thing as those triggers that are automatically inferred.

The PR also changes forall ensures ... statements with no bound variables into assert ... by statements. (I think we attempted this change once before, only to find that the change tickled some brittleness issue. Let's see if they verify this time.)

Finally, the PR improves some reads fn clauses, where fn is some function. In one case, such a reads clause was probably accidentally (and I changed it to the more specific reads fn(this)). In the other cases, the reads fn clause had ended up reading more than the enclosing function's precondition could support. The new reads clauses are more specific than the previous, so, semantically, they should not affect callers. I'm surprised how these were not detected before (am I perhaps running the wrong version of Dafny when running the libraries test suite?). In fact, I also received an error, which I think is due to that a previous version of Dafny didn't assume requires clauses when checking reads clauses in lambda expressions (or was it perhaps the other way around?). If there's still a problem with my modifications, I hope CI will discover them.

Hint to reviewers: If you want to see each of the simple modifications, I suggest looking at each commit individually.

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

@RustanLeino RustanLeino marked this pull request as ready for review January 3, 2024 22:35
@@ -653,7 +653,7 @@ module {:options "-functionSyntax:4"} Seq {
/* Applying a function to a sequence is distributive over concatenation. That is, concatenating
two sequences and then applying Map is the same as applying Map to each sequence separately,
and then concatenating the two resulting sequences. */
lemma {:opaque} LemmaMapDistributesOverConcat<T,R>(f: (T ~> R), xs: seq<T>, ys: seq<T>)
lemma {:opaque} LemmaMapDistributesOverConcat<T, R>(f: (T ~> R), xs: seq<T>, ys: seq<T>)
Copy link
Member

Choose a reason for hiding this comment

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

An opaque lemma? :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, that makes no sense. I removed them. I also removed them in dafny-lang/dafny#4928.

@@ -15,7 +15,7 @@ module Dafny.Collections.Arrays {
import opened Relations
import opened Seq

method BinarySearch<T>(a: array<T>, key: T, less: (T, T) -> bool) returns (r: Option<nat>)
method BinarySearch<T(!new)>(a: array<T>, key: T, less: (T, T) -> bool) returns (r: Option<nat>)
Copy link
Member

Choose a reason for hiding this comment

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

I'm very surprised by this type characteristic. I would have thought it would have been possible to sort an array of arrays by the size of each array for example. Why is this required now? Can we relax it if possible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's because of how the specification was written. The precondition uses StrictTotalOrdering, which ends up quantifying over all values of the type. If those ordering predicates were instead written to apply only to an element of given set or sequence, then the (!new) would not be needed.

Copy link
Member

Choose a reason for hiding this comment

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

Bumping on this one.

Copy link
Member

Choose a reason for hiding this comment

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

When I wrote my last message, I hadn't seen yours on the thread. I should have refreshed the page first.

@RustanLeino
Copy link
Collaborator Author

Note, this libraries repository was recently deprecated (#155). Still, I think it makes sense to roll in this PR, since it contains valid changes and is otherwise ready to go.

Copy link
Member

@atomb atomb left a comment

Choose a reason for hiding this comment

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

I was just looking at making similar changes, and opened #156, but this PR already does more. I'd like to see it merged, since the libraries repo is still being used, even if it's deprecated.

@atomb atomb merged commit e9b898d into dafny-lang:master Jan 26, 2024
7 checks passed
@RustanLeino RustanLeino deleted the add-missing-type-characteristics branch February 20, 2024 18:02
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.

4 participants