Fix bug

owen-mc · owen-mc · commit 8dc0688b6f30 · 2024-11-26T22:25:47.000Z
diff --git a/go/ql/lib/semmle/go/Types.qll b/go/ql/lib/semmle/go/Types.qll
@@ -523,8 +523,12 @@ class StructType extends @structtype, CompositeType {
   private predicate hasFieldCand(string name, Field f, int depth, boolean isEmbedded) {
     f = this.getOwnField(name, isEmbedded) and depth = 0
     or
-    not this.hasOwnField(_, name, _, _) and
-    f = this.getFieldOfEmbedded(_, name, depth, isEmbedded)
+    f = this.getFieldOfEmbedded(_, name, depth, isEmbedded) and
+    // If this is a cyclic field and this is not the first time we see this embedded field
+    // then don't include it as a field candidate to avoid non-termination.
+    not exists(Type t | lookThroughPointerType(t) = lookThroughPointerType(f.getType()) |
+      this.hasOwnField(_, name, t, _)
+    )
   }
 
   private predicate hasMethodCand(string name, Method m, int depth) {
diff --git a/go/ql/test/library-tests/semmle/go/Types/Field_getPackage.expected b/go/ql/test/library-tests/semmle/go/Types/Field_getPackage.expected
@@ -13,8 +13,8 @@
 | depth.go:19:2:19:2 | f | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
 | embedded.go:4:2:4:2 | A | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
 | embedded.go:8:3:8:5 | Baz | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
-| embedded.go:13:2:13:4 | Qux | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
-| embedded.go:14:2:14:4 | Baz | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
+| embedded.go:12:2:12:4 | Qux | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
+| embedded.go:13:2:13:4 | Baz | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
 | generic.go:4:2:4:11 | valueField | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
 | generic.go:5:2:5:13 | pointerField | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
 | generic.go:6:2:6:11 | arrayField | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types |
diff --git a/go/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName2.expected b/go/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName2.expected
@@ -18,10 +18,11 @@
 | depth.go:19:2:19:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.c | f |
 | depth.go:19:2:19:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.d | f |
 | embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Baz | A |
+| embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz | A |
 | embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Qux | A |
 | embedded.go:8:3:8:5 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Qux | Baz |
-| embedded.go:13:2:13:4 | Qux | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz | Qux |
-| embedded.go:14:2:14:4 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz | Baz |
+| embedded.go:12:2:12:4 | Qux | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz | Qux |
+| embedded.go:13:2:13:4 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz | Baz |
 | generic.go:4:2:4:11 | valueField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.GenericStruct1 | valueField |
 | generic.go:5:2:5:13 | pointerField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.GenericStruct1 | pointerField |
 | generic.go:6:2:6:11 | arrayField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.GenericStruct1 | arrayField |
@@ -72,6 +73,7 @@
 | pkg2/tst.go:4:2:4:2 | g | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2.T | g |
 | pkg2/tst.go:8:2:8:2 | g | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2.G | g |
 | pkg2/tst.go:8:2:8:2 | g | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2.T | g |
+| pkg2/tst.go:17:2:17:8 | NCField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsNameClash | NCField |
 | pkg2/tst.go:17:2:17:8 | NCField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1.NameClash | NCField |
 | pkg2/tst.go:17:2:17:8 | NCField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2.NameClash | NCField |
 | struct_tags.go:4:2:4:7 | field1 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.S1 | field1 |
diff --git a/go/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName3.expected b/go/ql/test/library-tests/semmle/go/Types/Field_hasQualifiedName3.expected
@@ -18,10 +18,11 @@
 | depth.go:19:2:19:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | c | f |
 | depth.go:19:2:19:2 | f | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | d | f |
 | embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | Baz | A |
+| embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | EmbedsBaz | A |
 | embedded.go:4:2:4:2 | A | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | Qux | A |
 | embedded.go:8:3:8:5 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | Qux | Baz |
-| embedded.go:13:2:13:4 | Qux | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | EmbedsBaz | Qux |
-| embedded.go:14:2:14:4 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | EmbedsBaz | Baz |
+| embedded.go:12:2:12:4 | Qux | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | EmbedsBaz | Qux |
+| embedded.go:13:2:13:4 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | EmbedsBaz | Baz |
 | generic.go:4:2:4:11 | valueField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | GenericStruct1 | valueField |
 | generic.go:5:2:5:13 | pointerField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | GenericStruct1 | pointerField |
 | generic.go:6:2:6:11 | arrayField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | GenericStruct1 | arrayField |
@@ -72,6 +73,7 @@
 | pkg2/tst.go:4:2:4:2 | g | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2 | T | g |
 | pkg2/tst.go:8:2:8:2 | g | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2 | G | g |
 | pkg2/tst.go:8:2:8:2 | g | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2 | T | g |
+| pkg2/tst.go:17:2:17:8 | NCField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | EmbedsNameClash | NCField |
 | pkg2/tst.go:17:2:17:8 | NCField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg1 | NameClash | NCField |
 | pkg2/tst.go:17:2:17:8 | NCField | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types/pkg2 | NameClash | NCField |
 | struct_tags.go:4:2:4:7 | field1 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types | S1 | field1 |
diff --git a/go/ql/test/library-tests/semmle/go/Types/ImplementsComparable.expected b/go/ql/test/library-tests/semmle/go/Types/ImplementsComparable.expected
@@ -1,2 +1,2 @@
-failures
 testFailures
+failures
diff --git a/go/ql/test/library-tests/semmle/go/Types/MethodTypes.expected b/go/ql/test/library-tests/semmle/go/Types/MethodTypes.expected
@@ -30,6 +30,7 @@
 | interface.go:95:6:95:8 | i18 | StringA | func() string |
 | interface.go:101:6:101:8 | i19 | StringB | func() string |
 | interface.go:105:6:105:8 | i20 | StringB | func() string |
+| main.go:17:6:17:20 | EmbedsNameClash | NCMethod | func()  |
 | pkg1/embedding.go:19:6:19:13 | embedder | f | func() int |
 | pkg1/embedding.go:22:6:22:16 | ptrembedder | f | func() int |
 | pkg1/embedding.go:22:6:22:16 | ptrembedder | g | func() int |
diff --git a/go/ql/test/library-tests/semmle/go/Types/QualifiedNames.expected b/go/ql/test/library-tests/semmle/go/Types/QualifiedNames.expected
@@ -9,7 +9,7 @@
 | depth.go:18:6:18:6 | d | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.d |
 | embedded.go:3:6:3:8 | Baz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Baz |
 | embedded.go:7:6:7:8 | Qux | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.Qux |
-| embedded.go:12:6:12:14 | EmbedsBaz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz |
+| embedded.go:11:6:11:14 | EmbedsBaz | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.EmbedsBaz |
 | generic.go:3:6:3:19 | GenericStruct1 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.GenericStruct1 |
 | generic.go:11:6:11:27 | CircularGenericStruct1 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.CircularGenericStruct1 |
 | generic.go:15:6:15:31 | UsesCircularGenericStruct1 | github.com/github/codeql-go/ql/test/library-tests/semmle/go/Types.UsesCircularGenericStruct1 |
diff --git a/go/ql/test/library-tests/semmle/go/Types/SignatureType_isVariadic.expected b/go/ql/test/library-tests/semmle/go/Types/SignatureType_isVariadic.expected
@@ -1,2 +1,2 @@
-failures
 testFailures
+failures
diff --git a/go/ql/test/library-tests/semmle/go/Types/StructFields.expected b/go/ql/test/library-tests/semmle/go/Types/StructFields.expected
@@ -20,8 +20,9 @@
 | embedded.go:3:6:3:8 | Baz | embedded.go:3:10:5:1 | struct type | A | string |
 | embedded.go:7:6:7:8 | Qux | embedded.go:7:10:9:1 | struct type | A | string |
 | embedded.go:7:6:7:8 | Qux | embedded.go:7:10:9:1 | struct type | Baz | * Baz |
-| embedded.go:12:6:12:14 | EmbedsBaz | embedded.go:12:16:15:1 | struct type | Baz | string |
-| embedded.go:12:6:12:14 | EmbedsBaz | embedded.go:12:16:15:1 | struct type | Qux | Qux |
+| embedded.go:11:6:11:14 | EmbedsBaz | embedded.go:11:16:14:1 | struct type | A | string |
+| embedded.go:11:6:11:14 | EmbedsBaz | embedded.go:11:16:14:1 | struct type | Baz | string |
+| embedded.go:11:6:11:14 | EmbedsBaz | embedded.go:11:16:14:1 | struct type | Qux | Qux |
 | generic.go:3:6:3:19 | GenericStruct1 | generic.go:3:28:9:1 | struct type | arrayField | [10]T |
 | generic.go:3:6:3:19 | GenericStruct1 | generic.go:3:28:9:1 | struct type | mapField | [string]T |
 | generic.go:3:6:3:19 | GenericStruct1 | generic.go:3:28:9:1 | struct type | pointerField | * T |
@@ -33,6 +34,7 @@
 | generic.go:19:6:19:19 | GenericStruct2 | generic.go:19:42:22:1 | struct type | structField | GenericStruct1 |
 | generic.go:24:6:24:20 | GenericStruct2b | generic.go:24:39:26:1 | struct type | structField | GenericStruct2 |
 | generic.go:28:6:28:27 | CircularGenericStruct2 | generic.go:28:39:30:1 | struct type | pointerField | * CircularGenericStruct2 |
+| main.go:17:6:17:20 | EmbedsNameClash | main.go:17:22:19:1 | struct type | NCField | string |
 | main.go:17:6:17:20 | EmbedsNameClash | main.go:17:22:19:1 | struct type | NameClash | NameClash |
 | pkg1/embedding.go:19:6:19:13 | embedder | pkg1/embedding.go:19:15:19:28 | struct type | base | base |
 | pkg1/embedding.go:22:6:22:16 | ptrembedder | pkg1/embedding.go:22:18:22:32 | struct type | base | * base |
diff --git a/go/ql/test/library-tests/semmle/go/Types/Types.expected b/go/ql/test/library-tests/semmle/go/Types/Types.expected
@@ -9,7 +9,7 @@
 | depth.go:18:6:18:6 | d | d |
 | embedded.go:3:6:3:8 | Baz | Baz |
 | embedded.go:7:6:7:8 | Qux | Qux |
-| embedded.go:12:6:12:14 | EmbedsBaz | EmbedsBaz |
+| embedded.go:11:6:11:14 | EmbedsBaz | EmbedsBaz |
 | generic.go:3:6:3:19 | GenericStruct1 | GenericStruct1 |
 | generic.go:11:6:11:27 | CircularGenericStruct1 | CircularGenericStruct1 |
 | generic.go:15:6:15:31 | UsesCircularGenericStruct1 | UsesCircularGenericStruct1 |
diff --git a/go/ql/test/library-tests/semmle/go/Types/embedded.go b/go/ql/test/library-tests/semmle/go/Types/embedded.go
@@ -8,7 +8,6 @@ type Qux struct {
 	*Baz
 }
 
-// EmbedsBaz should have a field A but does not
 type EmbedsBaz struct {
 	Qux
 	Baz string

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,2 @@`
`1`		`-failures`
`2`	`1`	`testFailures`
	`2`	`+failures`