this repo has no description
cue/ast/astutil,tools/fix: add cross-file shadowing detection for predeclared identifiers
When Sanitize processes a single file, it cannot know if a predeclared
name like "self" is shadowed by a top-level field in another file of
the same package. This causes issues when fix generates "let X = self"
for value alias conversion - if another file has "self: 42", the "self"
reference should be renamed to "__self".
Add SanitizeFiles to sanitize all files of a package at once, detecting
cross-file shadowing of predeclared identifiers. Pull sanitization out
of the internal file() function so callers choose the appropriate
entrypoint: File() uses single-file Sanitize, Instances() uses
cross-file SanitizeFiles.
Change-Id: Ie7028fa3d05dbbcde405657404c4d82926169758
Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com>
Reviewed-on: https://cue.gerrithub.io/c/cue-lang/cue/+/1231454
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
+241
-5
+3
cue/ast/astutil/resolve.go
+3
cue/ast/astutil/resolve.go
···
79
79
// Value Field Field
80
80
// Pkg nil ImportSpec
81
81
82
+
// TODO: allow passing all files in a package and mark nodes as predeclared
83
+
// identifiers.
84
+
82
85
// Resolve resolves all identifiers in a file, populating [ast.Ident.Node] fields.
83
86
// Unresolved identifiers are recorded in [ast.File.Unresolved].
84
87
// It will not overwrite already resolved identifiers.
+51
-1
cue/ast/astutil/sanitize.go
+51
-1
cue/ast/astutil/sanitize.go
···
30
30
// - change field from foo to "foo" if it isn't referenced, rather than
31
31
// relying on introducing a unique alias.
32
32
33
+
// SanitizeFiles sanitizes all CUE files belonging to a single package,
34
+
// detecting cross-file shadowing of predeclared identifiers.
35
+
func SanitizeFiles(files []*ast.File) error {
36
+
names := make(map[string]bool)
37
+
for _, f := range files {
38
+
for _, d := range f.Decls {
39
+
if x, ok := d.(*ast.Field); ok {
40
+
if name := labelName(x.Label); name != "" {
41
+
names[name] = true
42
+
}
43
+
}
44
+
}
45
+
}
46
+
for _, f := range files {
47
+
if err := sanitize(f, names); err != nil {
48
+
return err
49
+
}
50
+
}
51
+
return nil
52
+
}
53
+
54
+
// labelName returns the name of a label, or "" if it cannot be determined.
55
+
func labelName(label ast.Label) string {
56
+
switch x := label.(type) {
57
+
case *ast.Ident:
58
+
return x.Name
59
+
case *ast.Alias:
60
+
if id, ok := x.Expr.(*ast.Ident); ok {
61
+
return id.Name
62
+
}
63
+
}
64
+
return ""
65
+
}
66
+
33
67
// Sanitize rewrites File f in place to be well-formed after automated
34
68
// construction of an AST.
35
69
//
···
38
72
// - unshadows imports associated with idents
39
73
// - unshadows references for identifiers that were already resolved.
40
74
func Sanitize(f *ast.File) error {
75
+
return sanitize(f, nil)
76
+
}
77
+
78
+
func sanitize(f *ast.File, names map[string]bool) error {
41
79
z := &sanitizer{
42
80
file: f,
43
81
rand: rand.New(rand.NewPCG(123, 456)), // ensure determinism between runs
···
46
84
importMap: map[string]*ast.ImportSpec{},
47
85
referenced: map[ast.Node]bool{},
48
86
altMap: map[ast.Node]string{},
87
+
}
88
+
89
+
for name := range names {
90
+
z.names[name] = true
49
91
}
50
92
51
93
// Gather all names.
···
194
236
195
237
_, _, node := s.lookup(n.Name)
196
238
if node.node == nil {
239
+
if n.IsPredeclared() {
240
+
// Check if the predeclared name is shadowed by a top-level field
241
+
// in another file of the same package.
242
+
if z.names[n.Name] {
243
+
n.Name = "__" + n.Name
244
+
}
245
+
n.Scope = nil
246
+
return true
247
+
}
197
248
spec, ok := n.Node.(*ast.ImportSpec)
198
249
if !ok {
199
250
// Clear node. A reference may have been moved to a different
···
254
305
// declaration. Use the "__"-prefixed form to avoid the shadow.
255
306
if n.IsPredeclared() {
256
307
n.Name = "__" + n.Name
257
-
n.Node = nil
258
308
n.Scope = nil
259
309
return false
260
310
}
+128
cue/ast/astutil/sanitize_test.go
+128
cue/ast/astutil/sanitize_test.go
···
20
20
"cuelang.org/go/cue/ast"
21
21
"cuelang.org/go/cue/ast/astutil"
22
22
"cuelang.org/go/cue/format"
23
+
"cuelang.org/go/cue/token"
23
24
"cuelang.org/go/internal"
24
25
25
26
"github.com/go-quicktest/qt"
···
466
467
for _, tc := range testCases {
467
468
t.Run(tc.desc, func(t *testing.T) {
468
469
err := astutil.Sanitize(tc.file)
470
+
if err != nil {
471
+
t.Fatal(err)
472
+
}
473
+
474
+
b, errs := format.Node(tc.file)
475
+
if errs != nil {
476
+
t.Fatal(errs)
477
+
}
478
+
479
+
got := string(b)
480
+
qt.Assert(t, qt.Equals(got, tc.want))
481
+
})
482
+
}
483
+
}
484
+
485
+
func TestSanitizeCrossFileShadowing(t *testing.T) {
486
+
testCases := []struct {
487
+
desc string
488
+
file *ast.File
489
+
pkgFiles []*ast.File
490
+
want string
491
+
}{{
492
+
desc: "Predeclared self shadowed by top-level field in another file",
493
+
file: func() *ast.File {
494
+
// File with a reference to predeclared "self"
495
+
selfIdent := ast.NewPredeclared("self")
496
+
return &ast.File{Decls: []ast.Decl{
497
+
&ast.Field{
498
+
Label: ast.NewIdent("foo"),
499
+
Value: ast.NewStruct(
500
+
&ast.LetClause{
501
+
Ident: ast.NewIdent("X"),
502
+
Expr: selfIdent,
503
+
},
504
+
ast.NewIdent("a"), ast.NewIdent("X"),
505
+
),
506
+
},
507
+
}}
508
+
}(),
509
+
pkgFiles: []*ast.File{{
510
+
// Another file in the package with top-level "self" field
511
+
Decls: []ast.Decl{
512
+
&ast.Field{
513
+
Label: ast.NewIdent("self"),
514
+
Value: ast.NewLit(token.INT, "42"),
515
+
},
516
+
},
517
+
}},
518
+
want: `foo: {
519
+
let X = __self
520
+
a: X
521
+
}
522
+
`,
523
+
}, {
524
+
desc: "Predeclared self not shadowed when no cross-file conflict",
525
+
file: func() *ast.File {
526
+
selfIdent := ast.NewPredeclared("self")
527
+
return &ast.File{Decls: []ast.Decl{
528
+
&ast.Field{
529
+
Label: ast.NewIdent("foo"),
530
+
Value: ast.NewStruct(
531
+
&ast.LetClause{
532
+
Ident: ast.NewIdent("X"),
533
+
Expr: selfIdent,
534
+
},
535
+
ast.NewIdent("a"), ast.NewIdent("X"),
536
+
),
537
+
},
538
+
}}
539
+
}(),
540
+
pkgFiles: []*ast.File{{
541
+
// Another file without "self" field
542
+
Decls: []ast.Decl{
543
+
&ast.Field{
544
+
Label: ast.NewIdent("other"),
545
+
Value: ast.NewLit(token.INT, "42"),
546
+
},
547
+
},
548
+
}},
549
+
want: `foo: {
550
+
let X = self
551
+
a: X
552
+
}
553
+
`,
554
+
}, {
555
+
desc: "Predeclared self shadowed by aliased field in another file",
556
+
file: func() *ast.File {
557
+
selfIdent := ast.NewPredeclared("self")
558
+
return &ast.File{Decls: []ast.Decl{
559
+
&ast.Field{
560
+
Label: ast.NewIdent("foo"),
561
+
Value: ast.NewStruct(
562
+
&ast.LetClause{
563
+
Ident: ast.NewIdent("X"),
564
+
Expr: selfIdent,
565
+
},
566
+
ast.NewIdent("a"), ast.NewIdent("X"),
567
+
),
568
+
},
569
+
}}
570
+
}(),
571
+
pkgFiles: []*ast.File{{
572
+
// Another file with aliased "self" field (X=self: ...)
573
+
Decls: []ast.Decl{
574
+
&ast.Field{
575
+
Label: &ast.Alias{
576
+
Ident: ast.NewIdent("X"),
577
+
Expr: ast.NewIdent("self"),
578
+
},
579
+
Value: ast.NewLit(token.INT, "42"),
580
+
},
581
+
},
582
+
}},
583
+
want: `foo: {
584
+
let X = __self
585
+
a: X
586
+
}
587
+
`,
588
+
}}
589
+
for _, tc := range testCases {
590
+
t.Run(tc.desc, func(t *testing.T) {
591
+
var err error
592
+
if tc.pkgFiles != nil {
593
+
err = astutil.SanitizeFiles(append(tc.pkgFiles, tc.file))
594
+
} else {
595
+
err = astutil.Sanitize(tc.file)
596
+
}
469
597
if err != nil {
470
598
t.Fatal(err)
471
599
}
+3
-4
tools/fix/fix.go
+3
-4
tools/fix/fix.go
···
87
87
if err != nil {
88
88
panic(err)
89
89
}
90
+
if err := astutil.Sanitize(f); err != nil {
91
+
panic(err)
92
+
}
90
93
return f
91
94
}
92
95
···
204
207
f = simplify(f)
205
208
}
206
209
207
-
err = astutil.Sanitize(f)
208
-
if err != nil {
209
-
return nil, errors.Wrapf(err, token.NoPos, "fix: sanitize failed")
210
-
}
211
210
return f, nil
212
211
}
213
212
+5
tools/fix/fixall.go
+5
tools/fix/fixall.go
···
18
18
"os"
19
19
20
20
"cuelang.org/go/cue/ast"
21
+
"cuelang.org/go/cue/ast/astutil"
21
22
"cuelang.org/go/cue/build"
22
23
"cuelang.org/go/cue/errors"
23
24
"cuelang.org/go/cue/token"
···
75
76
if err != nil {
76
77
return err
77
78
}
79
+
}
80
+
81
+
if err := astutil.SanitizeFiles(b.Files); err != nil {
82
+
return errors.Wrapf(err, token.NoPos, "fix: sanitize failed")
78
83
}
79
84
}
80
85
+51
tools/fix/testdata/crossfile_self.txtar
+51
tools/fix/testdata/crossfile_self.txtar
···
1
+
2
+
#exp:aliasv2
3
+
4
+
-- cue.mod/module.cue --
5
+
module: "test.example"
6
+
7
+
language: version: "v0.15.0"
8
+
9
+
-- a.cue --
10
+
// This file defines a top-level field named "self" that shadows
11
+
// the predeclared "self" identifier.
12
+
package foo
13
+
14
+
self: 42
15
+
-- b.cue --
16
+
// This file uses a value alias that converts to "let X = self".
17
+
// Since "self" is shadowed by a top-level field in a.cue, the
18
+
// generated code should use "__self" instead.
19
+
package foo
20
+
21
+
// Value alias conversion should detect cross-file shadowing
22
+
foo: X={
23
+
i: X.a
24
+
a: 1
25
+
}
26
+
-- out/fixmod --
27
+
--- cue.mod/module.cue
28
+
module: "test.example"
29
+
language: {
30
+
version: "v0.15.0"
31
+
}
32
+
--- a.cue
33
+
// This file defines a top-level field named "self" that shadows
34
+
// the predeclared "self" identifier.
35
+
package foo
36
+
37
+
self: 42
38
+
--- b.cue
39
+
@experiment(aliasv2)
40
+
41
+
// This file uses a value alias that converts to "let X = self".
42
+
// Since "self" is shadowed by a top-level field in a.cue, the
43
+
// generated code should use "__self" instead.
44
+
package foo
45
+
46
+
// Value alias conversion should detect cross-file shadowing
47
+
foo: {
48
+
let X = __self
49
+
i: X.a
50
+
a: 1
51
+
}