hashicorp / syntax

TextMate grammars for highlighting HCL, HCL-based languages and Sentinel.
Mozilla Public License 2.0
23 stars 11 forks source link

First segment of references and object contents mis-highlighted #79

Open scott-doyland-burrows opened 1 year ago

scott-doyland-burrows commented 1 year ago

Extension Version

v2.26.20230511

VS Code Version

Version: 1.79.1 (user setup) Commit: 4cb974a7aed77a74c7813bdccd99ee0d04901215 Date: 2023-06-12T16:14:05.102Z Electron: 22.5.7 Chromium: 108.0.5359.215 Node.js: 16.17.1 V8: 10.8.168.25-electron.0 OS: Windows_NT x64 10.0.19044

Operating System

Windows 10

Steps to Reproduce

Setup files as below - note the colours in the main.pkr.hcl file for:

    repo          = var.GITHUB_REPOSITORY
    branch        = var.REF_NAME
    event_type    = var.EVENT_NAME

Expected Behavior

Colours should be correct.

Actual Behavior

Colours incorrect on lines 12/13/14.

image

Configuration

file.pkr.hcl

source "amazon-ebs" "nat-ubuntu" {
  ami_name      = "digital-images-nat-ubuntu-${local.timestamp}"
  source_ami    = data.amazon-ami.nat-ubuntu.id
  instance_type = "t4g.micro"
  region        = "eu-west-1"
  ssh_username  = "ubuntu"
  ami_regions   = var.ami_regions

  tags = {
    base_ami_id   = "{{ .SourceAMI }}"
    base_ami_name = "{{ .SourceAMIName }}"
    repo          = var.GITHUB_REPOSITORY
    branch        = var.REF_NAME
    event_type    = var.EVENT_NAME
    production    = var.REF_NAME == "main" ? "true" : "false"
  }

  vpc_filter {
    filters = {
      "isDefault" : "false"
    }
  }
  subnet_filter {
    filters = {
      "tag:Name" : "primary*"
    }
    random = true
  }

  ami_users = var.accounts
}

variable.pkr.hcl

variable "accounts" {
  type = list(string)

  default = []
}

variable "ami_regions" {
  type = list(string)

  default = [
    "us-east-1",
    "eu-central-1",
    "eu-west-2",
    "sa-east-1",
  ]
}

variable "GITHUB_REPOSITORY" {
  description = "GitHub repo"

  type = string

  default = "Hogarth-Worldwide/terraform-aws-amis-digital"
}

variable "REF_NAME" {
  description = "GitHub branch"

  type = string

  default = "local_running"
}

variable "EVENT_NAME" {
  description = "GitHub event name"

  type = string

  default = "local_running"
}
radeksimko commented 1 year ago

Hi @scott-doyland-burrows Based on the screenshot I'm guessing there are two problems.

With default VS Code configuration, the same snippet of code gets highlighted a bit differently to me:

Screenshot 2023-06-21 at 09 32 48

(note the difference in the first segments of addresses where here none of them are highlighted) This would suggest that you may have configured VS Code such that it treats *.hcl or *.pkr.hcl as terraform language, making the Terraform extension to highlight it. The Terraform extension is designed to only work with Terraform code (i.e. *.tf and *.tfvars), not arbitrary HCL based language. Also if it's configured that way it will likely lead to other kind of problems when attempting to format the code and do things other than just highlighting as all of the functionality is - again - tailored to Terraform.

That said even in my screenshot it looks like something is still off in that the first address segment is highlighted differently when the address is inside a nested block (tags in this case), unless it's part of a conditional expression. That is most certainly a bug we'll need to address.

Thank you for the report.

scott-doyland-burrows commented 1 year ago

You are correct, it was set to use the terraform extension. I was actually flicking between the two extensions to see the difference and mistakenly submitted the screenshot when the terraform extension was in use.

But as you say, something does seem incorrect with the HCL extension - and I see the same as you when I use the correct extension.

image

In your screenshot, the source_ami line shows data in white, is that correct?

radeksimko commented 1 year ago

In your screenshot, the source_ami line shows data in white, is that correct?

Good catch, yes, that could be considered a bug too, which applies to all references - we can look into both at the same time - i.e. no need to file a separate issue for that.

radeksimko commented 1 year ago

I recall some semi-related earlier conversations we had when implementing the highlighting grammar about how this works out together with semantic highlighting which - one may expect - highlights valid references differently to invalid ones (i.e. such that refer to block/attribute that doesn't exist). That is something we need to dig through as part of https://github.com/hashicorp/terraform-ls/issues/1304 but it's mostly affecting Terraform rather than plain HCL. It just happens that we use bulk of the grammar for both HCL and Terraform.

I'm not sure if that issue will impact how we (can) fix this, I'm merely sharing some context on what's likely behind the bug.

radeksimko commented 1 year ago

I transferred the issue to the repository where we host the grammars which will need fixing.

I also did some investigation only to discover that this is a slightly wider problem of how we treat references in general, which may also have something to do with the limitations of TextMate grammars and regex.

https://github.com/hashicorp/syntax/blob/13b5b4f2832a27901f1391ce0264199065c01169/src/_main.yml#L374-L389

^ that is the part of the grammar responsible for detecting references

There's a couple of problems with it:

  1. it only detects reference steps on the 2nd and further positions, never the first one
  2. even though Terraform specifically probably doesn't have such a reference it doesn't detect single-step (practically any identifier which isn't boolean, null, or type declaration like number)

I'm not sure we can fix (2) reliably without going through a lot of pain in crafting complex regular expressions that exclude all the false positives but there should be some way of fixing (1).

I tried changing the expression in a few ways but I always ran into false positive matches because once the leading . becomes optional, a lot of things suddenly match the pattern.

Then there's also something wrong with the way we detect objects, or rather the expressions inside of it (tags = { ... }) which affects references in general and operators like == too.

https://github.com/hashicorp/syntax/blob/13b5b4f2832a27901f1391ce0264199065c01169/src/_main.yml#L248-L299

^ that is the snippet responsible for detecting object expressions, but I did not dig too deep into it to understand what exactly is wrong with it


To summarize, from UX perspective there are three visible issues. I used 2 different themes to visualise it below:

Screenshot 2023-06-23 at 11 33 38 Screenshot 2023-06-23 at 11 33 57
dbanck commented 3 months ago

I fixed issue 3 in #112.

1 & 2 still remain as they are a bit more involved as Radek explained.