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

Post-mortem/investigation on #2828 #2945

Closed
robin-aws opened this issue Oct 28, 2022 · 5 comments · Fixed by #3065
Closed

Post-mortem/investigation on #2828 #2945

robin-aws opened this issue Oct 28, 2022 · 5 comments · Fixed by #3065
Assignees
Labels
priority: next Will consider working on this after in progress work is done

Comments

@robin-aws
Copy link
Member

robin-aws commented Oct 28, 2022

#2790 introduced a case (issue ref), where the resolver crashed on an invalid Dafny program. This kind of regression is especially disruptive for those using the VSCode extension, since it crashes the extension completely, and it often isn't obvious to users what part of their code is triggering the bug so it looks like the extension is 100% broken.

Let's look into a few questions:

  1. Are there general mechanisms for making the extension and/or language server more robust on a program that can't be resolved without an exception?
  2. Are there general testing strategies for catching such bugs in the first place? Perhaps we could leverage https://github.com/dafny-lang/xdsmith to fuzz test by generating invalid programs.
  3. Could we make the extension automatically offer to revert back to a previous version of Dafny when this happens?
@MikaelMayer
Copy link
Member

  1. Back in July, there was a mechanism to ensure that every call to parser and resolver handled the exception if any. I know that @keyboardDrummer has modified the exception reporting mechanism since, but it's still there for the resolver and parser alltogether
  2. I don't know of any current testing strategy
  3. Everything is possible

@cpitclaudel
Copy link
Member

One thing that happens often is that you have an error in your code, the default path after detecting that error in the resolver breaks an invariant (e.g. something ends up being null or a TypeProxy), and later the code crashes.

In these cases the CLI output shows the error before the crash. So, we should either:

  • Tell the user to run on the CLI
  • Warn about the crash but still display any accumulated errors.

The second option would be particularly nice, but how easy is it?

@keyboardDrummer
Copy link
Member

keyboardDrummer commented Oct 31, 2022

Are there general mechanisms for making the extension and/or language server more robust on a program that can't be resolved without an exception?

Testing the IDE with the bug: it doesn’t recover if you remove the code that triggers the bug, even though it should. I'm guessing it fails to update the document state after you trigger the bug so it keeps working with the broken document.

Also, even though in the case of this bug the problem occurs both in the CLI and LSP server, I think we can do better to ensure uniformity between server and CLI. I think we should expose a small Dafny agnostic compiler API, something similar to LSP, that exposes enough for both the language server and CLI to operate on, so there is little room for differences between the CLI and language server. This would also mean we remove the duplication in compilation driving logic that exists between LSP server and CLI.

A coarse way to view this is that the CLI would run on top of the current LSP server back-end, which would move to DafnyPipeline : )

Also, PR related to this postmortem: #2949

@cpitclaudel
Copy link
Member

cpitclaudel commented Nov 1, 2022

I think we should expose a small Dafny agnostic compiler API, something similar to LSP, that exposes enough for both the language server and CLI to operate on, so there is little room for differences between the CLI and language server.

#2229 was a bit of that, but it wasn't complete (and we abandoned it because there was no identified use case at the time).

@keyboardDrummer
Copy link
Member

Action item: #2945

keyboardDrummer added a commit that referenced this issue Nov 21, 2022
Fixes #2945

### Changes

Remove an instance of static state in the compilation pipeline. Such
static state is bad because the pipeline may be run multiple times,
causing subsequent runs to be affected by previous ones.

### Testing

- TODO, but out of scope of this PR: change our integration tests to run
all the tests in a single Dafny process.

<!-- Is this a user-visible change? Remember to update RELEASE_NOTES.md
-->

<!-- Is this a bug fix? Remember to include a test in Test/git-issues/
-->

<!-- Is this a bug fix for an issue introduced in the latest release?
Mention this in the PR details and ensure a patch release is considered
-->

<!-- Does this PR need tests? Add them to `Test/` or to
`Source/*.Test/…` and run them with `dotnet test` -->

<!-- Are you moving a large amount of code? Read CONTRIBUTING.md to
learn how to do that while maintaining git history -->

<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
priority: next Will consider working on this after in progress work is done
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants