Skip to content

Conversation

@RustanLeino
Copy link
Collaborator

@RustanLeino RustanLeino commented Jul 30, 2024

This PR prepares for making the new resolver the default (i.e., making --type-system-refresh --general-traits=datatype --general-newtypes the default), but it does not actually change the default.

  • It fixes many bugs in the new resolver and makes other improvements (tighter inference, better error messages, etc.)
  • It changes the test suite to run the new resolver, except in a few dozen test cases that explicitly invoke the legacy resolver. (Among those exceptions, many will be moved to use the new resolver in due time, but for now it seems better to get these bug fixes in rather than delaying this PR.) By default, tests that use %testDafnyForEachResolver and %testDafnyForEachCompiler use both the old and new resolver.
  • It changes the building of the Dafny runtime, Dafny standard library, AutoExtern, and documentation to use the new resolver.

This PR does not change the default resolver. So, users of the old resolver are mostly unaffected, and users of the new resolver obtain the PR's bug fixes.

Commnents for reviewers

  • The behavior of the resolver requires changes in some Dafny code, which affects the test suite:

    • In the new type system, a trait is not a reference type unless it's declared with extends object.
    • The new resolver infers all bound variables to be of base types (like int), never of a subset type (like nat).
    • In some places, specific type parameters have to be given explicitly (usually because the type needs to be a subset type--maybe these can be improved in the future).
    • The old type system would automatically downcast (from a trait to a class). The new resolver requires an explicit cast for this (as does C# and Java, for example).
  • Sometimes, the tests show a different number of things verified. That's because the new or old type system takes care of all the proof obligations.

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

@RustanLeino RustanLeino changed the title Refresh resolver by default Use refresh resolver by default Jul 30, 2024
@RustanLeino RustanLeino marked this pull request as ready for review March 9, 2025 20:37
@RustanLeino RustanLeino changed the title Use refresh resolver by default Prepare for making new resolver the default Mar 9, 2025
}

public static IEnumerable<MemberDecl> AllMembers(IEnumerable<TopLevelDecl> declarations) {
foreach (var decl in declarations.OfType<TopLevelDeclWithMembers>()) {
Copy link
Member

Choose a reason for hiding this comment

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

better to use declarations.OfType<TopLevelDeclWithMembers>().SelectMany(d => d.Members)

public static readonly Option<bool> TypeSystemRefresh = new("--type-system-refresh", () => false,
@"
false - The type-inference engine and supported types are those of Dafny 4.0.
false (default) - The type-inference engine and supported types are those of Dafny 4.0.
Copy link
Member

Choose a reason for hiding this comment

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

The generated documentation already indicates the default value, so no need to specify it explicitly. Example:
image

Constraints.AddConfirmation(tok, () => CheckComparableTypes(a, b, allowBaseTypeCast), errorMessage);
}

private void AddComparableTypesDefault(PreType a, PreType b) {
Copy link
Member

@keyboardDrummer keyboardDrummer Mar 10, 2025

Choose a reason for hiding this comment

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

Would be good to have an example of what Dafny code this is could help with

}
case LiteralExpr:
CheckResultToBeInType(expr.Origin, expr, expr.Type, locals, builder, etran);
if (expr is StringLiteralExpr stringLiteralExpr) {
Copy link
Member

Choose a reason for hiding this comment

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

What's this about?


function Object(js: Grammar.jobject): DeserializationResult<seq<(string, Values.JSON)>> {
Seq.MapWithResult(d requires d in js.data => KeyValue(d.t), js.data)
var f := d requires d in js.data => KeyValue(d.t);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

@keyboardDrummer
Copy link
Member

keyboardDrummer commented Mar 10, 2025

The new resolver infers all bound variables to be of base types (like int), never of a subset type (like nat).

From a user's perspective, this is a regression of the type system, right? I see quite a few changes were made in the standard library to accommodate for this change.

assert sep.StrictlySplitFrom?(elem.cs, c => Spec.Structural(c, SpecView)) by {
assert sep.BytesSplitFrom?(elem.cs, c => Spec.Structural(c, SpecView)) by {
assert sep.SplitFrom?(elem.cs, c => Spec.Structural(c, SpecView));
assert sep.StrictlySplitFrom?(elem.cs, (c: Structural<jcomma>) => Spec.Structural(c, SpecView)) by {
Copy link
Member

@keyboardDrummer keyboardDrummer Mar 10, 2025

Choose a reason for hiding this comment

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

These types of changes are not exactly pretty. What's the trade-off here?

Copy link
Member

@keyboardDrummer keyboardDrummer left a comment

Choose a reason for hiding this comment

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

I committed a few small changes. Approving, although the change to no longer infer subset types for bound variables makes user code less readable. What would it take to enable that?

Ideally we would enable that before moving the stdlib to the new resolver, so it would not need many of the changes it got now.

# Conflicts:
#	Source/DafnyCore/AST/TypeDeclarations/Declaration.cs
@RustanLeino
Copy link
Collaborator Author

Thanks for the review, changes, and comments. I will merge this into master and will write down work items as an Issue.

the change to no longer infer subset types for bound variables makes user code less readable. What would it take to enable that?

This change was in the new resolver before. That is, this PR does not change this behavior. The old resolver could infer bound variables as subset types, but that can potentially create surprises. For example, given

predicate P(n: nat, s: string)
predicate Q(s: string)

consider inferring the type of x in

forall x, s :: if 0 <= x then P(x, s) else Q(s)

If the type of x is inferred as nat (which may be tempting, given that P's first parameter has type nat), then the else branch is dead code. The new resolver handles bound variables consistently by never inferring subset types.

@RustanLeino RustanLeino merged commit 60e9ae5 into dafny-lang:master Mar 19, 2025
22 checks passed
@RustanLeino RustanLeino deleted the refresh-by-default branch March 19, 2025 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Any change that will cause existing Dafny codebases to break (excluding verification instability)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants