Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/vet: ensure that checks work correctly in the presence of aliases #17756

Closed
josharian opened this issue Nov 3, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@josharian
Copy link
Contributor

commented Nov 3, 2016

As an example, cmd/vet/lostcancel.go contains this code:

// isContextWithCancel reports whether n is one of the qualified identifiers
// context.With{Cancel,Timeout,Deadline}.
func isContextWithCancel(f *File, n ast.Node) bool {
	if sel, ok := n.(*ast.SelectorExpr); ok {
		switch sel.Sel.Name {
		case "WithCancel", "WithTimeout", "WithDeadline":
			if x, ok := sel.X.(*ast.Ident); ok {
				if pkgname, ok := f.pkg.uses[x].(*types.PkgName); ok {
					return pkgname.Imported().Path() == contextPackage
				}
				// Import failed, so we can't check package path.
				// Just check the local package name (heuristic).
				return x.Name == "context"
			}
		}
	}
	return false
}

It is looking specifically for constructs like context.WithCancel. But with aliases, that could be instead just be WithCancel, or something else entirely. And this is particularly likely to happen with the context package because of the golang.org/x/net to stdlib context move. I believe there are other checks for selector expressions as well that need to be updated to be alias-aware.

I suspect that this sort of need will happen enough ("is this thing really context.WithCancel, regardless of what it happens to be called right here?") that it'd be good to have go/types support to make it easier. Haven't thought yet about what the API looks like.

See also #17755.

cc @robpike @alandonovan @griesemer

@josharian josharian added this to the Go1.8Maybe milestone Nov 3, 2016

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

A challenge with vet is that it cannot assume it has correct or indeed any type information for imported packages. Consequently, there's a fine balance to be struck between using syntax information, using type information that can be relied upon because it's local to a file, and using type information from dependencies, which is likely to be absent for all non-standard packages. The way go/types represents dangling aliases, that is, aliases to members of packages for which no type information is available, does not tell us even the name of the missing thing, so using aliases here would make the tool less robust.

In the specific case of context.WithCancel, it's likely that the various clones of the context package (the standard one, the one in golang.org/x/net, and Google's original one) may use aliases to smooth over their differences, but probably all clients will access the WithCancel function using a qualified identifier, context.WithCancel, for some unspecified import that defines context. This check's apparent sloppiness about matching the package name was a conscious and deliberate choice.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

Here's a list of uses of selector expressions in vet:

  • atomic: uses package name == "atomic"
  • cgo: uses package name == "C"
  • lostcancel: uses package name == "context"
  • nilfunc: looking for any functions whatsoever
  • print: looking for any functions whatsoever
  • tests: only checks selector, not package name
  • unsafeptr: uses t.Obj().Pkg().Path() == "reflect"
  • unused: uses obj.Pkg().Path()

The ones that use package name directly are probably fine, per Alan's comments above. It seems fairly likely that packages atomic, C, and context won't be aliased away.

For unsafeptr, unsafe can't be aliased, and reflect probably won't be.

It might be better for unused to use package name rather than obj.Pkg().Path(), although the packages that matter out of the box (errors, fmt, sort) are unlikely to be aliased. Leaving that decision to Alan.

That's all I see from a first visual inspection of vet. Alan, if you're happy, feel free to close this, and we'll deal with other specific issues as they come up in testing.

@josharian josharian closed this Nov 5, 2016

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

For the record, I audited every go/ast-using package in the standard library and fixed the important ones where aliases could lead to a crash or incorrect results, but there are rapidly diminishing returns to worrying about aliases in these vet checks. Standard packages are, I suspect, unlikely to be the targets of aliases, and vet has a very fragmentary view of things outside the few files it analyzes.

@golang golang locked and limited conversation to collaborators Nov 7, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.