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: Reveal statements can now invoke instance functions statically #4180

Merged
merged 21 commits into from
Jun 20, 2023

Conversation

EkanshdeepGupta
Copy link
Collaborator

Fixes #4176 by making generated reveal lemmas be static.

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

Copy link
Member

@MikaelMayer MikaelMayer left a comment

Choose a reason for hiding this comment

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

Two changes to bring, but thank you very much for this contribution!

@@ -47,6 +47,16 @@
<Content Include="..\..\node_modules\**\*.*" LinkBase="TestFiles\LitTests\LitTest\node_modules">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>
<Content Update="..\..\Test\git-issues\github-issue-4176.dfy">
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this change. We don't need this here, tests are discovered automatically. It was probably added by Rider.

Source/DafnyCore/Rewriters/OpaqueMemberRewriter.cs Outdated Show resolved Hide resolved
MikaelMayer
MikaelMayer previously approved these changes Jun 14, 2023
Copy link
Member

@MikaelMayer MikaelMayer left a comment

Choose a reason for hiding this comment

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

This is a great fix that surely @atomb will appreciate too ! Looking forward to the test suite passing.

@MikaelMayer
Copy link
Member

Still two more tests failing! But you are making great progress!

@atomb
Copy link
Member

atomb commented Jun 19, 2023

Oh, this is great! Requiring reveal to have an appropriate object in scope has been a significant difficulty for me in the past. And I'm glad it was so simple to fix!

Copy link
Member

@MikaelMayer MikaelMayer left a comment

Choose a reason for hiding this comment

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

Great fine tuning to ensure all tests pass !
One last thing: it's a feature, so please add an entry in docs/dev/reveal-opaque-instance-functions.feat with a line like "It's now possible to reveal an instance function of a class by a static reveal, without the need of an object of that class"

Copy link
Member

@MikaelMayer MikaelMayer left a comment

Choose a reason for hiding this comment

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

Let's ship it! Great job.

@MikaelMayer MikaelMayer enabled auto-merge (squash) June 20, 2023 16:53
@MikaelMayer MikaelMayer enabled auto-merge (squash) June 20, 2023 16:53
@MikaelMayer MikaelMayer merged commit fce9e26 into master Jun 20, 2023
@MikaelMayer MikaelMayer deleted the fix-4176-reveal-crash branch June 20, 2023 22:55
@MikaelMayer MikaelMayer changed the title Fix 4176 reveal crash Fix: Reveal statements can now invoke instance functions statically Sep 1, 2023
RustanLeino added a commit to RustanLeino/dafny that referenced this pull request Sep 13, 2023
RustanLeino added a commit that referenced this pull request Sep 13, 2023
Evidently, this was fixed by
#4180


<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
keyboardDrummer pushed a commit to keyboardDrummer/dafny that referenced this pull request Sep 15, 2023
Evidently, this was fixed by
dafny-lang#4180


<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
keyboardDrummer pushed a commit that referenced this pull request Sep 15, 2023
Evidently, this was fixed by
#4180


<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
keyboardDrummer pushed a commit that referenced this pull request Sep 19, 2023
Evidently, this was fixed by
#4180


<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
keyboardDrummer pushed a commit that referenced this pull request Sep 19, 2023
Evidently, this was fixed by
#4180


<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
RustanLeino added a commit that referenced this pull request Nov 2, 2023
PR #4180 introduced the ability
to `reveal` an instance function in a static context. In the recent PR
#4498, this has been implemented
to put the instance function itself, without a receiver argument, into
an attribute. The recent PR then special-cases the resolution of that
attribute. I would call that a hack.

This PR mimics that hack for the new resolver. That makes
`git-issue-4315.dfy` (see Issue
#4315) pass with both
resolvers.

The hack introduced in this PR and in
#4498 is not a great idea,
because a use of `this` is placed into the AST in a context where there
is no `this`. If (rather, when) this gets fixed, it should be fixed in
both resolvers.

### How has this been tested?

Test `git-issues/git-issue-4315.dfy` has been converted to run with both
the old and new resolver.

<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
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.

Dafny crashing when revealing a non-static class method from outside the class
3 participants