Validate On Save

[appilon]

This creates a new configuration object for the language server called ExperimentalFeatures which allows users to opt into specific features which may not be ready to be on by default. The first feature is calling terraform validate on save (with the saved file as the target directory).

This does work as expected, however the user experience is a bit awkward in 2 cases:

• Validate is not run on open, only on save. This makes it awkward when opening your project, you will not see any validation errors for the file in focus until you save.
• Modules is supported, but the experience isn't ideal. For simplicity, in Support Terraform Validate Command #323 we do not run terraform validate when a module file is saved. This is because it could be referenced/downstream from multiple root modules (we don't know which root module to call terraform validate in? Could lead to weird duplicate diagnostics). terraform validate does of course provide diagnostics for any referenced modules when saving a file in a root module (great! as expected).
  • The problem is that when "fixing" the mistake in the module file and saving, the diagnostic is not cleared, since for reasons mentioned above, we don't run terraform validate from within the module folder

I think with the right messaging, and the fact that the feature is opt-in, that this UX is okay for now, but could use improvement.

To be clear this clearing issue only affects saves of modules, not of regular files of "rootmodules" (side note we really need to fixup our nomenclature on this topic)

In contrast, when we enable calling validate from the command palette on the client side, we would ask the user to specify which folder to run validate in, from a quick-picker style dialog. This experience is smoother since I think intuitively the user will always pick the right spot and won't experience this awkward clearing deficiency.

[radeksimko]

Validate is not run on open, only on save. This makes it awkward when opening your project, you will not see any validation errors for the file in focus until you save.

I wouldn't say it's awkward, that's just the nature of how it works given the limitations we're working with. I have doubts that this behaviour will change any time soon due to the fact that validate requires providers to be installed, which in turn introduces a lot of complexity, i.e. increases chance of validate to fail for reasons unrelated to the configuration itself. As far as I know there's unfortunately a decent amount of users which start the editor without running init and it wouldn't make sense to hit them with (even more) errors straight after opening the folder.

Relatedly I think we should probably detect uninitialized workspace - i.e. missing providers and either ignore validation entirely, or raise that as info or warning, certainly not as an error, but I'm totally happy for that to be addressed in a separate PR given that it's opt-in for now.

Modules is supported, but the experience isn't ideal. For simplicity, in #323 we do not run terraform validate when a module file is saved. This is because it could be referenced/downstream from multiple root modules (we don't know which root module to call terraform validate in? Could lead to weird duplicate diagnostics). terraform validate does of course provide diagnostics for any referenced modules when saving a file in a root module (great! as expected).

I'm not sure what the best UX here is either, but I think we should at least display diags which are actionable - i.e. diags from local modules which can be edited and fixed from where they're installed, since changes don't involve release, but can be committed and tested alongside the module which calls this submodule.

As for external modules (coming from GitHub or the Registry) I'm not sure. I guess it's useful to know whether the module works in a particular context with particular variable values, but I'm not sure how useful that is if it can't really be fixed from that context - i.e. users should not be editing external modules installed into .terraform.

All that said I'd be happy to leave UX enhancements for a follow-up PR.

This overall LGTM aside from my two inline comments.

[radeksimko]

Good catch! Looks like this was previously prone to race conditions if we were parsing files (and hence writing into the map) while also reading from that map.

[radeksimko]

Is there a reason for reading the module manifest here? Could we not create the entries "lazily" automatically below? e.g. something like

diags, ok := diagsMap[d.Range.Filename]
if !ok {
    diagsMap[d.Range.Filename] = make(hcl.Diagnostics, 0)
    diags = diagsMap[d.Range.Filename]
}

I understand there is a need for filtering out submodule diags, but couldn't this be done via something like strings.HasPrefix(d.Range.Filename, rm.Path())?

[appilon]

ah I don't believe so no, the point of this is to create an entry in the map for every possible file, so that clearing diagnostics over the protocol works correctly. I also authored this a while though so I can't describe it in detail, but we can revisit/refactor in follow ups

[radeksimko]

One more thing: Will you document the new config option under https://github.com/hashicorp/terraform-ls/blob/main/docs/SETTINGS.md please?

[ghost]

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.