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

Let the IDE translate to and resolve Boogie only when attempting to verify #4378

Merged

Conversation

keyboardDrummer
Copy link
Member

@keyboardDrummer keyboardDrummer commented Aug 3, 2023

Changes

  • Let the IDE translate to and resolve Boogie only when attempting to verify, instead of doing those steps for all code in the project right after resolution, and before allowing any verification to be done.

    • Using the experimental verification caching feature to detect which tasks were already verified before the user triggers any verification, no longer works. This is because that caching relies on pre-verification which we now only do when the user triggers verification. However, when trying to verify these cached tasks, they'll be completed immediately after pre-verification finishes. In the future, we will have to implement caching of verification based on the Dafny AST if we want it to show up before pre-verification.
    • The Dafny AST nodes must now specify whether they can be verified, even before being translated to Boogie. This creates a slight duplication of effort, since this information was implicitly also provided by the Boogie translation
  • Properly close files after they are read, both .dfy and .dfyconfig.toml files

  • XUnit tests now all run sequentially. This does not seem to have a significant affect on total test runtime and makes debugging test timeouts easier because it excludes lack of available threads as a cause.

  • The class ConstantField is moved to a separate file

  • Add AutoGeneratedToken where they were missing.

  • Configure the dotnet test call on the CLI so individual server tests won't run for more than 6 minutes.

  • Split the existing OnChange mode into OnChangeFile and OnChangeProject, which let verification automatically run if either their containing file or containing project has changed, respectively.

  • Stringify, a method to help with debugging, has been improved

  • Small cleanup of CounterExampleTest, changing a List<Action> into an Action

  • Many improvements to logging

Testing

  • Existing tests have been adapted for the new behavior
  • The performance part improvement of this PR is not tested, since we don't have a good framework for testing performance.
  • Add tests for the behavior of OnChangeFile and OnChangeProject mode.
  • Add tests for Stringify
  • Add tests for FindNode

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.

Just finished my review. I did not test this PR myself yet, so I'm relying on the CI tests, although I did not find any obvious or potential issues in the code itself.

}

private IdeState CreateDocumentWithEmptySymbolTable(Compilation compilation,
IReadOnlyDictionary<Uri, IReadOnlyList<Diagnostic>> resolutionDiagnostics) {
var dafnyOptions = DafnyOptions.Default;
var program = new EmptyNode();
return new IdeState(
compilation.Version,
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that what was sufficient to ensure that gutter icons are handled in order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate?

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 before the Ide state did not have this field and I remember you mentioning to me that because versioning had been lost, gutter icons were sometimes published in the wrong order. I was wondering if this fixes that.

Source/DafnyLanguageServer/Workspace/CompilationManager.cs Outdated Show resolved Hide resolved
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.

One minor typo and I think it's good to go!

}

private IdeState CreateDocumentWithEmptySymbolTable(Compilation compilation,
IReadOnlyDictionary<Uri, IReadOnlyList<Diagnostic>> resolutionDiagnostics) {
var dafnyOptions = DafnyOptions.Default;
var program = new EmptyNode();
return new IdeState(
compilation.Version,
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 before the Ide state did not have this field and I remember you mentioning to me that because versioning had been lost, gutter icons were sometimes published in the wrong order. I was wondering if this fixes that.

@keyboardDrummer
Copy link
Member Author

It looks like before the Ide state did not have this field and I remember you mentioning to me that because versioning had been lost, gutter icons were sometimes published in the wrong order. I was wondering if this fixes that.

It previously sort of had this field, through its other field Compilation which has a version. This field is only needed because IdeState can now be migrated while keeping the same compilation, which is used in IdeObserverState.Migrate

MikaelMayer
MikaelMayer previously approved these changes Aug 10, 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.

Thanks for this excellent work.

@keyboardDrummer keyboardDrummer merged commit 99635a1 into dafny-lang:master Aug 11, 2023
18 checks passed
@keyboardDrummer keyboardDrummer deleted the onDemandPreverification branch August 11, 2023 08:39
keyboardDrummer added a commit to keyboardDrummer/dafny that referenced this pull request Sep 15, 2023
…erify (dafny-lang#4378)

Depends on dafny-lang#4395

### Changes

Let the IDE translate to and resolve Boogie only when attempting to
verify, instead of doing those steps for all code in the project right
after resolution, and before allowing any verification to be done.

- Using the experimental verification caching feature to detect which
tasks were already verified before the user triggers any verification,
no longer works. This is because that caching relies on pre-verification
which we now only do when the user triggers verification. However, when
trying to verify these cached tasks, they'll be completed immediately
after pre-verification finishes. In the future, we will have to
implement caching of verification based on the Dafny AST if we want it
to show up before pre-verification.
- The Dafny AST nodes must now specify whether they can be verified,
even before being translated to Boogie. This creates a slight
duplication of effort, since this information was implicitly also
provided by the Boogie translation

### Testing
- Existing tests have been adapted for the new behavior
- The performance part improvement of this PR is not tested, since we
don't have a good framework for testing performance.
- Add tests for the behavior of `OnChangeFile` and `OnChangeProject`
mode.

<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>

---------

Co-authored-by: Aaron Tomb <[email protected]>
keyboardDrummer added a commit that referenced this pull request Sep 19, 2023
…erify (#4378)

Depends on #4395

### Changes

Let the IDE translate to and resolve Boogie only when attempting to
verify, instead of doing those steps for all code in the project right
after resolution, and before allowing any verification to be done.

- Using the experimental verification caching feature to detect which
tasks were already verified before the user triggers any verification,
no longer works. This is because that caching relies on pre-verification
which we now only do when the user triggers verification. However, when
trying to verify these cached tasks, they'll be completed immediately
after pre-verification finishes. In the future, we will have to
implement caching of verification based on the Dafny AST if we want it
to show up before pre-verification.
- The Dafny AST nodes must now specify whether they can be verified,
even before being translated to Boogie. This creates a slight
duplication of effort, since this information was implicitly also
provided by the Boogie translation

### Testing
- Existing tests have been adapted for the new behavior
- The performance part improvement of this PR is not tested, since we
don't have a good framework for testing performance.
- Add tests for the behavior of `OnChangeFile` and `OnChangeProject`
mode.

<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>

---------

Co-authored-by: Aaron Tomb <[email protected]>
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.

None yet

3 participants