hcl icon indicating copy to clipboard operation
hcl copied to clipboard

Support unknown variable evaluation

Open notnoop opened this issue 5 years ago • 3 comments

Expose a callback to handle unknown variables found in hcl syntax. This is useful in situations where the variables aren't known statically upfront.

When a chain of EvalContexts is used, the deepest child context with set UnknownVariable is used.

notnoop avatar Oct 23 '20 12:10 notnoop

Hi @notnoop!

This seems reasonable to me in principle. I'd like to suggest a couple small tweaks to it in the hope of making it fit better with the existing features of this module:

  • The word "unknown variable" confused me a little at first because HCL (via cty) already has a special idea of a "unknown value", which is a variable that is explicitly defined as not being known yet. I'd prefer to call this new field UndefinedVariable or MissingVariable to avoid creating two meanings of the word "unknown" in this context.

  • In HCL, functions that might fail typically return hcl.Diagnostics instead of error, so that the function can return zero or more structured diagnostic messages rather than an opaque error objects. Since this callback function is called during expression evaluation, I'd prefer its signature to return (cty.Value, hcl.Diagnostics) and then return the diagnostics to the TraverseAbs caller.

    The typical pattern for that is for the function to start by declaring a zero-value Diagnostics and then gradually append new diagnostics to it as it goes:

    var diags Diagnostics
    // ...
    if unknownHandler != nil {
          v, moreDiags := unknownHandler(t)
          diags = append(diags, moreDiags...)
          return v, diags
    }
    //
    diags = diags.Append(&Diagnostic{
        Severity: DiagError,
        Summary:  "Unknown variable",
         // etc
    })
    return cty.DynamicVal, diags
    

    (Note that diags can be non-nil even if there are no errors, because it can potentially contain warnings too.)

apparentlymart avatar Dec 02 '20 23:12 apparentlymart

Thanks @apparentlymart for the feedback and sorry for taking long to get back. I have updated the PR to use UndefinedVariable.

I have opted not go through the diags accumulation approach. The function TraverseAbs never returns the accumulation of multiple diagnostics. If UndefinedVariables returns some error diagnostics, we probably should return those as is, without additional "did you mean ..." suggestions, showing multiple, maybe conflicting, error messages for the same variable reference will be confusing to users, I suspect.

notnoop avatar Jan 29 '21 14:01 notnoop

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


Mahmood Ali seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you already have a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

hashicorp-cla avatar Sep 09 '21 10:09 hashicorp-cla