Add new query ImpossibleInterfaceNilCheck.

change-notes/1.24/analysis-go.md

@@ -5,6 +5,7 @@
 | **Query**                                                                 | **Tags**                                                                   | **Purpose**                                                                                                                                            |
 |---------------------------------------------------------------------------|----------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Constant length comparison (`go/constant-length-comparison`)     | correctness | Highlights code that checks the length of an array or slice against a constant before indexing it using a variable, suggesting a logic error. Results are shown on LGTM by default. |
+| Impossible interface nil check (`go/impossible-interface-nil-check`) | correctness | Highlights code that compares an interface value that cannot be `nil` to `nil`, suggesting a logic error. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

ql/src/RedundantCode/ImpossibleInterfaceNilCheck.go

@@ -0,0 +1,16 @@
+package main
+
+import "fmt"
+
+func fetch(url string) (string, *RequestError)
+
+func niceFetch(url string) {
+	var s string
+	var e error
+	s, e = fetch(url)
+	if e != nil {
+		fmt.Printf("Unable to fetch URL: %v\n", e)
+	} else {
+		fmt.Printf("URL contents: %s\n", s)
+	}
+}

ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Interface values in Go are type tagged, that is, they are essentially pairs of the form
+<code>(value, type)</code>, where <code>value</code> is a non-interface value with the given
+<code>type</code>. Such a pair is never <code>nil</code>, even if <code>value</code> is
+<code>nil</code>.
+</p>
+<p>
+In particular, if a non-interface value <code>v</code> is assigned to a variable <code>x</code>
+whose type is an interface, then <code>x</code> will never be <code>nil</code>, regardless of
+<code>v</code>. Comparing <code>x</code> to <code>nil</code> is pointless, and may indicate
+a misunderstanding of Go interface values or some other underlying bug.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Carefully inspect the comparison to ensure it is not a symptom of a bug.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows a declaration of a function <code>fetch</code> which fetches the
+contents of a URL, returning either the contents or an error value, which is a pointer to a custom
+error type <code>RequestError</code> (not shown). The function <code>niceFetch</code> wraps this
+function, printing out either the URL contents or an error message.
+</p>
+<sample src="ImpossibleInterfaceNilCheck.go"/>
+<p>
+However, because <code>e</code> is declared to be of type <code>error</code>, which is an
+interface, the <code>nil</code> check will never succeed, since <code>e</code> can never be
+<code>nil</code>.
+</p>
+<p>
+In this case, the problem can be solved by using a short variable declaration using operator
+<code>:=</code>, which will automatically infer the more precise non-interface type
+<code>*ResourceError</code> for <code>e</code>, making the <code>nil</code> check behave as
+expected.
+</p>
+<sample src="ImpossibleInterfaceNilCheckGood.go" />
+</example>
+</qhelp>

ql/src/RedundantCode/ImpossibleInterfaceNilCheck.ql

@@ -0,0 +1,49 @@
+/**
+ * @name Impossible interface nil check
+ * @description Comparing a non-nil interface value to nil may indicate a logic error.
+ * @kind problem
+ * @problem.severity warning
+ * @id go/impossible-interface-nil-check
+ * @tags correctness
+ * @precision high
+ */
+
+import go
+
+/**
+ * Holds if `eq` compares interface value `nd` to `nil`.
+ */
+predicate interfaceNilCheck(DataFlow::EqualityTestNode eq, DataFlow::Node nd) {
+  nd = eq.getAnOperand() and
+  nd.getType().getUnderlyingType() instanceof InterfaceType and
+  eq.getAnOperand().getType() instanceof NilLiteralType
+}
+
+/**
+ * Holds if the result of `nd` may flow to an interface `nil` check.
+ */
+predicate flowsToInterfaceNilCheck(DataFlow::Node nd) {
+  interfaceNilCheck(_, nd) or
+  flowsToInterfaceNilCheck(nd.getASuccessor())
+}
+
+/**
+ * Holds if `nd` is a non-nil interface value.
+ */
+predicate nonNilWrapper(DataFlow::Node nd) {
+  flowsToInterfaceNilCheck(nd) and
+  forex(DataFlow::Node pred | pred = nd.getAPredecessor() |
+    exists(Type predtp | predtp = pred.getType().getUnderlyingType() |
+      not predtp instanceof InterfaceType and
+      not predtp instanceof NilLiteralType
+    )
+    or
+    nonNilWrapper(pred)
+  )
+}
+
+from DataFlow::EqualityTestNode eq, DataFlow::Node nd
+where
+  interfaceNilCheck(eq, nd) and
+  nonNilWrapper(nd)
+select nd, "This value can never be nil, since it is a wrapped interface value."

ql/src/RedundantCode/ImpossibleInterfaceNilCheckGood.go

@@ -0,0 +1,12 @@
+package main
+
+import "fmt"
+
+func niceFetchGood(url string) {
+	s, e := fetch(url)
+	if e != nil {
+		fmt.Printf("Unable to fetch URL: %v\n", e)
+	} else {
+		fmt.Printf("URL contents: %s\n", s)
+	}
+}

ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.expected

@@ -0,0 +1,2 @@
+| ImpossibleInterfaceNilCheck.go:9:5:9:5 | e | This value can never be nil, since it is a wrapped interface value. |
+| tst.go:10:14:10:14 | y | This value can never be nil, since it is a wrapped interface value. |

ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.go

@@ -0,0 +1,14 @@
+package main
+
+import "fmt"
+
+func niceFetch(url string) {
+	var s string
+	var e error
+	s, e = fetch(url)
+	if e != nil {
+		fmt.Printf("Unable to fetch URL: %v\n", e)
+	} else {
+		fmt.Printf("URL contents: %s\n", s)
+	}
+}

ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.qlref

@@ -0,0 +1 @@
+RedundantCode/ImpossibleInterfaceNilCheck.ql

ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheckGood.go

@@ -0,0 +1,12 @@
+package main
+
+import "fmt"
+
+func niceFetchGood(url string) {
+	s, e := fetch(url)
+	if e != nil {
+		fmt.Printf("Unable to fetch URL: %v\n", e)
+	} else {
+		fmt.Printf("URL contents: %s\n", s)
+	}
+}

ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/lib.go

@@ -0,0 +1,11 @@
+package main
+
+type RequestError string
+
+func (e RequestError) Error() string {
+	return string(e)
+}
+
+func fetch(url string) (string, *RequestError) {
+	return "stuff", nil
+}

ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/tst.go

@@ -0,0 +1,27 @@
+package main
+
+import "fmt"
+
+func test1() {
+	var x *int = nil
+	var y interface{} = x
+	fmt.Println(x == nil)
+	fmt.Println(x == y)
+	fmt.Println(y == nil) // NOT OK
+}
+
+func test2() {
+	var x *int = nil
+	test3(x)
+}
+
+func test3(y interface{}) {
+	// we don't want to flag this one, even though we might be able to
+	// inter-procedurally establish that y cannot be nil
+	fmt.Println(y == nil) // OK
+}
+
+func test4() {
+	var y interface{}
+	fmt.Println(y == nil) // OK
+}