]> Cypherpunks repositories - gostls13.git/commitdiff
go/types: more robust behavior in the presence errors (due to import "C")
authorRobert Griesemer <gri@golang.org>
Fri, 19 Jan 2018 01:45:58 +0000 (17:45 -0800)
committerRobert Griesemer <gri@golang.org>
Tue, 23 Jan 2018 20:52:40 +0000 (20:52 +0000)
- 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
src/go/types/call.go
src/go/types/decl.go
src/go/types/stmt.go
src/go/types/testdata/importC.src
src/go/types/typexpr.go

index e5ea071e864dd8112fd52484f8682e67becf27dc..98c9e121b055a88327c55934962bf458d4bf8b30 100644 (file)
@@ -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 {
index 345df66a8a97b5e50c25cd5f6d4f0a0c19a85ba3..8fe65e41d5f35cb02a07c9ee5740cd30eda482a4 100644 (file)
@@ -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
index 7428f8f99500a78a03a8e8798237095637e4e9ed..9b250b30e74c96a47936313868a967a6259dbe34 100644 (file)
@@ -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
                }
index 1292f5cec183fa4ca71de941095c8f8c0f7f13ea..ab320088b074241184dc9edbef63262b560d7321 100644 (file)
@@ -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)
index 31436be6ada39a10e9b3acd828cd7df639608d91..f50f7f33d3e8648da277c37adf9f96bc3290d833 100644 (file)
@@ -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
index 0ab6dfdb79b37aa8d2b26bc615317d812838cc8f..92ab06b0f25a3adc2524d2a8483f4cf2370a10c7 100644 (file)
@@ -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
                }