go/types: more robust behavior in the presence errors (due to import "C")

- Don't complain about invalid constant type if the type is
  invalid already (we do this in other places as well). This
  is useful to do in general, and even more so if we have
  invalid types due to import "C".

- Type-check the lhs of an assignment even if we bail out early
  due to an error on the rhs. This was simply an oversight. We
  already have machinery in place to "use" expressions; in this
  case we just have to also make sure we don't overcount "uses"
  of variables on the lhs.

- Fix overcount uses correction in assignments: Only do it if
  the variable in question is declared inside the same package
  to avoid possible race conditions when type-checking exported
  variables concurrently.

Fixes #22090.

Change-Id: I4c1b59f9ce38970e7129fedc5f6023908386e4f1
Reviewed-on: https://go-review.googlesource.com/88375
Reviewed-by: Alan Donovan <adonovan@google.com>

src/go/types/assignments.go

@@ -154,8 +154,11 @@ func (check *Checker) assignVar(lhs ast.Expr, x *operand) Type {
 	var v_used bool
 	if ident != nil {
 		if _, obj := check.scope.LookupParent(ident.Name, token.NoPos); obj != nil {
-			v, _ = obj.(*Var)
-			if v != nil {
+			// It's ok to mark non-local variables, but ignore variables
+			// from other packages to avoid potential race conditions with
+			// dot-imported variables.
+			if w, _ := obj.(*Var); w != nil && w.pkg == check.pkg {
+				v = w
 				v_used = v.used
 			}
 		}
@@ -249,6 +252,7 @@ func (check *Checker) assignVars(lhs, rhs []ast.Expr) {
 	l := len(lhs)
 	get, r, commaOk := unpack(func(x *operand, i int) { check.multiExpr(x, rhs[i]) }, len(rhs), l == 2)
 	if get == nil {
+		check.useLHS(lhs...)
 		return // error reported by unpack
 	}
 	if l != r {

src/go/types/call.go

@@ -90,15 +90,52 @@ func (check *Checker) call(x *operand, e *ast.CallExpr) exprKind {
 // use type-checks each argument.
 // Useful to make sure expressions are evaluated
 // (and variables are "used") in the presence of other errors.
+// The arguments may be nil.
 func (check *Checker) use(arg ...ast.Expr) {
 	var x operand
 	for _, e := range arg {
-		if e != nil { // be safe
+		// The nil check below is necessary since certain AST fields
+		// may legally be nil (e.g., the ast.SliceExpr.High field).
+		if e != nil {
 			check.rawExpr(&x, e, nil)
 		}
 	}
 }
 
+// useLHS is like use, but doesn't "use" top-level identifiers.
+// It should be called instead of use if the arguments are
+// expressions on the lhs of an assignment.
+// The arguments must not be nil.
+func (check *Checker) useLHS(arg ...ast.Expr) {
+	var x operand
+	for _, e := range arg {
+		// If the lhs is an identifier denoting a variable v, this assignment
+		// is not a 'use' of v. Remember current value of v.used and restore
+		// after evaluating the lhs via check.rawExpr.
+		var v *Var
+		var v_used bool
+		if ident, _ := unparen(e).(*ast.Ident); ident != nil {
+			// never type-check the blank name on the lhs
+			if ident.Name == "_" {
+				continue
+			}
+			if _, obj := check.scope.LookupParent(ident.Name, token.NoPos); obj != nil {
+				// It's ok to mark non-local variables, but ignore variables
+				// from other packages to avoid potential race conditions with
+				// dot-imported variables.
+				if w, _ := obj.(*Var); w != nil && w.pkg == check.pkg {
+					v = w
+					v_used = v.used
+				}
+			}
+		}
+		check.rawExpr(&x, e, nil)
+		if v != nil {
+			v.used = v_used // restore v.used
+		}
+	}
+}
+
 // useGetter is like use, but takes a getter instead of a list of expressions.
 // It should be called instead of use if a getter is present to avoid repeated
 // evaluation of the first argument (since the getter was likely obtained via

src/go/types/decl.go

@@ -111,7 +111,11 @@ func (check *Checker) constDecl(obj *Const, typ, init ast.Expr) {
 	if typ != nil {
 		t := check.typ(typ)
 		if !isConstType(t) {
-			check.errorf(typ.Pos(), "invalid constant type %s", t)
+			// don't report an error if the type is an invalid C (defined) type
+			// (issue #22090)
+			if t.Underlying() != Typ[Invalid] {
+				check.errorf(typ.Pos(), "invalid constant type %s", t)
+			}
 			obj.typ = Typ[Invalid]
 			return
 		}

src/go/types/stmt.go

@@ -731,6 +731,9 @@ func (check *Checker) stmt(ctxt stmtContext, s ast.Stmt) {
 		// declaration, but the post statement must not."
 		if s, _ := s.Post.(*ast.AssignStmt); s != nil && s.Tok == token.DEFINE {
 			check.softErrorf(s.Pos(), "cannot declare in post statement")
+			// Don't call useLHS here because we want to use the lhs in
+			// this errroneous statement so that we don't get errors about
+			// these lhs variables being declared but not used.
 			check.use(s.Lhs...) // avoid follow-up errors
 		}
 		check.stmt(inner, s.Body)

src/go/types/testdata/importC.src

@@ -8,3 +8,28 @@ import "C"
 import _ /* ERROR cannot rename import "C" */ "C"
 import foo /* ERROR cannot rename import "C" */ "C"
 import . /* ERROR cannot rename import "C" */ "C"
+
+// Test cases extracted from issue #22090.
+
+import "unsafe"
+
+const _ C.int = 0xff // no error due to invalid constant type
+
+type T struct {
+	Name    string
+	Ordinal int
+}
+
+func f(args []T) {
+	var s string
+	for i, v := range args {
+		cname := C.CString(v.Name)
+		args[i].Ordinal = int(C.sqlite3_bind_parameter_index(s, cname)) // no error due to i not being "used"
+		C.free(unsafe.Pointer(cname))
+	}
+}
+
+type CType C.Type
+
+const _ CType = C.X // no error due to invalid constant type
+const _ = C.X

src/go/types/typexpr.go

@@ -86,6 +86,9 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 		}
 
 	case *Var:
+		// It's ok to mark non-local variables, but ignore variables
+		// from other packages to avoid potential race conditions with
+		// dot-imported variables.
 		if obj.pkg == check.pkg {
 			obj.used = true
 		}