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

ast-check for zls! #617

Merged
merged 2 commits into from
Sep 1, 2022
Merged

ast-check for zls! #617

merged 2 commits into from
Sep 1, 2022

Conversation

SuperAuguste
Copy link
Member

@SuperAuguste SuperAuguste commented Sep 1, 2022

This should be ready to merge!

Small question I have is whether or not we should be emitting tree.errors diagnostics when ast-check diagnostics are enabled; they're usually the same (excluding stuff that only astgen can catch, of course) and it leads to error doubling (are there even cases where these would be different?)

Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

After taking a quick look into the source code of ast-check you can see here that tree.errors gets also printed as part of ast-check. Which explains why some diagnostics are very similar.

Therefore we have to options so that we avoid duplicate diagnostics:

  • Remove this code and let ast-check handle everything.
  • Only run and report ast-check if tree.errors is empty (i would recommend this option)

Also setup.zig should be updated according to the config changes.

src/Server.zig Outdated Show resolved Hide resolved
src/types.zig Outdated Show resolved Hide resolved
@Techatrix Techatrix merged commit d72cac0 into master Sep 1, 2022
@Techatrix Techatrix deleted the ast-check-diagnostics branch September 1, 2022 13:32
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.

2 participants