[tools] go/analysis/passes/modernize: additional pattern for embedlit
Madeline Kalil (Gerrit)
Madeline Kalil has uploaded the change for review![]( "Open in Gerrit")
Commit message
go/analysis/passes/modernize: additional pattern for embedlit
Modifies the embedlit modernizer to recognize an additional pattern:
t := T{...}
t.x = x
becomes
t := T{... x:x,}
Updates golang/go#77965
Change diff
diff --git a/go/analysis/passes/modernize/embedlit.go b/go/analysis/passes/modernize/embedlit.go
index 090ee4c..c0e410f 100644
--- a/go/analysis/passes/modernize/embedlit.go
+++ b/go/analysis/passes/modernize/embedlit.go
@@ -7,6 +7,7 @@
import (
"fmt"
"go/ast"
+ "go/printer"
"go/types"
"strings"
@@ -52,10 +53,13 @@
names []string // names of the embedded field types that can be removed
compLit = curLit.Node().(*ast.CompositeLit)
compLitType = info.TypeOf(compLit)
- check func(*ast.CompositeLit)
+ checkA func(*ast.CompositeLit)
+ checkB func(inspector.Cursor)
)
- check = func(lit *ast.CompositeLit) {
- for _, elt := range lit.Elts {
+ // Pattern A:
+ // T{U: U{f: v, ...}} => T{f: v, ...}
+ checkA = func(lit *ast.CompositeLit) {
+ for idx, elt := range lit.Elts {
// Can't promote an unkeyed field; would result in a syntax error.
if kv, ok := elt.(*ast.KeyValueExpr); ok {
if innerLit := isEmbeddedFieldLit(info, compLitType, kv); innerLit != nil {
@@ -76,6 +80,12 @@
if !moreiters.Empty(astutil.Comments(file, closingPos, innerLit.Rbrace+1)) {
continue
}
+ end := innerLit.Rbrace + 1
+ // If the current innerLit is empty, and it is not the last element
+ // in the outer lit, we must expand the range to delete the trailing comma.
+ if len(innerLit.Elts) == 0 && idx < len(lit.Elts)-1 {
+ end += 1
+ }
edits = append(edits, []analysis.TextEdit{
// T{U: U{f: v, ...}}
// ----- -
@@ -89,19 +99,146 @@
// white space or commas. Failing to delete trailing commas may
// result in invalid code.
Pos: closingPos,
- End: innerLit.Rbrace + 1,
+ End: end,
},
}...)
names = append(names, kv.Key.(*ast.Ident).Name)
- check(innerLit)
+ checkA(innerLit)
}
}
}
}
+ // Pattern B:
+ // t := T{...}; t.x = x => t := T{..., x: x}
+ // (or var t = ...)
+ // cur is the cursor for a comp lit.
+ checkB = func(cur inspector.Cursor) {
+ var (
+ // Ident for "t" in the assignment.
+ lhs *ast.Ident
+ // The cursor representing the statement that initializes the comp lit "t".
+ // We use its siblings to search for field assignments and verify that
+ // no intervening statements observe "t".
+ curEnclosing inspector.Cursor
+ )
+ compLit := cur.Node().(*ast.CompositeLit)
+ if hasUnkeyed(compLit) {
+ // Promoting additional embedded fields would result in mixing keyed and
+ // unkeyed fields, which isn't allowed.
+ return
+ }
+
+ if cur.ParentEdgeKind() == edge.AssignStmt_Rhs {
+ assign := cur.Parent().Node().(*ast.AssignStmt)
+ lhs = assign.Lhs[cur.ParentEdgeIndex()].(*ast.Ident)
+ if enc, ok := moreiters.First(cur.Enclosing((*ast.AssignStmt)(nil), (*ast.File)(nil))); ok {
+ curEnclosing = enc
+ }
+ } else if cur.ParentEdgeKind() == edge.ValueSpec_Values {
+ spec := cur.Parent().Node().(*ast.ValueSpec)
+ lhs = spec.Names[cur.ParentEdgeIndex()]
+ if enc, ok := moreiters.First(cur.Enclosing((*ast.DeclStmt)(nil), (*ast.File)(nil))); ok {
+ curEnclosing = enc
+ }
+ }
+ if lhs == nil || !curEnclosing.Valid() {
+ return
+ }
+ tObj := info.ObjectOf(lhs)
+ for sib, ok := curEnclosing.NextSibling(); ok; sib, ok = sib.NextSibling() {
+ var assign *ast.AssignStmt
+ // Any assignments of embedded field values must immediately proceed
+ // the struct initialization.
+ // TODO(mkalil): Check if intervening statements actually observe t.
+ if assign, ok = sib.Node().(*ast.AssignStmt); !ok {
+ return
+ }
+ for idx, expr := range assign.Lhs {
+ sel, ok := expr.(*ast.SelectorExpr)
+ if !ok {
+ return
+ }
+ // Verify that sel.X refers to the same object as "t"
+ selXId, ok := sel.X.(*ast.Ident)
+ if !ok {
+ // TODO: handle deeply nested expressions like t.B.x
+ return
+ }
+ obj := info.ObjectOf(selXId)
+ if obj != tObj {
+ return
+ }
+ // If the rhs uses a value of t (e.g. t.x = t.y), don't suggest a fix because
+ // we can't evaluate t.y when constructing the new literal.
+ rhs := assign.Rhs[idx]
+ rhsCur, _ := sib.FindNode(rhs)
+ for c := range rhsCur.Preorder((*ast.SelectorExpr)(nil), (*ast.CallExpr)(nil)) {
+ if sel, ok := c.Node().(*ast.SelectorExpr); ok {
+ if xid, ok := sel.X.(*ast.Ident); ok {
+ if info.ObjectOf(xid) == tObj {
+ return
+ }
+ } else {
+ // TODO(mkalil): handle deeply nested expressions like t.B.x
+ return
+ }
+ } else if _, ok := c.Node().(*ast.CallExpr); ok {
+ // Skip suggesting fixes when the rhs has a call expression, which
+ // could panic. Moving a call that panics into the literal will
+ // change the program's behavior, because thte program will
+ // terminate before the composite literal is constructed.
+ // (I don't think we need typesinternal.NoEffects here, because
+ // the order of effects will be preserved when we append the key value
+ // pairs in the comp lit. We just need to worry about a function call
+ // that panics)
+ return
+ }
+ }
+
+ // TODO(mkalil): When we allow intervening statements that do not
+ // observe t, we will also have to ensure that these assignments do
+ // not use variables declared after the struct lit initialization, as
+ // moving these values into the literal would reference a variable
+ // that is out of scope. For example:
+ // t := T{...}
+ // v := 2
+ // t.x = v // can't move t.x initialization into struct lit because v is declared after
+
+ // Emit edits to move the field assignment into the struct lit and
+ // then remove it from its current place.
+ var rhsBuf strings.Builder
+ if err := printer.Fprint(&rhsBuf, pass.Fset, rhs); err != nil {
+ // bug.Reportf?
+ return
+ }
+ // TODO(mkalil): Are comments preserved?
+ edits = append(edits, []analysis.TextEdit{
+ {
+ // Remove the assignment t.x = x.
+ Pos: assign.Pos(),
+ End: assign.End(),
+ },
+ {
+ // Append the key-value pair to the CompositeLit.
+ Pos: compLit.Rbrace,
+ NewText: fmt.Appendf(nil, ", %s: %s", sel.Sel.Name, rhsBuf.String()),
+ },
+ }...)
+ }
+ }
+ }
+
if curLit.ParentEdgeKind() != edge.KeyValueExpr_Value {
+ // non-nested comp lit
compLit := curLit.Node().(*ast.CompositeLit)
- check(compLit) // non-nested comp lit
+ checkA(compLit)
+ // TODO(mkalil): Figure out how to handle addition/removal of commas
+ // in the comp lit when we observe code where both patterns apply.
+ // For now, only apply edits from one pattern at a time.
+ if len(edits) == 0 {
+ checkB(curLit)
+ }
}
if len(edits) > 0 {
pass.Report(analysis.Diagnostic{
@@ -152,6 +289,7 @@
// type B struct { x int }
// _ = T{A: A{x: 1}}
// cannot be simplified to T{x: 1} because T has two different embedded fields called "x".
+ // We also reject composite literals with slice elements, as parentObj will be nil.
parentObj, _, _ := types.LookupFieldOrMethod(topLevelType, true, obj.Pkg(), k.Name)
if parentObj != obj {
return nil
@@ -173,3 +311,12 @@
}
return obj
}
+
+func hasUnkeyed(lit *ast.CompositeLit) bool {
+ for _, elt := range lit.Elts {
+ if _, ok := elt.(*ast.KeyValueExpr); !ok {
+ return true
+ }
+ }
+ return false
+}
diff --git a/go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go b/go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go
index f79de53..f559e97 100644
--- a/go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go
+++ b/go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go
@@ -2,6 +2,8 @@
package embedlit
+import "log"
+
type A struct {
a int
B
@@ -55,6 +57,16 @@
type K struct{ L }
type L []int
+type T struct {
+ a int
+ b int
+ U
+}
+
+type U struct {
+ x int
+}
+
var (
_ = A{B: B{b: 1}} // want "embedded field type can be removed from struct literal"
_ = A{a: 1, B: B{b: 1, C: C{c: 1}}} // want "embedded field type can be removed from struct literal"
@@ -72,6 +84,8 @@
}
// empty composite lit
_ = A{a: 1, B: B{}} // want "embedded field type can be removed from struct literal"
+ _ = T{U: U{}, b: 1} // want "embedded field type can be removed from struct literal"
+
// don't suggest a fix if it's too tricky to preserve comments
_ = A{ // nope: comments within range to delete
B: B{ // one
@@ -92,4 +106,39 @@
}
_ = A{B: B{b: 1} /* comment */, a: 2} // want "embedded field type can be removed from struct literal"
_ = K{L: L{zero: 0}} // nope: cannot promote slice elements
+ _ = K{L: L{0: 100}} // nope: cannot promote slice elements
)
+
+func _() {
+ t1 := A{a: 1} // want "embedded field type can be removed from struct literal"
+ t1.b = 2
+
+ var t2 A
+ t2 = A{a: 1} // want "embedded field type can be removed from struct literal"
+ t2.b = 2
+
+ var t3 = A{a: 1} // want "embedded field type can be removed from struct literal"
+ t3.b = 2
+
+ t4 := T{1, 2, U{x: 3}} // nope: can't mix keyed and unkeyed elements
+ t4.x = 4
+
+ t5 := A{a: 1}
+ log.Print(t5) // nope: intervening statement observes t5
+ t5.b = 2
+
+ t6 := A{a: 1} // nope: value assigned depends on t6 itself
+ t6.b = t6.a + 1
+
+ t7 := A{a: 1} // nope: assigning a function call, don't know if it will panic
+ t7.b = foo()
+
+ t8 := A{ // want "embedded field type can be removed from struct literal"
+ B: B{b: 1},
+ }
+ t8.a = 2
+}
+
+func foo() int {
+ return 0
+}
diff --git a/go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go.golden b/go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go.golden
index d20da95..353022b 100644
--- a/go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go.golden
+++ b/go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go.golden
@@ -2,6 +2,8 @@
package embedlit
+import "log"
+
type A struct {
a int
B
@@ -55,6 +57,16 @@
type K struct{ L }
type L []int
+type T struct {
+ a int
+ b int
+ U
+}
+
+type U struct {
+ x int
+}
+
var (
_ = A{b: 1} // want "embedded field type can be removed from struct literal"
_ = A{a: 1, b: 1, c: 1} // want "embedded field type can be removed from struct literal"
@@ -71,6 +83,8 @@
}
// empty composite lit
_ = A{a: 1} // want "embedded field type can be removed from struct literal"
+ _ = T{b: 1} // want "embedded field type can be removed from struct literal"
+
// don't suggest a fix if it's too tricky to preserve comments
_ = A{ // nope: comments within range to delete
B: B{ // one
@@ -90,5 +104,37 @@
a: 2,
}
_ = A{b: 1 /* comment */, a: 2} // want "embedded field type can be removed from struct literal"
- _ = K{L: L{zero: 0}} // nope: cannot promote slice elements
+ _ = K{L: L{zero: 0}} // nope: cannot promote slice elements
+ _ = K{L: L{0: 100}} // nope: cannot promote slice elements
)
+
+func _() {
+ t1 := A{a: 1, b: 2} // want "embedded field type can be removed from struct literal"
+
+ var t2 A
+ t2 = A{a: 1, b: 2} // want "embedded field type can be removed from struct literal"
+
+ var t3 = A{a: 1, b: 2} // want "embedded field type can be removed from struct literal"
+
+ t4 := T{1, 2, U{x: 3}} // nope: can't mix keyed and unkeyed elements
+ t4.x = 4
+
+ t5 := A{a: 1}
+ log.Print(t5) // nope: intervening statement observes t5
+ t5.b = 2
+
+ t6 := A{a: 1} // nope: value assigned depends on t6 itself
+ t6.b = t6.a + 1
+
+ t7 := A{a: 1} // nope: assigning a function call, don't know if it will panic
+ t7.b = foo()
+
+ t8 := A{ // want "embedded field type can be removed from struct literal"
+ b: 1,
+ }
+ t8.a = 2
+}
+
+func foo() int {
+ return 0
+}
Change information
- M go/analysis/passes/modernize/embedlit.go
- M go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go
- M go/analysis/passes/modernize/testdata/src/embedlit/embedlit_go127.go.golden
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil added 1 comment![]( "Open in Gerrit")
I left several TODOs so this still a WIP, but it would be great if you could review what I have so far if you have time
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 24 comments![]( "Open in Gerrit")
checkA func(*ast.CompositeLit)
checkB func(inspector.Cursor)Let's declare these as late as possible, otherwise it looks like they might be mutually recursive, but in fact it's just two recursive functions.
// in the outer lit, we must expand the range to delete the trailing comma.Does the logic below assume that the comma immediately follows the brace?
// in the outer lit, we must expand the range to delete the trailing comma.A little picture would help here.
// The cursor representing the statement that initializes the comp lit "t".File or Stmt
but see below. I think File is actually unreachable, so this should be curStmt.
// Ident for "t" in the assignment.
lhs *ast.Ident
// The cursor representing the statement that initializes the comp lit "t".
// We use its siblings to search for field assignments and verify that
// no intervening statements observe "t".
curEnclosing inspector.Cursor
)Move down to L132.
if hasUnkeyed(compLit) {if !moreiters.Every(lit.Elts, is[*ast.KeyValueExpr]) {
lhs = assign.Lhs[cur.ParentEdgeIndex()].(*ast.Ident)How do we know it's an ident? It could be x.y[i] = ...
if enc, ok := moreiters.First(cur.Enclosing((*ast.AssignStmt)(nil), (*ast.File)(nil))); ok {Isn't the first enclosing AssignStmt or File always going to be the Parent?
if enc, ok := moreiters.First(cur.Enclosing((*ast.DeclStmt)(nil), (*ast.File)(nil))); ok {How can it match the File?
}else return
Then you can assume lhs != nil && curEnclosing.Valid.
for sib, ok := curEnclosing.NextSibling(); ok; sib, ok = sib.NextSibling() {Since we don't need curStmt (nee curEnclosing), we might as well update it:
```
for {
var ok bool
curStmt, ok = curStmt.NextSibling()
if ok { return } // end of (e.g.) block
```
var assign *ast.AssignStmtassign, ok := sib.Node().(*ast.AssignStmt)
!ok { return }
for idx, expr := range assign.Lhs {I don't see any testcases for an assignment with multiple Lhs variables. Does it actually work? (It looks like it might.) I wouldn't worry about that case if it's tricky (just stop if len(Lhs) != 1), but either way we should test it.
// TODO: handle deeply nested expressions like t.B.xTODO(mkalil):
for c := range rhsCur.Preorder((*ast.SelectorExpr)(nil), (*ast.CallExpr)(nil)) {Ident
Any reference to t on the RHS is a problem.
// Skip suggesting fixes when the rhs has a call expression, which
// could panic. Moving a call that panics into the literal will
// change the program's behavior, because thte program will
// terminate before the composite literal is constructed.This is true, but I don't think panic is the interesting thing here. If `t = T{...}` expression is populating a local variable then it will disappear during the panic; and if it's a global variable, the panic means you have bigger problems. In general, we can assume that programs shouldn't panic.
There's a concern that any expression with side effects might mutate the values of the expressions that we move up to the literal; but shouldn't the transformed code keep all the `k: v` pairs in the same order, so there's nothing to worry about?
A concrete example of the problem we're trying to solve here would help.
// TODO(mkalil): When we allow intervening statements that do not
// observe t, we will also have to ensure that these assignments doI suggest we don't attempt to do that. Basically, permit t := T{...} immediately followed by one or more t.x = ... assignments, and that's all.
if err := printer.Fprint(&rhsBuf, pass.Fset, rhs); err != nil {Remember that reformatting an expression is liable to lose its comments and vertical space. Is it necessary to reformat? All the expressions are in the correct order, so I think we should be able to replace the glue in between them. Something like this:
```
t := T{...}; t.x = v
----- --- -
t := T{..., x: v}
```
(Obviously spaces and semicolons are a little more fiddly. We should keep the newline if that's how the logical semicolon was written in the source.)
// in the comp lit when we observe code where both patterns apply.Yeah, that's gonna be tricky. I think what you have here is the best we're going to be able to do without heroic effort.
import "log"
Tip: analysis tests run faster if they don't use imports: `go list -deps log` pulls in 61 packages. I suggest you declare a use(any) function instead of log.Print.
}Can we add a test that uses two following statements?
x = T{...}
x.a.b = 123
x.a.c = 456
I suspect only the first will get fixed, and the user will have to run the tool again to get the second one.
t1 := A{a: 1, b: 2} // want "embedded field type can be removed from struct literal"nice
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 23 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
23 comments
checkA func(*ast.CompositeLit)
checkB func(inspector.Cursor)Let's declare these as late as possible, otherwise it looks like they might be mutually recursive, but in fact it's just two recursive functions.
Done
// in the outer lit, we must expand the range to delete the trailing comma.A little picture would help here.
Done
// in the outer lit, we must expand the range to delete the trailing comma.Does the logic below assume that the comma immediately follows the brace?
Yes, I thought this was the case in valid code, is that incorrect?
// The cursor representing the statement that initializes the comp lit "t".File or Stmt
but see below. I think File is actually unreachable, so this should be curStmt.
Changed to curStmt, but see my other comment about why File is reachable
var (
// Ident for "t" in the assignment.
lhs *ast.Ident
// The cursor representing the statement that initializes the comp lit "t".
// We use its siblings to search for field assignments and verify that
// no intervening statements observe "t".
curEnclosing inspector.Cursor
)Madeline KalilMove down to L132.
Done
if !moreiters.Every(lit.Elts, is[*ast.KeyValueExpr]) {
Had to slightly modify to satisfy the params:
```
if !moreiters.Every(slices.Values(compLit.Elts), func(e ast.Expr) bool {
_, ok := e.(*ast.KeyValueExpr)
return ok
})
```
How do we know it's an ident? It could be x.y[i] = ...
Good point, although x.y[i] = T{..} is a bizarre case, will leave a TODO
if enc, ok := moreiters.First(cur.Enclosing((*ast.AssignStmt)(nil), (*ast.File)(nil))); ok {Isn't the first enclosing AssignStmt or File always going to be the Parent?
Oh yes, thanks. In the value spec case we need to search for the enclosing DeclStmt since it won't just be the parent, but we don't need to use this pattern for the normal assignment case.
if enc, ok := moreiters.First(cur.Enclosing((*ast.DeclStmt)(nil), (*ast.File)(nil))); ok {How can it match the File?
A declaration using the blank identifier doesn't have a DeclStmt, the enclosing path is [*ast.CompositeLit *ast.ValueSpec *ast.GenDecl *ast.File]. But I think the *ast.File and the check of lhs == nil and curStmt.IsValid() is redundant, so I will delete the *ast.File and leave the nil checks. (And decls using the blank identifier aren't relevant to pattern B anyways).
else return
Then you can assume lhs != nil && curEnclosing.Valid.
See my other comment: we still need a nil check here
for sib, ok := curEnclosing.NextSibling(); ok; sib, ok = sib.NextSibling() {Since we don't need curStmt (nee curEnclosing), we might as well update it:
```
for {
var ok bool
curStmt, ok = curStmt.NextSibling()
if ok { return } // end of (e.g.) block
```
Done
assign, ok := sib.Node().(*ast.AssignStmt)
!ok { return }
Done
I don't see any testcases for an assignment with multiple Lhs variables. Does it actually work? (It looks like it might.) I wouldn't worry about that case if it's tricky (just stop if len(Lhs) != 1), but either way we should test it.
Oops! Yeah, the edits get trickier when we handle multiple lhs vars. I'll leave this as a TODO for now and will prioritize it if it ends up being a common pattern in std, etc.
// TODO: handle deeply nested expressions like t.B.xMadeline KalilTODO(mkalil):
Done
for c := range rhsCur.Preorder((*ast.SelectorExpr)(nil), (*ast.CallExpr)(nil)) {Ident
Any reference to t on the RHS is a problem.
Done
// Skip suggesting fixes when the rhs has a call expression, which
// could panic. Moving a call that panics into the literal will
// change the program's behavior, because thte program will
// terminate before the composite literal is constructed.This is true, but I don't think panic is the interesting thing here. If `t = T{...}` expression is populating a local variable then it will disappear during the panic; and if it's a global variable, the panic means you have bigger problems. In general, we can assume that programs shouldn't panic.
There's a concern that any expression with side effects might mutate the values of the expressions that we move up to the literal; but shouldn't the transformed code keep all the `k: v` pairs in the same order, so there's nothing to worry about?
A concrete example of the problem we're trying to solve here would help.
This was a Gemini-generated edge case that makes theoretical sense to me:
t = T{...}
t.a = funcThatMightPanic()
Here, t will get initialized but a won't be set.
If we move the assignment up, there is a behavior change:
t = T{..., a: funcThatMightPanic()}
because t won't get initialized. But if we are assuming that programs shouldn't panic, then it doesn't make sense to handle this so I'll delete it. And I agree that expressions with side effects are fine because we preserve the original order of the key value exprs.
// TODO(mkalil): When we allow intervening statements that do not
// observe t, we will also have to ensure that these assignments doI suggest we don't attempt to do that. Basically, permit t := T{...} immediately followed by one or more t.x = ... assignments, and that's all.
Sounds good to me, that makes it easier. Deleted the TODOs
if err := printer.Fprint(&rhsBuf, pass.Fset, rhs); err != nil {Remember that reformatting an expression is liable to lose its comments and vertical space. Is it necessary to reformat? All the expressions are in the correct order, so I think we should be able to replace the glue in between them. Something like this:
```
t := T{...}; t.x = v
----- --- -
t := T{..., x: v}
```(Obviously spaces and semicolons are a little more fiddly. We should keep the newline if that's how the logical semicolon was written in the source.)
Yeah, I just thought it would be less error-prone to paste the formatted rhs rather than do the surgical edits and deal with whitespace. In terms of losing comments, I think this only happens when the rhs itself is a struct literal?
```
t12 := A{}
t12.B = B{
b: 1, // this comment gets deleted
}
```
vs
```
t10 := A{}
t10.a = 1 // this comment is preserved
t10.b = 2 // this one too
```
I'll come back to this but wanted to push the other changes
// in the comp lit when we observe code where both patterns apply.Yeah, that's gonna be tricky. I think what you have here is the best we're going to be able to do without heroic effort.
Acknowledged
Tip: analysis tests run faster if they don't use imports: `go list -deps log` pulls in 61 packages. I suggest you declare a use(any) function instead of log.Print.
Done, thanks.
Can we add a test that uses two following statements?
x = T{...}
x.a.b = 123
x.a.c = 456I suspect only the first will get fixed, and the user will have to run the tool again to get the second one.
I have an outstanding TODO for handling deeply nested exprs, but I added a test case for something like:
x = T{...}
x.b = 1
x.c = 2
and it produces:
x = T{..., b: 1, c: 2}
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 2 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
2 comments
// in the outer lit, we must expand the range to delete the trailing comma.Does the logic below assume that the comma immediately follows the brace?
Yes, I thought this was the case in valid code, is that incorrect?
Nevermind, commas/whitespace are valid. But we only need this logic in order to handle empty composite lits, and since there's no fields in them to promote, although they can be removed entirely, I don't think we need to handle them at all.
if err := printer.Fprint(&rhsBuf, pass.Fset, rhs); err != nil {Madeline KalilRemember that reformatting an expression is liable to lose its comments and vertical space. Is it necessary to reformat? All the expressions are in the correct order, so I think we should be able to replace the glue in between them. Something like this:
```
t := T{...}; t.x = v
----- --- -
t := T{..., x: v}
```(Obviously spaces and semicolons are a little more fiddly. We should keep the newline if that's how the logical semicolon was written in the source.)
Yeah, I just thought it would be less error-prone to paste the formatted rhs rather than do the surgical edits and deal with whitespace. In terms of losing comments, I think this only happens when the rhs itself is a struct literal?
```
t12 := A{}
t12.B = B{
b: 1, // this comment gets deleted
}
```
vs
```
t10 := A{}
t10.a = 1 // this comment is preserved
t10.b = 2 // this one too
```
I'll come back to this but wanted to push the other changes
Updated to use surgical edits instead. We have to gather all the assign statements that will be promoted first, and then produce the edits at the end.
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 7 comments![]( "Open in Gerrit")
var (
edits []analysis.TextEdit
names []string // names of the embedded field types that can be removed
compLit = curLit.Node().(*ast.CompositeLit)
compLitType = info.TypeOf(compLit)
)checkA and B are quite long, and are almost completely independent from each other and from their parent function (runEmbedLit); the only things they share are these locals. I think it would be worth breaking the two functions into two top-level functions that each call pass.Report as their last step.
_, ok := e.(*ast.KeyValueExpr)
return ok`return is[*ast.KeyValueExpr](e)`
if cur.ParentEdgeKind() == edge.AssignStmt_Rhs {```
switch cur.ParentEdgeKind() { case A: case B: default: }
```
avoids evaluating it twice.
type assignInfo struct {
assign *ast.AssignStmt
sel *ast.SelectorExpr
rhs ast.Expr
}Rather than allocate a new array of assignInfos for the sequence of statements after every keyed composite literal, you could record the existing statement slice itself (e.g. a `[]Stmt` portion of `BlockStmt.List`, or a pair of Cursors for the first and last statement).
It's easy to derive the sel (`assign.Lhs[0].(*ast.SelectorExpr)`) and rhs (`assign.Rhs[0]`) fields later.
if id, ok := c.Node().(*ast.Ident); ok {You can safely assume this because of the type filter passed to Preorder.
src, err := os.ReadFile(filename)As a general note, it is rarely correct to discard I/O error messages as I/O can fail for a huge variety of reasons that you might want to know about. Better to propagate the error upwards, which in this case means to return it from the error from the Analyzer.run function. This will cause analysis to fail, but that's appropriate, since it should never happen.
Specifically, an analyzer should never read the file system directly. There's no guarantee that what it reads from the file system matches the parse tree: the tree could have been derived from an edited file via the overlay mechanism; or the file might not exist at all (as in the case of a purely virtualized analysis processing RPC service). Indeed, in the future, the secure analysis sandbox will directly reject attempt to read the file system.
Instead, Analyzers should call pass.ReadFile to read the contents of one of the files under analysis, at the appropriate version.
(It's unfortunate that we have to use ReadFile at all--it's a symptom of design problems in the AST--but I think it should be efficient since at this point we committed to reporting a diagnostic.)
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 7 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
edits []analysis.TextEdit
names []string // names of the embedded field types that can be removed
compLit = curLit.Node().(*ast.CompositeLit)
compLitType = info.TypeOf(compLit)
)checkA and B are quite long, and are almost completely independent from each other and from their parent function (runEmbedLit); the only things they share are these locals. I think it would be worth breaking the two functions into two top-level functions that each call pass.Report as their last step.
Yeah that's better, thanks.
_, ok := e.(*ast.KeyValueExpr)
return okMadeline Kalil`return is[*ast.KeyValueExpr](e)`
Done
```
switch cur.ParentEdgeKind() { case A: case B: default: }
```
avoids evaluating it twice.
Done
type assignInfo struct {
assign *ast.AssignStmt
sel *ast.SelectorExpr
rhs ast.Expr
}Rather than allocate a new array of assignInfos for the sequence of statements after every keyed composite literal, you could record the existing statement slice itself (e.g. a `[]Stmt` portion of `BlockStmt.List`, or a pair of Cursors for the first and last statement).
It's easy to derive the sel (`assign.Lhs[0].(*ast.SelectorExpr)`) and rhs (`assign.Rhs[0]`) fields later.
Used a pair of cursors for firstStmt and lastStmt
You can safely assume this because of the type filter passed to Preorder.
Done
As a general note, it is rarely correct to discard I/O error messages as I/O can fail for a huge variety of reasons that you might want to know about. Better to propagate the error upwards, which in this case means to return it from the error from the Analyzer.run function. This will cause analysis to fail, but that's appropriate, since it should never happen.
Specifically, an analyzer should never read the file system directly. There's no guarantee that what it reads from the file system matches the parse tree: the tree could have been derived from an edited file via the overlay mechanism; or the file might not exist at all (as in the case of a purely virtualized analysis processing RPC service). Indeed, in the future, the secure analysis sandbox will directly reject attempt to read the file system.
Instead, Analyzers should call pass.ReadFile to read the contents of one of the files under analysis, at the appropriate version.
(It's unfortunate that we have to use ReadFile at all--it's a symptom of design problems in the AST--but I think it should be efficient since at this point we committed to reporting a diagnostic.)
Thank you! I didn't know about pass.ReadFile. Indeed it is unfortunate that we have to read the file but I'm not sure there's a way to handle the comma issue without looking at the actual file content (at least I couldn't think of one).
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil added 1 comment![]( "Open in Gerrit")
Results from this morning of running this modernizer over std: https://go-review.git.corp.google.com/c/go/+/776862
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 16 comments![]( "Open in Gerrit")
func checkA(pass *analysis.Pass, info *types.Info, curLit inspector.Cursor) bool {Now that these share the modernize package namespace, let's give them slightly more scoped and informative names, e.g. embedlitUnnest and embedlitCombine.
checkLit = func(pass *analysis.Pass, info *types.Info, curLit inspector.Cursor, lit *ast.CompositeLit, litType types.Type) {Delete these three parameters and litType; they're all already in scope.
if innerLit := isEmbeddedFieldLit(info, litType, kv); innerLit != nil {complitType
curStmt, ok = curStmt.NextSibling()var ok bool
then delete L187
src, err := os.ReadFile(filename)Madeline KalilAs a general note, it is rarely correct to discard I/O error messages as I/O can fail for a huge variety of reasons that you might want to know about. Better to propagate the error upwards, which in this case means to return it from the error from the Analyzer.run function. This will cause analysis to fail, but that's appropriate, since it should never happen.
Specifically, an analyzer should never read the file system directly. There's no guarantee that what it reads from the file system matches the parse tree: the tree could have been derived from an edited file via the overlay mechanism; or the file might not exist at all (as in the case of a purely virtualized analysis processing RPC service). Indeed, in the future, the secure analysis sandbox will directly reject attempt to read the file system.
Instead, Analyzers should call pass.ReadFile to read the contents of one of the files under analysis, at the appropriate version.
(It's unfortunate that we have to use ReadFile at all--it's a symptom of design problems in the AST--but I think it should be efficient since at this point we committed to reporting a diagnostic.)
Thank you! I didn't know about pass.ReadFile. Indeed it is unfortunate that we have to read the file but I'm not sure there's a way to handle the comma issue without looking at the actual file content (at least I couldn't think of one).
Don't forget to handle the error.
rhsCur, _ := curStmt.FindNode(rhs)No need for Find: you can use `curRhs := curStmt.ChildAt(edge.Assign_Rhs, 0)`.
rhsUsesT := false
for c := range rhsCur.Preorder((*ast.Ident)(nil)) {
id := c.Node().(*ast.Ident)
// If the rhs uses a value of t (e.g. t.x = t.y), don't suggest a fix because
// we can't evaluate t.y when constructing the new literal.
if info.ObjectOf(id) == tObj {
rhsUsesT = true
break
}
// Note: we don't need to worry about expressions with side effects
// changing the behavior when moved inside the comp lit. The order of
// effects will be preserved because we preserve the order of the key
// value pairs inside the comp lit.
}
if rhsUsesT {
break
}BTW, you could replace this whole loop with `if uses(curRhs, index, tObj) { break } ` if you Require typeindex.
(Although this function does iterate over all the uses of t, it is very efficient and doesn't allocate.)
if src[i] == ',' {Is it possible this might be triggered by a comma in a comment?
for i := lastEltOffset; i < rbraceOffset && i < len(src); i++ {
if src[i] == ',' {
hasTrailingComma = true
break
}
}hasTrailingComma = bytes.ContainsByte(src[lastEltOffset:rbraceOffset], ',')
// Replace "=" with ":"Can you add a test case that uses += to make sure we leave it alone?
Pos: assign.TokPos,Shouldn't this be expr.End(), so that `t.x = y` becomes `x: t` without a space?
if len(edits) > 0 {delete (we can't reach here without getting edits)
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |