Skip to content

Commit 5c202af

Browse files
authored
Ignore idents for constants, types, packages and nil (#201)
* Ignore idents for constants, types, packages and nil There are a lot of nodes that holds an `*ast.Ident` that's not a name that's declared by the user in the code. This can be things like constants such as `true` or `false`, or just a type name such as `string` in the statement `var a string`. We never want to treat them as idents when looking for intersection because they're not related to the code written by the user. In this example: ```go var z string defer func(x string) {}("") ``` There are no intersections, only `z` on 1st line and `x` on 2nd line, but faulty identifying `string` on both lines result in a false negative where a non existing intersection is found. We now make use of the types info we get from the analysis pass and ignore type names, constants, nil and package names. Fixes #199 * Fix newly discovered violation
1 parent 97cc51e commit 5c202af

File tree

3 files changed

+142
-22
lines changed

3 files changed

+142
-22
lines changed
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
package testpkg
2+
3+
import "fmt"
4+
5+
func dontMatchTypeNames() {
6+
var z string
7+
defer func(x string) { // want `missing whitespace above this line \(no shared variables above defer\)`
8+
fmt.Printf("%v", x)
9+
}("")
10+
11+
var a any = nil
12+
defer func(x any) { // want `missing whitespace above this line \(no shared variables above defer\)`
13+
fmt.Printf("%v", x)
14+
}(nil)
15+
16+
b := true
17+
defer func(x any) { // want `missing whitespace above this line \(no shared variables above defer\)`
18+
fmt.Printf("%v", x)
19+
}(true)
20+
21+
c := false
22+
defer func(x any) { // want `missing whitespace above this line \(no shared variables above defer\)`
23+
fmt.Printf("%v", x)
24+
}(false)
25+
26+
_, _, _, _ = a, b, c, z
27+
}
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package testpkg
2+
3+
import "fmt"
4+
5+
func dontMatchTypeNames() {
6+
var z string
7+
8+
defer func(x string) { // want `missing whitespace above this line \(no shared variables above defer\)`
9+
fmt.Printf("%v", x)
10+
}("")
11+
12+
var a any = nil
13+
14+
defer func(x any) { // want `missing whitespace above this line \(no shared variables above defer\)`
15+
fmt.Printf("%v", x)
16+
}(nil)
17+
18+
b := true
19+
20+
defer func(x any) { // want `missing whitespace above this line \(no shared variables above defer\)`
21+
fmt.Printf("%v", x)
22+
}(true)
23+
24+
c := false
25+
26+
defer func(x any) { // want `missing whitespace above this line \(no shared variables above defer\)`
27+
fmt.Printf("%v", x)
28+
}(false)
29+
30+
_, _, _, _ = a, b, c, z
31+
}
32+

wsl.go

Lines changed: 83 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ func (w *WSL) checkCuddlingMaxAllowed(
292292
}
293293

294294
numStmtsAbove := w.numberOfStatementsAbove(cursor)
295-
previousIdents := identsFromNode(previousNode, true)
295+
previousIdents := w.identsFromNode(previousNode, true)
296296

297297
// If we don't have any statements above, we only care about potential error
298298
// cuddling (for if statements) so check that.
@@ -335,21 +335,21 @@ func (w *WSL) checkCuddlingMaxAllowed(
335335
// FEATURE(AllowWholeBlock): Allow identifier used anywhere in block
336336
// (including recursive blocks).
337337
if w.config.AllowWholeBlock {
338-
allIdentsInBlock := identsFromNode(stmt, false)
338+
allIdentsInBlock := w.identsFromNode(stmt, false)
339339
if checkIntersection(allIdentsInBlock) {
340340
return
341341
}
342342
}
343343

344344
// FEATURE(AllowFirstInBlock): Allow identifiers used first in block.
345345
if !w.config.AllowWholeBlock && w.config.AllowFirstInBlock {
346-
firstStmtIdents := identsFromNode(firstBlockStmt, true)
346+
firstStmtIdents := w.identsFromNode(firstBlockStmt, true)
347347
if checkIntersection(firstStmtIdents) {
348348
return
349349
}
350350
}
351351

352-
currentIdents := identsFromNode(stmt, true)
352+
currentIdents := w.identsFromNode(stmt, true)
353353
if checkIntersection(currentIdents) {
354354
return
355355
}
@@ -412,7 +412,7 @@ func (w *WSL) checkCuddlingWithoutIntersection(stmt ast.Node, cursor *Cursor) {
412412
prevIsValidType := previousNode == nil || prevIsAssign || prevIsDecl || prevIsIncDec
413413

414414
if _, ok := w.config.Checks[CheckAssignExpr]; !ok {
415-
if _, ok := previousNode.(*ast.ExprStmt); ok && hasIntersection(stmt, previousNode) {
415+
if _, ok := previousNode.(*ast.ExprStmt); ok && w.hasIntersection(stmt, previousNode) {
416416
prevIsValidType = prevIsValidType || ok
417417
}
418418
}
@@ -506,7 +506,7 @@ func (w *WSL) checkAppend(stmt *ast.AssignStmt, cursor *Cursor) {
506506
return
507507
}
508508

509-
if !hasIntersection(appendNode, previousNode) {
509+
if !w.hasIntersection(appendNode, previousNode) {
510510
w.addErrorNoIntersection(stmt.Pos(), CheckAppend)
511511
}
512512
}
@@ -577,7 +577,7 @@ func (w *WSL) checkDefer(stmt *ast.DeferStmt, cursor *Cursor) {
577577
cursor.Previous()
578578
cursor.Previous()
579579

580-
if hasIntersection(cursor.Stmt(), stmt) {
580+
if w.hasIntersection(cursor.Stmt(), stmt) {
581581
return 1, false
582582
}
583583
}
@@ -655,9 +655,10 @@ func (w *WSL) checkError(
655655
}
656656

657657
previousIdents := []*ast.Ident{}
658+
658659
if assign, ok := previousNode.(*ast.AssignStmt); ok {
659660
for _, lhs := range assign.Lhs {
660-
previousIdents = append(previousIdents, identsFromNode(lhs, true)...)
661+
previousIdents = append(previousIdents, w.identsFromNode(lhs, true)...)
661662
}
662663
}
663664

@@ -1154,7 +1155,7 @@ func (w *WSL) maybeCheckBlock(
11541155
if check != CheckSwitch && check != CheckTypeSwitch && check != CheckSelect {
11551156
blockList = blockStmt.List
11561157
} else {
1157-
allowedIdents = identsFromCaseArms(node)
1158+
allowedIdents = w.identsFromCaseArms(node)
11581159
}
11591160

11601161
w.checkCuddlingBlock(node, blockList, allowedIdents, cursor, 1)
@@ -1302,13 +1303,13 @@ func asGenDeclWithValueSpecs(n ast.Node) *ast.GenDecl {
13021303
return genDecl
13031304
}
13041305

1305-
func hasIntersection(a, b ast.Node) bool {
1306-
return len(nodeIdentIntersection(a, b)) > 0
1306+
func (w *WSL) hasIntersection(a, b ast.Node) bool {
1307+
return len(w.nodeIdentIntersection(a, b)) > 0
13071308
}
13081309

1309-
func nodeIdentIntersection(a, b ast.Node) []*ast.Ident {
1310-
aI := identsFromNode(a, true)
1311-
bI := identsFromNode(b, true)
1310+
func (w *WSL) nodeIdentIntersection(a, b ast.Node) []*ast.Ident {
1311+
aI := w.identsFromNode(a, true)
1312+
bI := w.identsFromNode(b, true)
13121313

13131314
return identIntersection(aI, bI)
13141315
}
@@ -1327,7 +1328,31 @@ func identIntersection(a, b []*ast.Ident) []*ast.Ident {
13271328
return intersects
13281329
}
13291330

1330-
func identsFromNode(node ast.Node, skipBlock bool) []*ast.Ident {
1331+
func isTypeOrPredeclConst(obj types.Object) bool {
1332+
switch o := obj.(type) {
1333+
case *types.TypeName:
1334+
// Covers predeclared types ("string", "int", ...) and user types.
1335+
return true
1336+
case *types.Const:
1337+
// true/false/iota are universe consts.
1338+
return o.Parent() == types.Universe
1339+
case *types.Nil:
1340+
return true
1341+
case *types.PkgName:
1342+
// Skip package qualifiers like "fmt" in fmt.Println
1343+
return true
1344+
default:
1345+
return false
1346+
}
1347+
}
1348+
1349+
// identsFromNode returns all *ast.Ident in a node except:
1350+
// - type names (types.TypeName)
1351+
// - builtin constants from the universe (true, false, iota)
1352+
// - nil (*types.Nil)
1353+
// - package names (types.PkgName)
1354+
// - the blank identifier "_"
1355+
func (w *WSL) identsFromNode(node ast.Node, skipBlock bool) []*ast.Ident {
13311356
var (
13321357
idents []*ast.Ident
13331358
seen = map[string]struct{}{}
@@ -1337,34 +1362,70 @@ func identsFromNode(node ast.Node, skipBlock bool) []*ast.Ident {
13371362
return idents
13381363
}
13391364

1365+
addIdent := func(ident *ast.Ident) {
1366+
if ident == nil {
1367+
return
1368+
}
1369+
1370+
name := ident.Name
1371+
if name == "" || name == "_" {
1372+
return
1373+
}
1374+
1375+
if _, ok := seen[name]; ok {
1376+
return
1377+
}
1378+
1379+
idents = append(idents, ident)
1380+
seen[name] = struct{}{}
1381+
}
1382+
13401383
ast.Inspect(node, func(n ast.Node) bool {
13411384
if skipBlock {
13421385
if _, ok := n.(*ast.BlockStmt); ok {
13431386
return false
13441387
}
13451388
}
13461389

1347-
if ident, ok := n.(*ast.Ident); ok {
1348-
if _, exists := seen[ident.Name]; !exists {
1349-
idents = append(idents, ident)
1350-
seen[ident.Name] = struct{}{}
1351-
}
1390+
ident, ok := n.(*ast.Ident)
1391+
if !ok {
1392+
return true
13521393
}
13531394

1395+
// Prefer Uses over Defs; fall back to Defs if not a use site.
1396+
var typesObject types.Object
1397+
if obj := w.typeInfo.Uses[ident]; obj != nil {
1398+
typesObject = obj
1399+
} else if obj := w.typeInfo.Defs[ident]; obj != nil {
1400+
typesObject = obj
1401+
}
1402+
1403+
// Unresolved (could be a build-tag or syntax artifact). Keep it.
1404+
if typesObject == nil {
1405+
addIdent(ident)
1406+
return true
1407+
}
1408+
1409+
if isTypeOrPredeclConst(typesObject) {
1410+
return true
1411+
}
1412+
1413+
addIdent(ident)
1414+
13541415
return true
13551416
})
13561417

13571418
return idents
13581419
}
13591420

1360-
func identsFromCaseArms(node ast.Node) []*ast.Ident {
1421+
func (w *WSL) identsFromCaseArms(node ast.Node) []*ast.Ident {
13611422
var (
13621423
idents []*ast.Ident
13631424
nodes []ast.Stmt
13641425
seen = map[string]struct{}{}
13651426

13661427
addUnseen = func(node ast.Node) {
1367-
for _, ident := range identsFromNode(node, true) {
1428+
for _, ident := range w.identsFromNode(node, true) {
13681429
if _, ok := seen[ident.Name]; ok {
13691430
continue
13701431
}

0 commit comments

Comments
 (0)