Merge pull request #195 from max/impossible-interface-nil-check

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,53 @@
+<!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>
+
+<references>
+<li>Jordan Orelli: <a href="https://jordanorelli.com/post/32665860244/how-to-use-interfaces-in-go">How to use interfaces in Go</a>.</li>
+<li>Go Frequently Asked Questions: <a href="https://golang.org/doc/faq#nil_error">Why is my nil error value not equal to nil?</a>.</li>
+</references>
+</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/src/semmle/go/controlflow/IR.qll

@@ -552,42 +552,64 @@ module IR {
 
     ExtractTupleElementInstruction() { this = MkExtractNode(s, i) }
 
-    /** Holds if this extracts the `idx`th value of the result of `base`. */
-    predicate extractsElement(Instruction base, int idx) {
+    Instruction getBase() {
       exists(Expr baseExpr |
         baseExpr = s.(Assignment).getRhs() or
         baseExpr = s.(ValueSpec).getInit()
       |
-        base = evalExprInstruction(baseExpr) and
-        idx = i
+        result = evalExprInstruction(baseExpr)
       )
       or
-      base = MkNextNode(s) and
-      idx = i
+      result = MkNextNode(s)
       or
-      base = evalExprInstruction(s.(ReturnStmt).getExpr()) and
-      idx = i
+      result = evalExprInstruction(s.(ReturnStmt).getExpr())
       or
-      base = evalExprInstruction(s.(CallExpr).getArgument(0)) and
-      idx = i
+      result = evalExprInstruction(s.(CallExpr).getArgument(0).stripParens())
+    }
+
+    /** Holds if this extracts the `idx`th value of the result of `base`. */
+    predicate extractsElement(Instruction base, int idx) {
+      base = getBase() and idx = i
     }
 
     override Type getResultType() {
-      result = s.(Assignment).getLhs(i).getType()
+      exists(CallExpr c | getBase() = evalExprInstruction(c) |
+        result = c.getTarget().getResultType(i)
+      )
       or
-      result = s.(ValueSpec).getNameExpr(i).getType()
-      or
-      i = 0 and
-      result = s.(RangeStmt).getKey().getType()
-      or
-      i = 1 and
-      result = s.(RangeStmt).getValue().getType()
-      or
-      result = s.(ReturnStmt).getEnclosingFunction().getType().getResultType(i)
-      or
-      exists(CallExpr inner |
-        inner = s.(CallExpr).getArgument(0).stripParens() and
-        result = inner.getTarget().getResultType(i)
+      exists(Type rangeType | rangeType = s.(RangeStmt).getDomain().getType().getUnderlyingType() |
+        exists(Type baseType |
+          baseType = rangeType.(ArrayType).getElementType() or
+          baseType = rangeType.(PointerType).getBaseType().getUnderlyingType().(ArrayType).getElementType() or
+          baseType = rangeType.(SliceType).getElementType() |
+          i = 0 and
+          result instanceof IntType
+          or
+          i = 1 and
+          result = baseType
+        )
+        or
+        rangeType instanceof StringType and
+        (
+          i = 0 and
+          result instanceof IntType
+          or
+          result = Builtin::rune().getType()
+        )
+        or
+        exists(MapType map | map = rangeType |
+          i = 0 and
+          result = map.getKeyType()
+          or
+          i = 1 and
+          result = map.getValueType()
+        )
+        or
+        i = 0 and
+        result = rangeType.(RecvChanType).getElementType()
+        or
+        i = 0 and
+        result = rangeType.(SendRecvChanType).getElementType()
       )
     }
 

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
+}