Semgrep: Go type inference fails when variable is redeclared in a new scope.

[dgryski]

To Reproduce
https://semgrep.dev/s/l0Jy/

Expected behavior
r is an *http.Request in the outer scope, but is a new variable (with a new type) redefined in the defer block.

Environment
Encountered in homebrew build, replicated on semgrep.dev.

[mschwager]

Hey @dgryski,

Thanks for finding this - it looks like a bug to me. I've added this to our current sprint :+1:

For my own edification, how does the scoping of r work here? Will r in h.ServeHTTP(w, r) be the value from recover() or *http.Request from the outer scope? I have limited Golang experience.

[dgryski]

The scope of the redeclared r in the defer block ends at the closing brace. In h.ServeHTTP(w, r) the value of r will be the one from the outer scope (in this case the function argument).

Go specification on scopes: https://golang.org/ref/spec#Declarations_and_scope

[aryx]

This is a bug in Naming_AST.ml. We think it's the same variable.
Could take a bit of time to fix it. The naming algorithm tries to handle all the languages at once, and so it's quite tricky to modify.

[dgryski]

Hmm.. doesn't this affect more than just Go then? Or is it Go's short declaration syntax the issue?

[aryx]

Yes, I think := got transformed at parsing time into a regular assign, which confuses Naming_ast.ml

[aryx]

I think because I didn't fully understand at the time the semantic of :=, and whether it was redefining a new var or reassigning an existing var (I still don't fully get the different with regular Go declaration and the
advantage of := over var declarations).

[dgryski]

A quick summary of the rules (although check the spec for the fine print):

:= declares a variable if it doesn't exist in this scope; it is an error to use := with a variable that already exists in the current scope. However, if you have a multiple assignment (a, b := 10, 20), then you're allowed to use := as long as at least one of the variables is being declared. := is treated like regular variable assignment. This is handy to allow for reusing err through a chunk of code.

If you're in a new scope, then := will shadow a declaration in the outer scope. (This is occasionally a bug in Go code and occasionally a workaround to a different Go mis-feature.)

In the above case, we have := in a new scope (the if block), so it's declaring a new variable r shadowing the r *http.Request in the outer scope (the function literal argument list).

Spec: https://golang.org/ref/spec#Short_variable_declarations