hashicorp / hcl2

Former temporary home for experimental new version of HCL
https://github.com/hashicorp/hcl
Mozilla Public License 2.0
373 stars 66 forks source link

hclwrite: Fix heredocs never close during format #122

Closed nozaq closed 4 years ago

nozaq commented 4 years ago

Fix for hashicorp/terraform#21434.

Currently linesForFormat() waits for TokenCHeredoc to appear in a different line after finding TokenOHeredoc. It never happens, however, since scanTokens() does not produce TokenNewline inside heredocs so all tokens between TokenOHeredoc and TokenCHeredoc are put inside the same line. Incorrect eq sign alignment happened because inHeredoc flag never set back to false then the following line.assign calculations were skipped.

codecov-io commented 4 years ago

Codecov Report

Merging #122 into master will decrease coverage by 0.03%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #122      +/-   ##
=========================================
- Coverage   69.03%     69%   -0.04%     
=========================================
  Files          98      98              
  Lines       10318   10307      -11     
=========================================
- Hits         7123    7112      -11     
  Misses       2868    2868              
  Partials      327     327
Impacted Files Coverage Δ
hclwrite/format.go 95.37% <ø> (-0.23%) :arrow_down:

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 984d1e8...82d141c. Read the comment docs.

apparentlymart commented 4 years ago

Thanks @nozaq! This seems plausible to me. Thanks for digging in here and tracking down the mistaken logic.

The handling of heredocs changed several times since initially implemented, so these assumptions have shifted over time and seems like this codepath hadn't caught up with it. Your explanation here does match with my expectations of the latest behavior though, and I can see that it does not change the behavior of the existing test case that this now-redundant heredoc handling was originally added to handle.

apparentlymart commented 4 years ago

Note that Terraform uses vendoring for its dependencies, so to actually address hashicorp/terraform#21434 will also take a vendor update PR in the hashicorp/terraform repository.

The usual procedure for that would be:

go get github.com/hashicorp/hcl2@master
go mod tidy
go mod vendor
git add go.mod go.sum vendor

We plan to land the code in this repository (with some reorganization) into the hashicorp/hcl repository at some time in the next couple months and then update Terraform's imports, but I expect we can complete a merge of this update into Terraform much quicker and so these two changes should not collide.

(For anyone reading this in the future after that repository reorganization is complete, at that point the procedure would include also running an HCL library release and using that tag instead of @master in the above, but we're tracking master here for now to represent that this is a temporary location.)

nozaq commented 4 years ago

@apparentlymart Thanks for the explanation, glad it helped 👍