Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hclsyntax: Improve conditional type mismatch errors (somewhat) #530

Merged
merged 2 commits into from Apr 21, 2022

Conversation

apparentlymart
Copy link
Member

For a long time now we've had a very simplistic error message for the case of conditional expression result arms not having the same type, which only works for situations where the two types have differing "friendly names" down in the cty layer.

Unfortunately due to the typical complexity of the structural type kinds (object and tuple types) their friendly names are just "object" and "tuple", which tends to lead us to seemingly-incorrect error messages like:

The true and false result expressions must have consistent types.
The given expressions are object and object, respectively.

This changeset then is an attempt to use some more specialized messaging in some of the situations that led to that sort of weird message before. In particular, this handles:

  • both types are object types but their attributes don't match
  • both types are tuple types but their elements don't match
  • both types are the same kind of collection of either object or tuple types which don't match

These are the three shallow cases that the previous logic wasn't able to properly describe. This still leaves unaddressed a hopefully-less-common case of nested collections with differing structural types in their depths, but still avoids generating a confusing error message by instead generating a very vague but still correct error message:

At least one deeply-nested attribute or element is not compatible
across both the 'true' and the 'false' value.

My intent here is to make HCL return something precise enough most of the time, without letting perfect be the enemy of the good. This will generate some quite obnoxious long messages for particularly complex nested structures, but so far it appears that such values are relatively rare inside conditional expressions and so we'll wait to see what arises in practice before trying to handle those situations more concisely.

Ideally I would like to include some actionable feedback that in some cases it can help to explicitly convert ambiguously-typed expressions like null or tuples intended to be lists to the intended type, so that the type unification step has more information to infer the author intent. However, HCL itself doesn't have any builtins for such conversions and so today any messaging about that would need to be generated up at the application layer so the application can refer to whatever functions/etc it provides for type conversion. For example, Terraform would presumably recommend using tolist in situations where the author intends to choose between two different lists of the same type but differing lengths. It isn't clear how to do achieve an error message like that with the current design, so we'll leave that to be addressed another day.

For a long time now we've had a very simplistic error message for the
case of conditional expression result arms not having the same type, which
only works for situations where the two types have differing "friendly
names" down in the cty layer.

Unfortunately due to the typical complexity of the structural type kinds
(object and tuple types) their friendly names are just "object" and
"tuple", which tends to lead us to seemingly-incorrect error messages
like:

    The true and false result expressions must have consistent types.
    The given expressions are object and object, respectively.

This then is an attempt to use some more specialized messaging in some
of the situations that led to that sort of weird message before. In
particular, this handles:
 - both types are object types but their attributes don't match
 - both types are tuple types but their elements don't match
 - both types are the same kind of collection of either object or tuple
   types which don't match

These are the three _shallow_ cases that the previous logic wasn't able to
properly describe. This still leaves unaddressed a hopefully-less-common
case of nested collections with differing structural types in their
depths, but still avoids generating a confusing error message by instead
generating a _very vague but still correct_ error message:

    At least one deeply-nested attribute or element is not compatible
    across both the 'true' and the 'false' value.

My intent here is to make HCL return something precise enough _most of the
time_, without letting perfect be the enemy of the good. This will
generate some quite obnoxious long messages for particularly complex
nested structures, but so far it appears that such values are relatively
rare inside conditional expressions and so we'll wait to see what arises
in practice before trying to handle those situations more concisely.

Ideally I would like to include some actionable feedback that in some
cases it can help to explicitly convert ambiguously-typed expressions
like "null" or tuples intended to be lists to the intended type, so that
the type unification step has more information to infer the author intent.
However, HCL itself doesn't have any builtins for such conversions and so
today any messaging about that would need to be generated up at the
application layer so the application can refer to whatever functions/etc
it provides for type conversion. It isn't clear how to do that with the
current design, so we'll leave that to be addressed another day.
@apparentlymart apparentlymart added enhancement v2 Relates to the v2 line of releases syntax/native labels Apr 15, 2022
@apparentlymart apparentlymart requested a review from a team April 15, 2022 22:41
@apparentlymart apparentlymart self-assigned this Apr 15, 2022
@alisdair
Copy link
Member

An example Terraform bug tracking this problem downstream: hashicorp/terraform#30562

@apparentlymart
Copy link
Member Author

Thanks for the pointer to that Terraform issue, @alisdair!

Out of curiosity I tried that given example out with a local build of Terraform that has this PR integrated into it, and got the following updated error message, which seems reasonable to me:

╷
│ Error: Inconsistent conditional result types
│ 
│   on fun-with-tuples.tf line 20, in output "test":
│   20:   value = true ? { foo = { bar = 42 } } : { bar = 44 }
│ 
│ The true and false result expressions must have consistent types.
│ The 'true' value includes object attribute "foo", which is absent
│ in the 'false' value.
╵

As with everything about this PR I would like to eventually find a better way to present these that gives some better context and guidance on how to fix it, but it does at least achieve the immediate goal of actually saying what the problem is, rather than making a useless statement about the top-level type kinds.

Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think this is a good compromise between brevity and precision. I don't recall seeing any Terraform issues with the existing behaviour which used nested collections (they were always object trees), so I think it's very likely this will address the majority of instances.

@apparentlymart apparentlymart merged commit 357a7fd into main Apr 21, 2022
@apparentlymart apparentlymart deleted the f-hclsyntax-conditional-type-errs branch April 21, 2022 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement syntax/native v2 Relates to the v2 line of releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants