Cue fmt destroys working code #2567

vtomilov · 2023-08-24T15:18:03Z

What version of CUE are you using (`cue version`)?

$ cue version
cue version v0.6.0

go version go1.20.6
      -buildmode exe
       -compiler gc
       -trimpath true
     CGO_ENABLED 0
          GOARCH arm64
            GOOS darwin

Does this issue reproduce with the latest stable release?

Yes

What did you do?

Apply cue fmt on code like this

foo: [
	bar["baz"], //some comment
]

What did you expect to see?

Code should not change, or some whitespace changes

What did you see instead?

Code that doesn't compile

foo: [
	bar["baz" //some comment],
]

The text was updated successfully, but these errors were encountered:

mvdan · 2023-11-08T09:15:54Z

Here is another test case, found by @rudifa in #2646:

exec cue fmt in.cue

# Show what the CUE looks like after formatting.
exec cat in.cue

# Show that the resulting CUE is broken.
exec cue eval in.cue

-- in.cue --
[
	if true // inline comment
	{}]

$ testscript -v repro-fmt.txtar 
[...]
> exec cue fmt in.cue
# Show what the CUE looks like after formatting. (0.001s)
> exec cat in.cue
[stdout]
[
	if true {} // inline comment]
# Show that the resulting CUE is broken. (0.013s)
> exec cue eval in.cue
[stderr]
expected ']', found 'EOF':
    ./in.cue:2:32
[exit status 1]
FAIL: /tmp/testscript450645547/repro-fmt.txtar/script.txtar:7: unexpected command failure
error running repro-fmt.txtar in /tmp/testscript450645547/repro-fmt.txtar

rudifa · 2023-11-11T21:25:20Z

EDITED:

Actually, the "working code" above is not valid cue.
To what extent should cue fmt format invalid cue, if at all?

The "working code" above is valid cue, only it fails vet and eval.

Here is a testscript with working code that passes vet and eval before fmt; cue fmt reveals the same bug:

# 2267-2.txtar
exec cat 2567.cue
exec cue vet 2567.cue   ### vet passes
exec cue fmt 2567.cue   ### fmt misplaces the comment inside the brackets
exec cat 2567.cue
exec cue vet 2567.cue   ### vet fails

-- 2567.cue --
foo: [
	bar["baz"], //some comment
]
bar: baz: 41

rudifa · 2023-11-14T18:41:34Z

Here is the minimal comprehension example, with its DebugStr(f)

cue % cat testdata/2567-2.cue                                       [issues-fmt-comments L|…1]
[
	if true // inline comment
	{}]

cue % cat testdata/2567-2.cue | go run . fmt -                      [issues-fmt-comments L|…1]
[
	if true {} // inline comment]

DebugStr: [<[l2// inline comment] if true {}>]

Re above use of DebugStr, see my comments in #1447.

mvdan · 2023-12-20T09:21:59Z

I've sent https://review.gerrithub.io/c/cue-lang/cue/+/1173870 as a first fix, to ensure that newlines are always printed in the formatter.

This fixes Rudi's latest reproducer, but the original reproducer with an index expression still fails. I think that might be a parser bug, so I've filed #2738.

rudifa · 2023-12-27T13:44:31Z

The proposed change 1173870 fixes the missing newline at the end of a comment, and issue #2738 proposes to allow a comma inside the index expressions.

However, we need to solve yet another problem, that of comments misplaced with respect to elements they describe. Take this reproducer:

# script 2567-misplaced-comments.txtar
# state with cue v0.7.0
# the .got.cue files show the bad results of cue fmt

exec cue fmt 2567.cue
cmp 2567.cue 2567.got.cue
! exec cue vet 2567.cue         # result fails the cue vet

exec cue fmt 2567.1.2.3.cue
cmp 2567.1.2.3.cue 2567.1.2.3.got.cue
exec cue vet 2567.1.2.3.cue     # result passes the cue vet, but 2 comments are misplaced

-- 2567.cue --
foo: [
    bar[baz], // about the element `bar[baz]`
]
baz: 41
-- 2567.got.cue --
foo: [
	bar[baz // about the element `bar[baz]`],
]
baz: 41
-- 2567.1.2.3.cue --
[
    // about this clause
    if true // about this condition
        {} // about this value
]
-- 2567.1.2.3.got.cue --
[
	// about this clause
	if true {} // about this condition, // about this value
]
-- end --

Clearly, the misplaced comments don't express what the author of the cue fragments intended.

In fact, I believe that in these examples the formater should reproduce the input cue without any alterations.

When above reproducer is run with cue built with change 1173870 the updated results still do not correspond to what the author intended:

testdata % git diff                                                                                                            [more-tests L|✚1]
diff --git a/testdata/2567-misplaced-comments.txtar b/testdata/2567-misplaced-comments.txtar
index a6be0d1..a1b4f97 100644
--- a/testdata/2567-misplaced-comments.txtar
+++ b/testdata/2567-misplaced-comments.txtar
@@ -17,7 +17,8 @@ foo: [
 baz: 41
 -- 2567.got.cue --
 foo: [
-       bar[baz // about the element `bar[baz]`],
+       bar[baz // about the element `bar[baz]`
+       ],
 ]
 baz: 41
 -- 2567.1.2.3.cue --
@@ -29,6 +30,7 @@ baz: 41
 -- 2567.1.2.3.got.cue --
 [
        // about this clause
-       if true {} // about this condition, // about this value
+       if true {} // about this condition,
+       // about this value
 ]
 -- end --

I believe that these Line comments are misplaced after formatting because the parser did not attach them to the node that they refer to. In fact, the cue-ast-print for the second case shows that all 3 comments, Doc or Line, are attached to the whole clause, and this is exactly what the formatter reproduces (with the known defects).

-- 2567.1.2.3.ast --
*ast.File{
. Filename: "2567.1.2.3.cue"
. Decls: []ast.Decl{
. . *ast.EmbedDecl{
. . . Expr: *ast.ListLit{
. . . . Lbrack: token.Pos("2567.1.2.3.cue:1:1")
. . . . Elts: []ast.Expr{
. . . . . *ast.Comprehension{
. . . . . . Clauses: []ast.Clause{
. . . . . . . *ast.IfClause{
. . . . . . . . If: token.Pos("2567.1.2.3.cue:3:5")
. . . . . . . . Condition: *ast.BasicLit{
. . . . . . . . . ValuePos: token.Pos("2567.1.2.3.cue:3:8")
. . . . . . . . . Kind: token.Token("true")
. . . . . . . . . Value: "true"
. . . . . . . . }
. . . . . . . }
. . . . . . }
. . . . . . Value: *ast.StructLit{
. . . . . . . Lbrace: token.Pos("2567.1.2.3.cue:4:9")
. . . . . . . Elts: []ast.Decl{
. . . . . . . }
. . . . . . . Rbrace: token.Pos("2567.1.2.3.cue:4:10")
. . . . . . }
. . . . . . Comments: []*ast.CommentGroup{
. . . . . . . *ast.CommentGroup{
. . . . . . . . Doc: true
. . . . . . . . Line: false
. . . . . . . . Position: 0
. . . . . . . . List: []*ast.Comment{
. . . . . . . . . *ast.Comment{
. . . . . . . . . . Slash: token.Pos("2567.1.2.3.cue:2:5")
. . . . . . . . . . Text: "// about this clause"
. . . . . . . . . }
. . . . . . . . }
. . . . . . . }
. . . . . . . *ast.CommentGroup{
. . . . . . . . Doc: false
. . . . . . . . Line: true
. . . . . . . . Position: 2
. . . . . . . . List: []*ast.Comment{
. . . . . . . . . *ast.Comment{
. . . . . . . . . . Slash: token.Pos("2567.1.2.3.cue:3:13")
. . . . . . . . . . Text: "// about this condition"
. . . . . . . . . }
. . . . . . . . }
. . . . . . . }
. . . . . . . *ast.CommentGroup{
. . . . . . . . Doc: false
. . . . . . . . Line: true
. . . . . . . . Position: 4
. . . . . . . . List: []*ast.Comment{
. . . . . . . . . *ast.Comment{
. . . . . . . . . . Slash: token.Pos("2567.1.2.3.cue:4:12")
. . . . . . . . . . Text: "// about this value"
. . . . . . . . . }
. . . . . . . . }
. . . . . . . }
. . . . . . }
. . . . . }
. . . . }
. . . . Rbrack: token.Pos("2567.1.2.3.cue:5:1")
. . . }
. . }
. }
. Imports: []*ast.ImportSpec{
. }
}

This is similar to the nooverride whiteSpace flag, but only enforces the newline whitespace token to be printed. Note that https://cuelang.org/issue/2567 is not fully fixed, as the original reproducer now runs into https://cuelang.org/issue/2738. Updates cue-lang#2567. Signed-off-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: I85ec0cc40487fdfc90288f075a004dd2e3826c51

rudifa · 2024-01-18T22:29:42Z

Here is another reproducer:

# script 2567-misplaced-comments.slice.txtar
# state with cue v0.7.0
# the .got.cue file shows the bad results of cue fmt

exec cue fmt 2567.slice.cue
cmp 2567.slice.cue 2567.slice.got.cue
! exec cue vet 2567.slice.cue         # result fails the cue vet


-- 2567.slice.cue --
a: [1,2,3,4,5]
b:[
    a[2:3],  // first slice
    a[:2],   // second slice
    a[2:],   // third slice
]
-- 2567.slice.got.cue --
a: [1, 2, 3, 4, 5]
b: [
	a[2 // first slice
	:3],
	a[:2 // second slice],
	a[2  // third slice
	:],
]
-- end --

The debugStr

<[d0// 2567.slice.cue] a: [1, 2, 3, 4, 5]>, b: [<[l3// first slice] a[2:3]>, <[l3// second slice] a[:2]>, <[l3// third slice] a[2:]>]

shows that the parser gave to all 3 comments Position: 3, which is wrong in all 3 cases.

When list elements are formatted in cue/format, the algorithm prints each element followed by a comment. In case an element has associated comments, they are printed as part of the element. This causes an issue when the comments appear after the element, because the comma will be printed at the end of the comment, and will ulimately be considered part of the comment, causing syntax errors. For example, given the following valid example: list: [ a.b, // selector (a), // paren +1, // unary a[0], // index strings.Sort([1, 2]), // call a.b, // under selector a.b, // multiple // under // selector ] The current formatting logic will produce this: list: [ a.b // selector, (a // paren), +1 // unary, a[0 // index], strings.Sort([1, 2], // call), a.b // under selector, a.b // multiple // under // selector, ] To resolve this, all comments that appear after an element are temporarily disassociated from the element, so that the list formatting section can insert the comma in the correct location. The solution could be cleaner, but a larger refactor is avoided since an overhaul of cue/format is planned. Fixes #2274 Updates #2567 Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Change-Id: Icbbb10800ed3f04bd5adc658b883fa259844b7e9

When list elements are formatted in cue/format, the algorithm prints each element followed by a comma. In case an element has associated comments, they are printed as part of the element. This causes an issue when the comments appear after the element, because the comma will be printed at the end of the comment, and will ulimately be considered part of the comment, causing syntax errors. For example, given the following valid example: list: [ a.b, // selector (a), // paren +1, // unary a[0], // index strings.Sort([1, 2]), // call a.b, // under selector a.b, // multiple // under // selector ] The current formatting logic will produce this: list: [ a.b // selector, (a // paren), +1 // unary, a[0 // index], strings.Sort([1, 2], // call), a.b // under selector, a.b // multiple // under // selector, ] To resolve this, all comments that appear after an element are temporarily disassociated from the element, so that the list formatting section can insert the comma in the correct location. The solution could be cleaner, but a larger refactor is avoided since an overhaul of cue/format is planned. Fixes #2274 Updates #2567 Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Change-Id: Icbbb10800ed3f04bd5adc658b883fa259844b7e9

When list elements are formatted in cue/format, the algorithm prints each element followed by a comma. In case an element has associated comments, they are printed as part of the element. This causes an issue when the comments appear after the element, because the comma will be printed at the end of the comment, and will ulimately be considered part of the comment, causing syntax errors. For example, given the following valid example: list: [ a.b, // selector (a), // paren +1, // unary a[0], // index strings.Sort([1, 2]), // call a.b, // under selector a.b, // multiple // under // selector ] The current formatting logic will produce this: list: [ a.b // selector, (a // paren), +1 // unary, a[0 // index], strings.Sort([1, 2], // call), a.b // under selector, a.b // multiple // under // selector, ] To resolve this, all comments that appear after an element are temporarily disassociated from the element, so that the list formatting section can insert the comma in the correct location. The solution could be cleaner, but a larger refactor is avoided since an overhaul of cue/format is planned. Fixes #2274 Updates #2567 Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Change-Id: Icbbb10800ed3f04bd5adc658b883fa259844b7e9 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1193585 Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com> TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>

NoamTD · 2024-05-31T11:35:13Z

@mvdan I believe this can be closed now

mvdan · 2024-05-31T11:36:21Z

@NoamTD just to confirm, did you double check that all the examples above in this thread output valid CUE now?

NoamTD · 2024-05-31T11:44:38Z

Ah, I thought I did but there's a single case left:

[
	if true // inline comment
	{}]

this still results in

[
	if true {}, // inline comment]

I'll fix that up and then this can be closed

When an if comprehension is placed in a list like so: [ if true // a comment {}] cue fmt will generate invalid cue: [ if true {}, // a comment ] The comment is placed on the same line as the closing bracket, causing the bracket to be lost. To fix this we place the bracket on a new line in such cases. Fixes #2567. Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Change-Id: Ie504380677b12f3d879920a88ed8f02b7b3d6035

This is similar to the nooverride whiteSpace flag, but only enforces the newline whitespace token to be printed. Note that https://cuelang.org/issue/2567 is not fully fixed, as the original reproducer now runs into https://cuelang.org/issue/2738. Updates #2567. Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Co-authored-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: I83bebcca9aeba14f84cbcf4c4befa0677da3fb0f

This is similar to the nooverride whiteSpace flag, but only enforces the newline whitespace token to be printed. Note that https://cuelang.org/issue/2567 is not fully fixed, as the original reproducer now runs into https://cuelang.org/issue/2738. Fixes #2567. Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Co-authored-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: I83bebcca9aeba14f84cbcf4c4befa0677da3fb0f

This is similar to the nooverride whiteSpace flag, but only enforces the newline whitespace token to be printed. Fixes #2567. Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Co-authored-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: I83bebcca9aeba14f84cbcf4c4befa0677da3fb0f

This is similar to the nooverride whiteSpace flag, but only enforces the newline whitespace token to be printed. Fixes #2567. Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Signed-off-by: Daniel Martí <mvdan@mvdan.cc> Co-authored-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: I83bebcca9aeba14f84cbcf4c4befa0677da3fb0f

This is similar to the nooverride whiteSpace flag, but only enforces the newline whitespace token to be printed. Fixes #2567. Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Co-authored-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: I83bebcca9aeba14f84cbcf4c4befa0677da3fb0f

This is similar to the nooverride whiteSpace flag, but only enforces the newline whitespace token to be printed. Fixes #2567. Co-authored-by: Daniel Martí <mvdan@mvdan.cc> Co-authored-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Signed-off-by: Noam Dolovich <noam.tzvi.dolovich@gmail.com> Signed-off-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: I83bebcca9aeba14f84cbcf4c4befa0677da3fb0f

Change has been successfully cherry-picked as 9bd3e17 1 is the latest approved patch-set. The change was submitted with unreviewed changes in the following files: ``` The name of the file: cue/format/node.go Insertions: 1, Deletions: 1. @@ -157,7 +157,7 @@ f.print(f.current.parentSep) } if ellipsis != nil { - // ensure that comments associated with original ellipsis are preserved + // ensure that comments associated with the original ellipsis are preserved n := &ast.Ellipsis{} ast.SetComments(n, ast.Comments(ellipsis)) f.decl(n) ``` ``` The name of the file: cue/format/testdata/comments.golden Insertions: 14, Deletions: 0. @@ -136,7 +136,21 @@ // comment including some tabs +// issue #2567 +foo: [ + bar["baz"], //some comment +] + +[ + if true {}, // inline comment +] + { + + foo: { + // some + ... // comment + } // comments ... // surrounding ``` ``` The name of the file: cue/format/testdata/comments.input Insertions: 15, Deletions: 0. @@ -146,9 +146,24 @@ // comment including some tabs + +// issue #2567 +foo: [ + bar["baz"], //some comment +] + +[ + if true // inline comment + {}] + { // comments ... // surrounding // an ellipsis + + foo: { + // some + [string]: _ // comment + } } ``` Patch-set: 4 Subject: cue/format: preserve comments associated with ellipsis Status: merged Commit: 9bd3e17 Tag: autogenerated:gerrit:merged Groups: f8d7609 Label: Code-Review=+2, bf3417339b0b9a28e5f9ba7cbeb6ff776065d2ad Label: TryBot-Result=+1, 15f76bc677cb3507a9007c145115e658f96390b2 Gerrit User 1017882 <1017882@d5d70762-12d0-45a1-890d-524b12d3f735> Label: SUBM=+1, fbe6e8de5e19567b2b7d473a950b1724b7fb0607 Submission-id: 1195884 Submitted-with: OK Submitted-with: Rule-Name: gerrit~DefaultSubmitRule Submitted-with: MAY: Code-Review: Gerrit User 1017720 <1017720@d5d70762-12d0-45a1-890d-524b12d3f735> Submitted-with: MAY: Unity-Result Submitted-with: MAY: Trigger-Unity-New Submitted-with: MAY: Unity-New-Result Submitted-with: MAY: TryBot-Result: Gerrit User 1017882 <1017882@d5d70762-12d0-45a1-890d-524b12d3f735> Submitted-with: MAY: Hold

vtomilov added NeedsInvestigation Triage Requires triage/attention labels Aug 24, 2023

mvdan added NeedsFix fmt Related to formatting functionality. and removed NeedsInvestigation Triage Requires triage/attention labels Aug 24, 2023

mvdan added this to the fmt-redesign milestone Aug 24, 2023

mvdan added the good first issue Good for newcomers label Oct 23, 2023

rudifa mentioned this issue Nov 2, 2023

cmd/cue: def sometimes inserts CUE comments for errors #2646

Open

mvdan mentioned this issue Nov 8, 2023

cmd/fmt: invalid CUE emitted when all fields are commented inside struct #2423

Open

mvdan removed this from the fmt-redesign milestone Mar 19, 2024

mvdan removed the good first issue Good for newcomers label May 31, 2024

cueckoo closed this as completed in b423a27 Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cue fmt destroys working code #2567

Cue fmt destroys working code #2567

vtomilov commented Aug 24, 2023

mvdan commented Nov 8, 2023

rudifa commented Nov 11, 2023 •

edited

rudifa commented Nov 14, 2023

mvdan commented Dec 20, 2023

rudifa commented Dec 27, 2023

rudifa commented Jan 18, 2024 •

edited

NoamTD commented May 31, 2024

mvdan commented May 31, 2024

NoamTD commented May 31, 2024

Cue fmt destroys working code #2567

Cue fmt destroys working code #2567

Comments

vtomilov commented Aug 24, 2023

What version of CUE are you using (cue version)?

Does this issue reproduce with the latest stable release?

What did you do?

What did you expect to see?

What did you see instead?

mvdan commented Nov 8, 2023

rudifa commented Nov 11, 2023 • edited

rudifa commented Nov 14, 2023

mvdan commented Dec 20, 2023

rudifa commented Dec 27, 2023

rudifa commented Jan 18, 2024 • edited

NoamTD commented May 31, 2024

mvdan commented May 31, 2024

NoamTD commented May 31, 2024

What version of CUE are you using (`cue version`)?

rudifa commented Nov 11, 2023 •

edited

rudifa commented Jan 18, 2024 •

edited