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: Fix various type-system-refresh issues #5712

Merged
merged 11 commits into from
Sep 5, 2024

Conversation

RustanLeino
Copy link
Collaborator

Description

This PR contains various fixes in the new resolver, found while preparing Dafny to switch to --type-system-refresh as the default.

How has this been tested?

These issues that this PR fixes were found when running the test suite with a version of dafny that uses --type-system-refresh --general-traits=datatype --general-newtypes by default. However, since not all test files pass with these options, this PR does not change the default options, which also means that the existing tests don't test for the behavior that has been fixed. The purpose of this PR is to get these fixes in as soon as possible (and to make sure they don't break any existing functionality). The full testing will happen in a future PR that changes the default options.

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 September 4, 2024 18:48
@RustanLeino RustanLeino enabled auto-merge (squash) September 4, 2024 21:54
@@ -512,6 +525,19 @@ bool TryApplyDefaultAdviceFor(PreTypeProxy proxy) {
return false;
}

bool TryUseCompatibleTypesAsBounds() {
// if there is a compatible-types constraint "ty ~~ proxy", then decide on the bound "ty :> proxy"
Copy link
Member

Choose a reason for hiding this comment

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

What is a compatible types constraint, ty ~~ proxy ? If I'm in the code and I read this comment, can I follow links that lead me to a description of these constraints?

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.

Without a description of what the issues were and how they were resolved, I can't understand the functional changes made in this PR. I'm guessing this PR solves a newly discovered problem that was not discussed in previous documentation, so it would be good to extend the documentation.

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'll approve this since the code changes look fine, in case merging it before adding internal documentation serves some practical purpose.

@RustanLeino RustanLeino merged commit e387e3e into dafny-lang:master Sep 5, 2024
22 checks passed
@RustanLeino RustanLeino deleted the refresh-fixes branch September 5, 2024 23:13
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