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

Add tooltip on assertions that shows which functions are hidden #5929

Merged
merged 17 commits into from
Nov 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Source/DafnyCore/AST/Tokens.cs
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,15 @@ public int CompareTo(IToken other) {
public int CompareTo(Boogie.IToken other) {
return WrappedToken.CompareTo(other);
}

public static IToken Unwrap(IToken token, bool includeRanges = false) {
MikaelMayer marked this conversation as resolved.
Show resolved Hide resolved
if (token is TokenWrapper wrapper
&& (includeRanges || token is not RangeToken)) {
return Unwrap(wrapper.WrappedToken);
}

return token;
}
}

public static class TokenExtensions {
Expand Down
10 changes: 6 additions & 4 deletions Source/DafnyCore/Pipeline/Compilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -363,12 +363,14 @@ private async Task VerifyUnverifiedSymbol(bool onlyPrepareVerificationForGutterT
await ticket;

if (!onlyPrepareVerificationForGutterTests) {
foreach (var tokenTasks in tasks.GroupBy(t =>
BoogieGenerator.ToDafnyToken(true, t.Token)).
OrderBy(g => g.Key)) {
var groups = tasks.GroupBy(t =>
// We unwrap so that we group on tokens as they are displayed to the user by Reporter.Info
TokenWrapper.Unwrap(BoogieGenerator.ToDafnyToken(true, t.Token))).
OrderBy(g => g.Key);
foreach (var tokenTasks in groups) {
var functions = tokenTasks.SelectMany(t => t.Split.HiddenFunctions.Select(f => f.tok).
OfType<FromDafnyNode>().Select(n => n.Node).
Copy link
Member

Choose a reason for hiding this comment

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

Wrapping tokens to filter functions is not ideal. There are multi competing token wrapping strategies that will end up failing this test in the future. For example, refinement tokens will wrap your FromDafnyNode token making them not pass this test. There are other wrapped tokens types, like auto-generated tokens, nested tokens, override tokens for traits, etc.
I have always advocated for replacing tokens with a single meta-data field on which we would add some flags as needed.

I'm not expecting you to do this refactoring, so in the meantime, please use something like FromDafnyNode.Has() that would recursively traverse the tokens to find one of its kind.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here the wrapping happens right before handing the token to Boogie, so Dafny won't do any further wrapper, and Boogie might do some wrapping on tokens but only use that for nodes that it creates, so not on the nodes that I'm interested in. I don't think there's a potential issue here. If there is one, how can we test for it?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, good to know that the risk is mitigated. I think that we still have the risk that later, someone will wrap your token after your own wrapping and their test cases will be orthogonal to yours, so both won't be tested together.
If you want to make sure your wrapping is the last one before sending the token to boogie, I'd suggest: changing the name to something like DafnyMetaData so that future development will become compelled to add metadata here rather than adding a new wrapping token.

OfType<Function>()).Distinct();
OfType<Function>()).Distinct().OrderBy(f => f.tok);
var hiddenFunctions = string.Join(", ", functions.Select(f => f.FullDafnyName));
if (!string.IsNullOrEmpty(hiddenFunctions)) {
Reporter.Info(MessageSource.Verifier, tokenTasks.Key, $"hidden functions: {hiddenFunctions}");
Expand Down
94 changes: 94 additions & 0 deletions Source/DafnyLanguageServer.Test/Diagnostics/DiagnosticsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1331,11 +1331,105 @@ assert Outer(3) by {
reveal Outer, Inner;
}
}


".TrimStart();
var documentItem = CreateTestDocument(source, "HiddenFunctionHints.dfy");
client.OpenDocument(documentItem);
var diagnostics = await GetLastDiagnostics(documentItem, minimumSeverity: DiagnosticSeverity.Hint);

Assert.Equal(6, diagnostics.Length);
var sorted = diagnostics.OrderBy(d => d.Range.Start).ToList();
for (int index = 0; index < 3; index++) {
Assert.Equal(sorted[index * 2].Range, sorted[index * 2 + 1].Range);
}
}

[Fact]
public async Task HiddenFunctionTooltipBlowup() {
var source = @"
predicate A(x: int) {
x < 5
}
predicate B(x: int) {
x > 3
}
predicate P(x: int) {
A(x) && B(x)
}

method {:isolate_assertions} TestIsolateAssertions() {
hide *;
assert P(3);
}
".TrimStart();
var documentItem = CreateTestDocument(source, "HiddenFunctionHints.dfy");
client.OpenDocument(documentItem);
var diagnostics = await GetLastDiagnostics(documentItem, minimumSeverity: DiagnosticSeverity.Hint);

var infoDiagnostics = diagnostics.Where(d => d.Severity >= DiagnosticSeverity.Information).ToList();
Assert.Single(infoDiagnostics);
}

[Fact]
public async Task HiddenFunctionTooltipBlowup2() {
var source = @"
predicate A(x: int) {
x < 5
}
predicate B(x: int) {
x > 3
}
predicate P(x: int) {
A(x) && B(x)
}

method {:isolate_assertions} TestIsolateAssertions() {
hide *;
reveal P;
assert P(3);
}
".TrimStart();
var documentItem = CreateTestDocument(source, "HiddenFunctionHints.dfy");
client.OpenDocument(documentItem);
var diagnostics = await GetLastDiagnostics(documentItem, minimumSeverity: DiagnosticSeverity.Hint);

var infoDiagnostics = diagnostics.Where(d => d.Severity >= DiagnosticSeverity.Information).ToList();
Assert.Single(infoDiagnostics);
}

[Fact]
public async Task HiddenFunctionTooltipBlowup3() {
var source = @"
predicate A(x: int) {
x < 7 && C(x)
}
predicate B(x: int) {
x > 3
}

predicate C(x: int) {
x < 5
}

predicate P(x: int)
requires A(x) && B(x)
{
true
}

method {:isolate_assertions} TestIsolateAssertions() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you can now use @IsolateAssertions

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice

Copy link
Member Author

Choose a reason for hiding this comment

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

Would be good to update the test-suite so it uses that everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Yup. I updated the libraries only for now.

hide *;
reveal P;
assert P(6);
}
".TrimStart();
var documentItem = CreateTestDocument(source, "HiddenFunctionHints.dfy");
client.OpenDocument(documentItem);
var diagnostics = await GetLastDiagnostics(documentItem, minimumSeverity: DiagnosticSeverity.Hint);

var infoDiagnostics = diagnostics.Where(d => d.Severity >= DiagnosticSeverity.Information).ToList();
Assert.Single(infoDiagnostics);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a single info diagnostic because it's possible to prove the predicate B()?
What happens if you replace 6 by an unknown variable, will you also get only one hint with two error messages?
Also, could you please test what is the diagnostic message?

I would like to precisely understand why there is only one information message when there are two things to verify at this point.

Copy link
Member Author

@keyboardDrummer keyboardDrummer Nov 26, 2024

Choose a reason for hiding this comment

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

I would like to precisely understand why there is only one information message when there are two things to verify at this point.

There are multiple assertions at the Boogie level, but they all have the same range, and the diagnostics are grouped by range, so they're all piled together and end up as a single info message.

If we would change the reporting so the assertion error is shown on the assert keyword, while the call precondition error is show on the call, then it would show up as two separate hints. However, they would not have overlapping ranges, so they would not show up in the same tooltip.

If there are assertions with overlapping ranges, then two hiding messages can show up in one tooltip, which is unfortunate. My proposed solution is to make it so that non of the ranges of assertion diagnostics are both overlapping and not equal. That way, tooltips are less cluttered with errors related to different assertions.

}

public DiagnosticsTest(ITestOutputHelper output) : base(output) {
Expand Down