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

Rootmodule diagnostics #288

Merged
merged 4 commits into from
Nov 11, 2020
Merged

Rootmodule diagnostics #288

merged 4 commits into from
Nov 11, 2020

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Nov 10, 2020

This refactor based on the new decoder architecture brings the language server as close as possible to having project wide diagnostics #270 . Diagnostics are calculated for the entire rootmodule on file open and change. It does mean that a user that has several rootmodules open won't see diagnostics far a particular rootmodule (effectively folder) until a file is opened within a that rootmodule.

Closes #270

@appilon appilon changed the title Refactor codebase to support root module wide diagnostics Rootmodule diagnostics Nov 10, 2020
diags = append(diags, lsp.Diagnostic{
Range: HCLRangeToLSP(*hclDiag.Subject),
Severity: HCLSeverityToLSP(hclDiag.Severity),
Source: "Terraform",
Copy link
Contributor Author

@appilon appilon Nov 10, 2020

Choose a reason for hiding this comment

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

Previously I had this set to HCL, however now I think since the diagnostics are coming from the new decoder, they could be interpreted as not just syntax, but also provider schema specific? Therefore I thought saying the source was Terraform was most appropriate, let me know if that is an incorrect interpretation

Copy link
Member

@radeksimko radeksimko Nov 11, 2020

Choose a reason for hiding this comment

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

The spec doesn't seem to go into much detail in describing this field, but it seems to suggest that each linter should be reported with its own Source - in our case I reckon that would apply to tflint and I would think potentially to terraform validate too?

The key question is how does the field affect the UX or anything in practice - I have no idea 🤷‍♂️

Re HCL/Terraform - our current diags in particular actually come just from hclsyntax.ParseConfig, so HCL might be more appropriate here. The schema is not involved in the validation at all.

f, pDiags := hclsyntax.ParseConfig(src, name, hcl.InitialPos)
diags = append(diags, pDiags...)
if f != nil {
files[name] = f
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll switch it back then.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, just one question about the subject-less diags.

langserver/diagnostics/diagnostics.go Outdated Show resolved Hide resolved
internal/lsp/diagnostics.go Outdated Show resolved Hide resolved
@appilon
Copy link
Contributor Author

appilon commented Nov 10, 2020

@radeksimko I moved uri to it's own pkg, I stopped suppressing subjectless diagnostics, and simply ensured a zero'd range is provided (So in theory a diagnostic will be attributed to the file, but will default to first line/column, diagnostics must have a range in LSP). PTAL

}
if r.End.Line < 0 {
r.End.Line = 0
}
Copy link
Member

Choose a reason for hiding this comment

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

Was this added as a reaction to any particular edge case you ran into?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HCL range start with index 1, in the event there is no subject, I passed a zero'd HCL range to the converter, this would result in negative values in the LSP conversion (since LSP range is index 0), this just ensures we don't send an invalid range

@appilon appilon merged commit d394354 into master Nov 11, 2020
@appilon appilon deleted the project-diags branch November 11, 2020 16:28
@radeksimko radeksimko added this to the v0.10.0 milestone Nov 12, 2020
@ghost
Copy link

ghost commented Dec 11, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate all diagnostics
2 participants