Errors with no range cannot be identified when emitted in recursive mode

[acdha]

Summary

I was remediating the other breaking changes in tflint 0.47.0 and noticed an odd one on a large project which has a number of local submodules. After duplicating our required providers/versions config into every subdirectory, all of my modules and the top-level have a required_version declared but there's still a single error reported with the “source code not available” message:

Warning: terraform "required_version" attribute is required (terraform_required_version)

  on  line 0:
   (source code not available)

At the very least it would be useful to know the current working directory when this error was generated – I confirmed that if I remove the required_version from one of my submodules it will also trigger the same context-free error so I used Semgrep to confirm that every terraform block in the entire project has a required_version (this is how I found the submodule without one).

rules:
    - id: terraform_block_without_required_version
      languages:
          - terraform
      severity: ERROR
      message: Terraform blocks must specify the required version
      patterns:
          - pattern: |
                terraform { ... }
          - pattern-not-inside: |
                terraform {
                    ...
                    required_version = "..."
                    ...
                }

$ semgrep --config=terraform_provider_versions.yml
               
               
┌─────────────┐
│ Scan Status │
└─────────────┘
  Scanning 214 files tracked by git with 1 Code rule:
  Scanning 158 files.
  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00                                                                                                                        
                
                
┌──────────────┐
│ Scan Summary │
└──────────────┘
Some files were skipped or only partially analyzed.
  Scan was limited to files tracked by git.

Ran 1 rule on 158 files: 0 findings.

Command

tflint --only=terraform_required_version --recursive

Terraform Configuration

private

TFLint Configuration

rule "terraform_workspace_remote" {
  enabled = false
}

Output

tflint --only=terraform_required_version --recursive
1 issue(s) found:

Warning: terraform "required_version" attribute is required (terraform_required_version)

  on  line 0:
   (source code not available)

Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.4.0/docs/rules/terraform_required_version.md

TFLint Version

0.47.0

Terraform Version

Terraform v1.5.1 on darwin_arm64 + provider registry.terraform.io/hashicorp/archive v2.3.0 + provider registry.terraform.io/hashicorp/aws v4.67.0 + provider registry.terraform.io/hashicorp/dns v3.3.2 + provider registry.terraform.io/hashicorp/external v2.3.1 + provider registry.terraform.io/hashicorp/http v3.3.0 + provider registry.terraform.io/hashicorp/local v2.4.0 + provider registry.terraform.io/hashicorp/null v3.2.1

Operating System

• Linux
• macOS
• Windows

[bendrucker]

Retitled as there's no direct relationship to required_version. There are many rules that could potentially emit an issue about a lack of some required configuration, in which case there would be no associated source range. So as an enhancement to the recursive feature, we'd want to figure out some way to handle this at the TFLint level. In general it's never going to be helpful to attempt to print a zero-value hcl.Range anyway so perhaps this shouldn't be limited to recursive mode.

[wata727]

The simplest workaround for now is to always emit a range that includes the module path in a rule like terraform_required_version.

In the current design, the tflint.Issue is not related to a module, so if we'd want to implicitly assign the module path at the TFLint level, it may not be obvious in what cases the module path should be assigned.

[bendrucker]

The simplest workaround for now is to always emit a range that includes the module path in a rule like terraform_required_version.

Yep, thought about this. It's an improvement for end users but a hack and I worry where it might break. A range is meant to be associated with a file. Right now we print "source not available" when the file name is empty. While that could also apply when the file doesn't exist (because it's actually a dir), it's awkward to have to account for that discrepancy anywhere in the code. Seems like recursive mode would justify attaching a source module reference when emitting an issue so we can print its path for issues with no range.

A variation on the dir as range filename hack that would cover all comparable rules would be to handle this at the SDK level when calling EmitIssue. If the range is a zero-value, implicitly replace it with a range that refers to the directory.

[wata727]

Seems like recursive mode would justify attaching a source module reference when emitting an issue so we can print its path for issues with no range.

Makes sense. I agree to add a source module reference to tflint.Issue for such cases. This seems to avoid a hack for the empty range.

[wata727]

I currently have a slightly different position on this matter, so I'll share my thoughts.

Initially I was positive about attaching the source module reference. However, considering that the issues are processed by other tools (e.g. SARIF, LSP, problem matchers, etc.), it can be difficult to deal with issues attached to module directories.

Given this, it might be better to force the rules to emit issues for valid lines in some file in all cases.

Regarding recursion inspection, I think it's not a bad idea to attach a module reference to improve the output with the default formatter, apart from the valid ranges used for JSON output etc.