Prevent diagnostic writes from panicking on marked values

[wata727]

Fixes #737

When the DiagnosticTextWriter writes out a diagnostic, if the diagnostic has an Expression and EvalContext, it will also write out the evaluation context in which the diagnostic occurred.

The cty.Value of the variable relevant to the expression will be rendered as a string by valueStr, but this does not take into account marked values and will cause a panic by val.AsString(), val.LengthInt(), etc.

This change prevents panics by always returning "(marked value)" for marked values in valueStr. Whether this is always appropriate is debatable, but it is better than the current situation, which causes panics.

---

This PR changes valueStr, but there is also an idea to change WriteDiagnostic. For instance, unknown values ​​will be returned as "(not yet known)" in valueStr, but WriteDiagnostic will prevent unknown values ​​from being rendered:

hcl/diagnostic_text.go

Lines 168 to 170 in 561e199

	case !val.IsKnown():
	// Can't say anything about this yet, then.
	continue

I'd be happy to discuss here which one is more appropriate.

However, if you take this approach, you will need to adapt traversalStr as well.

hcl/diagnostic_text.go

Line 237 in 561e199

buf.WriteString(w.valueStr(tStep.Key))

EDIT: This is the same approach as Terraform.
https://github.com/hashicorp/terraform/blob/v1.5.7/internal/command/views/json/diagnostic.go#L400-L406

[crw]

Hi @wata727, in triage it was confirmed that unexpected marks are intended to panic in HCL. Language-specific implementations e.g. Terraform can handle that as they see fit, per your referenced code. Thanks for this submission!

[jbardin]

Hi @wata727, To clarify here a little more -- I think there may be an issue to resolve, but it's not clear exactly what should be done. HCL has no knowledge of marks, so hiding the information isn't necessarily the correct action, if anything I'm leaning towards just stripping the marks for display if it gets to this point. Terraform avoids the panic by dealing with unexpected marks before generating the diagnostic message, which is more consistent with the conceptual use of marks overall.

[apparentlymart]

This is a tricky case indeed: "marks" are an implementation detail that isn't really appropriate to expose directly as a user-facing concept, but HCL doesn't understand what they are intended to represent so it cannot really say anything useful about them.

When I originally wrote this default diagnostic presentation thing I think it didn't include the summary of values in scope at all and so it worked okay as a relatively simple diagnostic implementation for applications without special needs, but more complex systems like Terraform could provide their own.

Personally, the compromise I would choose here would be for it to just not say anything about marked values in the UI at all. Showing additional contextual information in diagnostics is always best-effort anyway, and so omitting some information that HCL doesn't know how to present seems like a good fit for a general-purpose default implementation, so those using it can assume it will just do its best to show the simple stuff.

As a bonus that seems consistent with Terraform's policy of only mentioning sensitive values for certain diagnostics that are known to be directly caused by a value being sensitive, since otherwise readers tend to incorrectly consider the sensitivity more significant than it actually is. HCL itself doesn't have any messages that are directly caused by sensitivity because it doesn't even know about that concept, so not ever showing such values effectively matches the Terraform decision.

Of course, this is just my opinion. 😀

[wata727]

Thank you everyone for the confirmation and clarification. I agree that the solution to this issue is not clear.
Given all the information above, it looks like we have two options:

1. Keep the current implementation which panics if a marked value is written out
   • In this case it is the caller's responsibility to ensure that diagnostics do not include marked value. Alternatively, you can fork the DiagnosticTextWriter to handle your own application-defined marks.
   • Given that HCL knows nothing about marks, it's a bit inconvenient but understandable, but any developer encountering this behavior will likely, like me, think it's a bug at first.
2. Prevents WriteDiagnostic from calling valueStr on marked values
   • This is @apparentlymart's suggestion. It goes against @jbardin's stripping the marks approach, but seems like an acceptable compromise, provided the extra diagnostic information is best-effort.

As an extension to 1, another approach might be to allow the caller to register a callback to handle application-defined marks. Imagine something like this:

dwr := hcl.NewDiagnosticTextWriter(os.Stdout, map[string]*hcl.File{"main.tf": file}, 40, true)
dwr.HandleMarkFunc(func(val cty.Value) string {
        if val.HasMark(marks.Sensitive) {
                return "(sensitive value)"
        }
        panic("value is marked, so must be unmarked first")
})

However, this idea may be difficult to move forward as it requires breaking changes to the DiagnosticWriter interface.

@jbardin @crw As current maintainers, what do you think about these options?

[jbardin]

I don't think the notion of marks should be incorporated into the API here, because they should generally be transparent through HCL. Also the diagnostic may just be in a place where the caller doesn't expect marks and is not prepared to deal with them anyway, so adding special API doesn't help.

I'm fine with either direction of option 2, either way seems perfectly valid. Not calling valueStr reduces the utility of the diagnostic, but stripping the marks has the possibility of presenting something contrary to the caller's use of the mark (though HCL should not care about the caller's interpretation of marks). The most conservative approach is to not call valueStr, so I'd recommend going with that, and we could always reconsider adding output later.

[wata727]

Thanks for your feedback. Opened #739 as a follow up to this PR.