Introduce a new job for running early validation #1346

dbanck · 2023-08-04T12:18:00Z

This PR adds a new job for running the early validation logic.

After discussing different approaches (e.g., running validation inside a module hook), we decided to use a new job instead. This allows us to decouple the validation from the diagnostic publishing, which still runs as a module hook. The decoupling makes integrating with the existing terraform validate logic in the didSave handler easier.

It is planned to create a follow-up PR that splits the job into two different jobs. Two jobs will allow us to publish AST-based validations earlier than reference-based validations (which require the results from additional jobs).

Closes #1338

Add the ability to use the collected origin and target references in early validation by providing a hook for validation funcs. This also adds a validator for unreferenced variables. Validation funcs will be provided by terraform-ls for now, but may be moved into hcl-lang in the future.

radeksimko

LGTM aside from that one in-line comment and minor comment wording change.

radeksimko · 2023-08-23T15:09:51Z

internal/indexer/document_change.go

@@ -124,6 +124,19 @@ func (idx *Indexer) decodeModule(ctx context.Context, modHandle document.DirHand
 	}
 	ids = append(ids, refOriginsId)

+	_, err = idx.jobStore.EnqueueJob(ctx, job.Job{


Is there a thought process behind why we should or should not wait for this job to finish? We seem to wait for all of them except for the one below.

I believe the original idea was that anything that would be affected by the change (such as parsing) and where it matters that we process changes in the right order, we should wait. Technically the client won't really wait in the sense that the UI shouldn't be blocked but it may affect how we process next change notifications.

Here I think we should try to avoid publishing outdated diagnostics.

TL;DR I think we should wait, i.e. return job ID and append to the slice here.

I thought we were waiting on all jobs to make sure we're done processing everything when a user hits autocomplete (or hovers, or...).

Diagnostics require no user interaction, are owned by the server, and can be published whenever they're ready. Since validation depends on parsing and decoding, we'll always use the latest parsed information for validation.

I don't see how waiting for the validation job would change things, so I discarded the job id.

We can observe the UX to later understand the impact of that. I suppose most of the concerns I have about outdated diagnostics could be alleviated by us using the document version in our requests

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#publishDiagnosticsParams

/** * Optional the version number of the document the diagnostics are published * for. * * @since 3.15.0 */ version?: [integer](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#integer);

It still seems wasteful to be computing diagnostics for a version which is already outdated by the time we compute them but maybe that's a different problem that we can't really address here anyway.

So TL;DR feel free to merge as-is, we can look into it later if necessary.

internal/terraform/module/module_ops.go

Co-authored-by: Radek Simko <radek.simko@gmail.com>

* Implement unreferenced variable validation (#1357) Add the ability to use the collected origin and target references in early validation by providing a hook for validation funcs. This also adds a validator for unreferenced variables. Validation funcs will be provided by terraform-ls for now, but may be moved into hcl-lang in the future. * Introduce ValidationDiagnostics field to module * Publish early validation diagnostics * Include validation diagnotics in changes check * Introduce early validation job * Check ValidationDiagnosticsState when running validation * Run early validation job after collection jobs * Bump hcl-lang to `b6a3f8` * Update internal/terraform/module/module_ops.go Co-authored-by: Radek Simko <radek.simko@gmail.com> --------- Co-authored-by: James Pogran <jpogran@outlook.com> Co-authored-by: Radek Simko <radek.simko@gmail.com>

github-actions · 2023-09-24T03:02:09Z

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

dbanck added the enhancement New feature or request label Aug 4, 2023

dbanck self-assigned this Aug 4, 2023

radeksimko changed the base branch from main to f-early-validation August 8, 2023 16:12

jpogran force-pushed the f-early-validation-hook branch from 6399262 to 36dee47 Compare August 10, 2023 18:45

jpogran force-pushed the f-early-validation-hook branch from 74e43a0 to 0b65393 Compare August 18, 2023 16:25

dbanck force-pushed the f-early-validation-hook branch from 0b65393 to b2c3405 Compare August 21, 2023 17:32

dbanck force-pushed the f-early-validation branch from 4f3c074 to 91febd3 Compare August 22, 2023 11:37

jpogran and others added 5 commits August 22, 2023 13:37

Introduce ValidationDiagnostics field to module

6574d86

Publish early validation diagnostics

cf851ca

Include validation diagnotics in changes check

a16dd29

Introduce early validation job

86e41f7

dbanck force-pushed the f-early-validation-hook branch from b2c3405 to 86e41f7 Compare August 22, 2023 11:38

Check ValidationDiagnosticsState when running validation

d81461c

dbanck force-pushed the f-early-validation-hook branch from f92c827 to d81461c Compare August 23, 2023 07:55

dbanck added 2 commits August 23, 2023 15:15

Run early validation job after collection jobs

a759960

Bump hcl-lang to b6a3f8

9b88c1e

dbanck changed the title ~~Implement new module hook for early validation~~ Introduce a new job for running early validation Aug 23, 2023

dbanck marked this pull request as ready for review August 23, 2023 13:25

dbanck requested a review from a team as a code owner August 23, 2023 13:25

radeksimko approved these changes Aug 23, 2023

View reviewed changes

Update internal/terraform/module/module_ops.go

065f477

Co-authored-by: Radek Simko <radek.simko@gmail.com>

dbanck merged commit 6e83ea1 into f-early-validation Aug 24, 2023

dbanck deleted the f-early-validation-hook branch August 24, 2023 09:57

dbanck mentioned this pull request Aug 28, 2023

Introduce early validation module hook #1338

Closed

This was referenced Aug 29, 2023

Reference validation for remaining parts of the language #1364

Open

Send document-version-annotated diagnostics to avoid staleness #1365

Open

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a new job for running early validation #1346

Introduce a new job for running early validation #1346

dbanck commented Aug 4, 2023 •

edited

Loading

radeksimko left a comment

radeksimko Aug 23, 2023

dbanck Aug 23, 2023

radeksimko Aug 24, 2023

github-actions bot commented Sep 24, 2023

Introduce a new job for running early validation #1346

Introduce a new job for running early validation #1346

Conversation

dbanck commented Aug 4, 2023 • edited Loading

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Aug 23, 2023

Choose a reason for hiding this comment

dbanck Aug 23, 2023

Choose a reason for hiding this comment

radeksimko Aug 24, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2023

dbanck commented Aug 4, 2023 •

edited

Loading