hcl/hclsyntax: Produce better error message for invalid apostrophe

[radeksimko]

Something that would probably help me in debugging when I (for various reasons) tried parsing the following:

    change "regexp_replace" {
        path = "README.md"

        regexp = '(?m)(^## Requirements\n.+- [Go]([^)]+ )[0-9.]+(^## )''
        replace = "$${1}1.10$${2}"
    }

Before

example.hcl:14,12-13: Invalid expression; Expected the start of an expression, but found an invalid expression token.

After

example.hcl:14,12-13: Invalid expression; Expected the start of an expression, but found an invalid expression token (').

---

I understand omitting unknown tokens from error message may have been intentional - perhaps to avoid potentially messing up the terminal with unsafe escape sequences? If that is the case then feel free to just close the PR.

[codecov-io]

Codecov Report

Merging #33 into master will increase coverage by 0.3%. The diff coverage is 100%.

@@            Coverage Diff            @@
##           master      #33     +/-   ##
=========================================
+ Coverage   67.27%   67.57%   +0.3%     
=========================================
  Files          97       97             
  Lines       10110    10122     +12     
=========================================
+ Hits         6801     6840     +39     
+ Misses       2988     2963     -25     
+ Partials      321      319      -2

Impacted Files	Coverage Δ
hcl/hclsyntax/token_type_string.go	0% <ø> (ø)	:arrow_up:
hcl/hclsyntax/scan_tokens.go	96.1% <100%> (ø)	:arrow_up:
hcl/hclsyntax/token.go	57.03% <100%> (+12.2%)	:arrow_up:
hcl/hclsyntax/parser.go	64.53% <0%> (+1.35%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cb33095...53facbf. Read the comment docs.

[apparentlymart]

The diagnostic messages here are written under the assumption that they'll be presented with a contextual source code snippet, and so they generally don't include direct copies of information in the source code unless it is important to understand the resulting full error message, which would look like this in your case:

I'll concede that in this particular case the underlining indicator on that ' token is pretty subtle and not as prominent as it would ideally be. I expect we'll iterate on different ways to visualize the erroneous source code over time.

Not having to deal with escape sequences is another good argument for not doing this! But I will confess it's not the one I had in mind at the time. (and indeed, we should make sure that the diagnostic message formatter will escape non-printable characters in the source code when it prints out these snippets)

This does unfortunately make things harder when you're not using the complete diagnostic printer, which I believe is the case in your application because you ended up wrapping all the diagnostics up in error values, where the Error function returns a more compact representation that's more consistent with how calling applications tend to expect errors to look.

In Terraform's case we have a helper function that can take a list of error values and type-switch each of them to see if any of them can be unwrapped into something richer than a plain error, including an hcl.Diagnostic. You might like to do something similar in your application and then use NewDiagnosticTextWriter to get the built-in equivalent of Terraform's error formatter (which differs slightly so it can deal with some other error types as well).

[apparentlymart]

The mistake you made here actually seems like something that warrants a specialized error message to tell the user that the language only supports double-quotes. That'll be some good polish to do once we've finished integrating this all into Terraform.

[radeksimko]

The diagnostic messages here are written under the assumption that they'll be presented with a contextual source code snippet

Ah, that makes sense and that error message on the attached screenshot looks much better. I didn't realize the diagnostics package can do all this magic 🙂

The mistake you made here actually seems like something that warrants a specialized error message to tell the user that the language only supports double-quotes. That'll be some good polish to do once we've finished integrating this all into Terraform.

👍

[radeksimko]

I finally got back to this PR and finished it 😅Will you please take another look @apparentlymart ?

One thing I noticed is that you apparently use a different version/distribution of ragel, which may have caused all the changes in the comment format. 😕

• firstly I think we should document ragel as a dependency for go generate and try to describe where/how to install it
• I'd like to find out what's causing the difference between output from my ragel and yours.

[apparentlymart]

The Ragel dependency is indeed kinda annoying, since it's not go get-able and so the only way for everyone to be consistent about a version across multiple platforms would be to build a specific version from source. Naturally that hadn't come up until now because I was the only one updating the scanner, but agreed that we'll need to shore this up in the near future.

It looks like so far I've been using this version:

$ ragel --version
Ragel State Machine Compiler version 6.8 Feb 2013
Copyright (c) 2001-2009 by Adrian Thurston

...which is the version packaged in Ubuntu 16.04. 6.10 is the latest stable release, and is available in all subsequent versions of Ubuntu, and also in Homebrew, so in the short term I think it's reasonable to switch over to that and assume for the moment that we'll all install from our respective package managers to get it and then we can document a process for retrieving and building a specific source package later on, as we get closer to making an initial HCL 2.0.0 release.

For now (assuming you're running 6.10 there), would you mind running go generate on a clean master and opening a PR for that, just so we can do the Ragel version switch separately from the rest? I'll also make sure my copy is updated before I next run go generate.

[radeksimko]

For now (assuming you're running 6.10 there), would you mind running go generate on a clean master and opening a PR for that, just so we can do the Ragel version switch separately from the rest? I'll also make sure my copy is updated before I next run go generate.

Sure, I will do that in a second.

You're right about the version. I just installed latest from Homebrew via brew install ragel.

$ ragel -v
Ragel State Machine Compiler version 6.10 March 2017
Copyright (c) 2001-2009 by Adrian Thurston

[radeksimko]

@apparentlymart PTAL, I just rebased the PR.

[radeksimko]

Oh, you already approved, sorry - I missed that 🙈 😄