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

Improve error message for nested lists #21132

Closed
radeksimko opened this issue Apr 26, 2019 · 4 comments
Closed

Improve error message for nested lists #21132

radeksimko opened this issue Apr 26, 2019 · 4 comments

Comments

@radeksimko
Copy link
Member

It's expectable that users will use the upgrade command and won't actually face this error very often 👍 So it would more often be a result of a genuine human error, rather than artefact of 0.11 configuration.

However - if they don't use the upgrade command - or if they do and just genuinely make mistake - they're presented with the following diagnostic:

Error: Incorrect attribute value type

  on main.tf line 39, in resource "aws_elb" "web":
  39:   instances = ["${aws_instance.web.*.id}"]

Inappropriate value for attribute "instances": element 0: string required.

I'm not sure how far we can go in improving this without damaging UX elsewhere (I can't think of other common related edge cases), but I'd expect to see something like this:

Inappropriate value (list of list) for attribute "instances": list of string required.

Terraform Version

64b8751

Terraform Configuration Files

provider "aws" {
  region = "us-west-2"
}

data "aws_ami" "ubuntu" {
  most_recent = true

  filter {
    name   = "name"
    values = ["ubuntu/images/hvm-ssd/ubuntu-trusty-14.04-amd64-server-*"]
  }

  filter {
    name   = "virtualization-type"
    values = ["hvm"]
  }

  owners = ["099720109477"] # Canonical
}

resource "aws_instance" "web" {
  count         = 3
  ami           = "${data.aws_ami.ubuntu.id}"
  instance_type = "t2.micro"
}

resource "aws_elb" "web" {
  name = "web-elb"

  availability_zones = ["us-west-2a", "us-west-2b", "us-west-2c"]

  listener {
    instance_port     = 80
    instance_protocol = "http"
    lb_port           = 80
    lb_protocol       = "http"
  }

  instances = ["${aws_instance.web.*.id}"]
}

Steps to Reproduce

  1. terraform init
  2. terraform apply
@apparentlymart
Copy link
Contributor

There is some earlier discussion on this problem in #19140.

The general problem with this sort of proposed error is that HCL permits various automatic conversions, and so when it says "string required" it really means "anything that can be converted to a string required". In early development the error messages did include the given type, but it often led to some pretty confusing misdirections because the message would report various different mismatches but leave the user unclear about which of the mismatches are important and which are convertible.

As a result, we have these messages that try to focus on the specific leaf value in question, rather than the value as a whole, to direct the user to the right problem. If we were to implement the proposal here directly then the message would be something like this:

Inappropriate value (tuple) for attribute "instances": list of string required.

This is confusing because it sounds like the user needs to use a list instead of a tuple. That's a distraction though, because a tuple can convert to a list just fine, as long as the element types are correct. So the error is trying to direct the user's attention to the first element of the tuple and not mention the tuple at all.

In principle the error could be something like this:

Inappropriate value for attribute "instances": element 0: got list of string, but string required.

In this particular case that would work, but that's only because the inner types happen to match. If instead the splat were over an attribute that gives a number, the message would become confusing again:

Inappropriate value for attribute "instances": element 0: got list of number, but string required.

It's the wrapping list that is the problem now, but on a quick read it sounds like it's the number that's wrong.

This imprecision in error reporting is, I think, an unfortunate consequence of having a loosely-typed language with automatic type conversions. We designed the language so that users don't have to think about types too much, but as a result we don't have as much context to report as would be present in a strongly-typed language. Where possible we are showing the values associated with references in the expression to try to replace that missing type system context, but I don't think that would really help anything here (though I am still a little confused as to why this isn't already happening):

Error: Incorrect attribute value type

  on main.tf line 39, in resource "aws_elb" "web":
  39:   instances = ["${aws_instance.web.*.id}"]
  |-----
  | aws_instance.web is a tuple with 2 elements

Inappropriate value for attribute "instances": element 0: string required.

With all of that said, I would like to improve the error message for this specific scenario because there are lots of pre-0.12 examples out there showing this. There's no reason to write such a thing in 0.12 (bare aws_instance.web.*.id would be idiomatic) but we can expect users to try to apply examples shown in older articles and books.

Ideally I'd like to find a way to detect and report this specific pattern with a specialized error message that explains what's going on:

Error: Redundant list brackets

  on main.tf line 39, in resource "aws_elb" "web":
  39:   instances = ["${aws_instance.web.*.id}"]

Prior to Terraform v0.12 it was valid to place an extra set of list brackets
around an expression that itself returns a list, without turning the result
into a list of lists.

This usage is no longer valid. Remove these unnecessary brackets to produce
a list value directly.

Implementing such a message will require some trickery because the current error is happening inside hcldec rather than inside Terraform. I tried a few things already but I didn't find a good place to do this where there's enough information available to recognize the problem and report on it before hcldec deals with it and produces the existing error.

Hopefully with some more investigation and prototyping we can find a suitable place to at least approximate this in a best-effort way (catch some common cases but don't try to catch everything) via static analysis, by traversing the HCL expression AST and making some conservative assumptions.

I think let's leave this issue open for now so at least there's something to come up in search results if people encounter this problem, and then we can revisit this after v0.12.0 final to see if there is a reasonable heuristic for generating a better error message here.

@ruudk
Copy link

ruudk commented Sep 24, 2019

In encountered this issue when upgrading to TF 0.12 using the upgrade command. I think it would really help if TF would automatically output the content of the variable. Then you immediately see what you are doing cannot work because it's a different structure.

@apparentlymart
Copy link
Contributor

Hi all!

Anecdotally (based on what I've seen helping users diagnose confusing error messages in other online forums) it seems like the confusion about this particular situation has waned over time as we've got further from Terraform v0.11 and thus there are fewer configurations in the world incorrectly creating lists of lists where a flat list is expected.

We have made some gradual improvements to error messages in the intervening time which give some more context in the event that this situation would arise, and the terraform 0.12upgrade command in Terraform v0.12 is still there for folks who are belatedly upgrading from Terraform v0.11, so I think it's time to call this issue "done enough" and close it. If anyone has specific feedback about the current form of this error or of any similar issues, please do open a new issue to discuss it and we can see what we can do with the current error reporting mechanisms in Terraform.

Thanks!

@apparentlymart apparentlymart closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2022
@github-actions
Copy link

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 details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants