8000 proposal: spec: consider more strict "declared and not used" check (go/vet or even spec) · Issue #20802 · golang/go · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

proposal: spec: consider more strict "declared and not used" check (go/vet or even spec) #20802

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.

Sign up for GitHub

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

Jump to bottom
Open
griesemer opened this issue Jun 26, 2017 · 6 comments
Open

proposal: spec: consider more strict "declared and not used" check (go/vet or even spec) #20802

griesemer opened this issue Jun 26, 2017 · 6 comments
Labels
LanguageChange Suggested changes to the Go language LanguageChangeReview Discussed by language change review committee Proposal
Milestone
Proposal

Comments

@griesemer
Copy link
Contributor

The following code

func f(i interface{}) *T {
	x, ok := i.(*T)
	if !ok {
		x := new(T)
		x.f = 0
	}
	return x
}

compiles without errors even though it is obviously incorrect: Inside the block of the if statement, the := operator should have been = instead. The compiler doesn't complain because the assignment to x.f counts as use of x.

Arguably, assignments to fields/elements of composite type variables shouldn't be counted as uses. Their effect is lost if the variable itself is never used and they may hide the fact that the variable is not used.

(This was the underlying cause for #20789.)
@gopherbot gopherbot added this to the Proposal milestone Jun 26, 2017
@rsc rsc changed the title Proposal: consider more strict "declared and not used" check (go/vet or even spec) proposal: spec: consider more strict "declared and not used" check (go/vet or even spec) Jun 26, 2017
@rsc
Copy link
Contributor
rsc commented Jun 26, 2017

Had to pick either spec or cmd/vet for a prefix; picked spec but can still consider vet here.

@bcmills bcmills mentioned this issue Jun 26, 2017
Open
@alandonovan
Copy link
Contributor
alandonovan commented Jul 6, 2017 •
edited
Loading

It's surprisingly tricky to detect "weakly used" variables in the general case. In the simple example above, where x is a variable of struct type T and its field f field is not a reference, it suffices to prove that all references to x occur "on the left side" of an assignment. However, this simple check is inadequate for selections that are indirect. If the type of x.f is a pointer or reference, then the sequence of assignments x.f = y; x.f.g = 1 makes x appear to be "weakly used" (that is, only assigned) but it has the additional effect y.g = 1, which could be visible to other functions, as in this example:

type T struct { u *U }
type U struct { i int }

var global = T{u: new(U)}

func f() {
        var t T
        t = global
        t.u.i = 1
}

A sound check strong enough to catch the bug that motivated this proposal would require SSA form and a simple escape analysis, which is more complex than belongs in the language spec, but possibly appropriate for vet.

@mvdan
Copy link
Member
mvdan commented Jul 6, 2017

Would it be possible to make this a compiler error without including the restriction in the spec? Or is that generally avoided? (This is assuming that when a spec change was mentioned, it would be the compiler enforcing it, not vet)

@alandonovan
Copy link
Contributor

I've sketched a simpler heuristic for vet in https://go-review.googlesource.com/c/47670.

@mvdan Unless a compiler accepts exactly the set of programs the spec requires it to accept, it is implementing a dialect of the language. Dialects cause problems for users because programs that work with one compiler or analysis tool don't work with another.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/47670 mentions this issue.

@rsc rsc added the v2 An incompatible library change label Jul 17, 2017
@ianlancetaylor
Copy link
Contributor

We can't detect all possible bugs, but if we only count x.f = 1 as an assignment and don't count x.f1.f2 = 1 as one, perhaps we can at least do that.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 20, 2018
@ianlancetaylor ianlancetaylor mentioned this issue May 15, 2018
Open
@ianlancetaylor ianlancetaylor mentioned this issue Oct 20, 2019
Open
@ianlancetaylor ianlancetaylor added LanguageChange Suggested changes to the Go language LanguageChangeReview Discussed by language change review committee and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. v2 An incompatible library change labels Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
LanguageChange Suggested changes to the Go language LanguageChangeReview Discussed by language change review committee Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
6 participants
@rsc @ianlancetaylor @mvdan @alandonovan @griesemer @gopherbot
0