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

Solution to issue 2265 related to erroneous substitution under bindin… #2745

Merged
merged 9 commits into from
Sep 17, 2022

Conversation

jtristan
Copy link
Contributor

…g guards

Fixes #2265

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

@jtristan jtristan added kind: bug Crashes, unsoundness, incorrect output, etc. If possible, add a `part:` label part: verifier Translation from Dafny to Boogie (translator) status: needs-expert Needs review by an expert on this part of the code labels Sep 14, 2022
Copy link
Member

@cpitclaudel cpitclaudel left a comment

Choose a reason for hiding this comment

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

Great fix! A few suggestions:

  • There are still a few whitespace-only changes in unrelated parts of the code that would be nice to undo
  • I think this PR is missing a fix for the if-case pattern that @RustanLeino sent an example of in the original issue discussion (if x :| P(x) => …)
  • The split-quantifiers case isn't quite right, I think — but the original code was broken too (!)
  • I've left other small comments in the code

Source/DafnyCore/Substituter.cs Show resolved Hide resolved
Source/DafnyCore/Substituter.cs Show resolved Hide resolved
Source/DafnyCore/Substituter.cs Show resolved Hide resolved
Source/DafnyCore/Substituter.cs Outdated Show resolved Hide resolved
Source/DafnyCore/Substituter.cs Show resolved Hide resolved
Source/DafnyCore/Substituter.cs Show resolved Hide resolved
Source/DafnyCore/Substituter.cs Show resolved Hide resolved
Source/DafnyCore/Substituter.cs Show resolved Hide resolved
Test/git-issues/git-issue-2265.dfy Outdated Show resolved Hide resolved
@jtristan
Copy link
Contributor Author

With respect to the additional issue raised by Rustan involving a case statement, I decided to create a new issue because even if the fix might be small and simple, I would first like to make sure I understand the semantics, and I will add a new corresponding test.

Copy link
Member

@cpitclaudel cpitclaudel left a comment

Choose a reason for hiding this comment

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

Thanks! Can you revert the whitespace-only changes? Then we can merge! :)

cpitclaudel
cpitclaudel previously approved these changes Sep 16, 2022
@cpitclaudel cpitclaudel enabled auto-merge (squash) September 16, 2022 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Crashes, unsoundness, incorrect output, etc. If possible, add a `part:` label part: verifier Translation from Dafny to Boogie (translator) status: needs-expert Needs review by an expert on this part of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ArgumentNullException in Translator.CheckDefiniteAssignment
2 participants