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: Better reporting if 'this' used in a subset type - and no crash #2994

Merged
merged 8 commits into from
Nov 10, 2022

Conversation

MikaelMayer
Copy link
Member

This PR fixes #2068
I added the corresponding test. The case of a "this" inside a subset type is not allowed, but there was no error mechanism to detect it. This PR solves that problem.

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

@@ -14875,7 +14874,11 @@ public ResolutionContext(ICodeContext codeContext, bool isTwoState)
if (currentClass is ClassDecl cd && cd.IsDefaultClass) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this case would also hit a crash, just later on in the pipeline?

Copy link
Member Author

Choose a reason for hiding this comment

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

How would you reproduce it?

Copy link
Member

Choose a reason for hiding this comment

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

I didn't have any luck, and it looks like the error immediately above should always be triggered, which seems to prevent any further problems.

// RUN: %dafny_0 /compile:0 "%s" > "%t"
// RUN: %diff "%s.expect" "%t"

type St = s:St_ | (forall o | o in s :: o.i(this))
Copy link
Member

Choose a reason for hiding this comment

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

As per my other comment, it would be great to show this fix working correctly in a few other illegal contexts as well, just for deeper testing coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added two other illegal context, the witness clause and a static const RHS. The static const RHS was also crashing Boogie. I was able to ensure the fixes use the same convention as for the existing code.
Of the top-level contexts, there is no other case to my knowledge where "this" should be disallowed.

robin-aws
robin-aws previously approved these changes Nov 8, 2022
@MikaelMayer MikaelMayer enabled auto-merge (squash) November 10, 2022 15:32
Copy link
Member

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

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

Nice, I like the refactoring to make this a more explicitly modeled restriction.

@MikaelMayer MikaelMayer merged commit 276bc48 into master Nov 10, 2022
@MikaelMayer MikaelMayer deleted the fix-2068-resolver-crash branch November 10, 2022 16:58
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.

Resolver crash
2 participants