should omit type uint32

[nathany]

should omit type uint32 from declaration of var flags; it will be inferred from the right-hand side

This is technically incorrect, as type inference assumes int.

This is similar to #7.

The line in question is this ugly bit of code:

var flags uint32 = syscall.IN_MOVED_TO | syscall.IN_MOVED_FROM |
    syscall.IN_CREATE | syscall.IN_ATTRIB | syscall.IN_MODIFY |
    syscall.IN_MOVE_SELF | syscall.IN_DELETE | syscall.IN_DELETE_SELF

Note: I updated to latest to confirm that this is still an issue, ae65d27.

[dsymonds]

Interesting. Hacking in some debugging statements I see that an underlying problem is IN_MOVED_TO not declared by package syscall, which means that for whatever reason the typechecker is not finding that symbol in the syscall package, and thus is giving up on typechecking that line entirely, which leads to golint not suppressing that warning message.

[dsymonds]

Oh, that was reproduced on a Mac, which doesn't have IN_MOVED_TO. But it still occurs on a Linux machine too.

[nathany]

I too was running golint on a Mac for a _linux.go file, which is odd in a way.

[nathany]

Is it just how the type checker handles constants? Certainly if I used the constants directly the compiler would be happy to consider them uint32.

[dsymonds]

It still occurs with var flags uint32 = syscall.AF_INET, so that rules out most platform-dependence, I believe.

Aah, but on Linux the typechecker is deducing a type of "uint32" on the RHS. I suspect that it is doing the type deduction in full, more aggressively than golint was expecting. Hmmm.

@adonovan, got any opinions here? I'm using info.TypeOf(v.Values[0]), where v is an *ast.ValueSpec, and finding that go/types is telling me it is a uint32. Is that expected? Is there a way to get the RHS's default type, rather than the type implied by the LHS of the var decl?

[dsymonds]

Yeah, I wonder if we are supposed to notice that the RHS is a constant expression and treat that especially. I'm just a little surprised that info.TypeOf behaves this way.

[nathany]

Simplified:

const num = 123
var flags uint32 = num

and likewise:

const num = 123.0
var flags float32 = num

Given that they are typeless constants, I'm not too surprised. It does seem like some special handling may be needed to prevent this false positive.

Thanks for looking into it.

[adonovan]

For each identifier that refers to a constant, the go/types API reports the effective type of the constant in that context, after convertUntyped() has changed it to the appropriate variable type and truncated the value if necessary. Only the *types.Const object itself holds the untruncated, "untyped", value.

(I find the term "untyped" to be very confusing, especially when discussing var decls without an explicit type, such as var x = "foo".)

I can see how this is not what you want here, but the logic in the typechecker is complex for a reason: the type of the LHS variable can be derived from the RHS constant, e.g.
var s = "foo"
and yet the value of the RHS constant can be derived from the type of the variable on the LHS, e.g.
const tenth = 0.1
var x float32 = tenth
var y float64 = tenth
The type checker reports different values for the two expressions 'tenth' here, because the two floating point types differently approximate the decimal fraction.

[dsymonds]

Thanks. So do you have a recommendation for what we should do here?

[griesemer]

adonovan explained the situation already. The type-checker determines the actual run-time type for constants once it needs them - in this case it determines that those constants must be uint32 (so it can verify that they actually fit). A compiler will also need this information (rather than the untyped value), so it makes sense.

I think the lint warning is too aggressive: Even if repeating the type on the lhs is redundant, it may still make sense for a programmer to repeat it, for readability.

You may want to target specifically more simple cases (var s string = "foo"; var i int = 42; etc.); basically cases where the lhs is constant, and the constants are literals or perhaps constant names declared in the same package. For more complex expressions like this one, or where the constants are declared elsewhere, it may make total sense to repeat the type in my mind.

[nathany]

If this lint is kept, is there a way to determine that the RHS was an untyped constant? via *types.Const?

If so then the LHS would only ever infer int or float64 for numeric types, correct? So anything else should not be considered an issue (int32, int64, uint32, uint64, float32).

Maybe it's not worth holding on to this particular linter though. Hm.

[griesemer]

There's no way at the moment; the expression is given a type via the assignment and the prior type is not exposed - it's unfortunately one of the more complicated parts of the type checker. Perhaps it should - feel free to file an issue as a reminder (but I won't promise that it will be tackled anytime soon).

[dsymonds]

I think I found a semi-ingenious workaround. Since I have the full AST, I can get the types.Scope for the variable declaration, use types.EvalNode to evaluate the RHS expression, and find the original type. It's a little cheeky, but it seems to work fine.

[griesemer]

Ah, yes! I was thinking about a solution along these lines (using types.New) but I forgot that we also have EvalNode, which is there exactly to evaluate arbitrary expressions in a given context (say for a debugger). It's not even cheeky; it's just a bit more work than one would hope to do because the original type for the rhs was determined once before (but than overridden because of the assignment).

[nathany]

👍 Sounds perfect. Thanks David.

[dsymonds]

39135ba partly addresses this, but it appears that types.EvalNode doesn't do cross-package name resolution (e.g. I get undeclared name: syscall) for the original case on this bug. I'll have to dig a bit further to see if I'm doing something wrong.

[nathany]

Is that a GOOS thing from #61 (comment)?

It works here (Mac):

❯ golint
kqueue.go:414:17: should omit type uint32 from declaration of var newFlags; it will be inferred from the right-hand side
kqueue.go:466:17: should omit type uint32 from declaration of var newFlags; it will be inferred from the right-hand side

❯ go get -u github.com/golang/lint/golint

❯ golint

Thanks. 😄

[dsymonds]

I thought it might have been, but then I tried net.IPv4len, and that's in
src/net/ip.go (no build tags there), and the error message is complaining
about the name of the package selector specifically.

[nathany]

Okay. And in case of a package selector error, does it decide not to report the lint?

[griesemer]

If the syscall package wasn't imported in the source it won't find it. EvalNode can only resolve names declared in the environment.

But I'm not sure that's the problem.

• Robert

On Sep 14, 2014, at 6:32 PM, David Symonds notifications@github.com wrote:

39135ba partly addresses this, but it appears that types.EvalNode doesn't do cross-package name resolution (e.g. I get undeclared name: syscall) for the original case on this bug. I'll have to dig a bit further to see if I'm doing something wrong.

—
Reply to this email directly or view it on GitHub.

[nathany]

Oh. Might this be why? 39135ba#diff-b2012451b01c3a63936f8be01c13c3e5R1238

Re-evaluate expr outside of its context to see if it's untyped.

So we need it outside of the context, but still with the same imports?

[dsymonds]

Here's the full file I tested:

package foo
import "syscall"
var flags uint32 = syscall.AF_INET

The typechecker doesn't complain at all during typechecking, so the syscall import is fine. And EvalNode is given the scope of the assignment. If I print scope.LookupParent("syscall"), though, then I get <nil>, which implies there's a problem with the scope I'm using for EvalNode, I believe.

[dsymonds]

Well, this is interesting. I am getting the scope from the flags identifier (the _ast.Ident in the assignment), and asking (_Info).ObjectOffor its scope. However, that turns out to be the package's scope, not the scope of the source file, andscope.NumChildren()is1. If I writescope = scope.Child(0)then I get the file's scope, andscope.LookupParent("syscall") returns something, and types.EvalNode starts behaving correctly.

So, it feels like (*Info).ObjectOf(id).Parent() is behaving incorrectly. It should be returning the file scope, not the package scope. Do you concur, @r5g?

[griesemer]

Nope. This is of the hidden peculiarities of Go's scoping rules.

Imported packages live in the file scope - because it's only visible in the
respective file - which is nested in the package scope (it has to be,
because a package may contain several files and thus file scopes, and they
are all contained within the package scope).

Any other object in the package is declared in the package scope (because
it's visible in all files).

For correct scope, you will need the file scope. The type checker goes out
of its way to evaluate package-level objects in the file scope of the file
in which they are declared.

The file scope can be found via the Info.Scopes map by using the correct
*ast.File. It might be painful to find that one, though. Perhaps go through
all *ast.Files and use ast.Inspect to find the object in the file and then
you know you have the right one. But maybe there's a more direct way. Can
look at this tomorrow.

• Robert

On Sun, Sep 14, 2014 at 9:11 PM, David Symonds notifications@github.com
wrote:

Well, this is interesting. I am getting the scope from the flags
identifier (the _ast.Ident in the assignment), and asking (_Info).ObjectOf
for its scope. However, that turns out to be the package's scope, not the
scope of the source file, and scope.NumChildren() is 1. If I write scope
= scope.Child(0) then I get the file's scope, and
scope.LookupParent("syscall") returns something, and types.EvalNode
starts behaving correctly.

So, it feels like (*Info).ObjectOf is behaving incorrectly. It should be
returning the file scope, not the package scope. Do you concur, @r5g
https://github.com/r5g?

—
Reply to this email directly or view it on GitHub
#61 (comment).

[dsymonds]

I understand the scoping rules. I guess what I don't understand is why the
scope of var x = ... (at top-level) is the package scope rather than the
file scope, but your explanation makes sense from the perspective of things
searching for the identifier.

Perhaps there needs to be a way to map from an ast.Expr to a *types.Scope,
or a map from token.Pos to *types.Scope? if I have the *ast.ValueSpec,
there should be a reasonably direct way to find the *types.Scope for it.
Perhaps you could write some helper in the types package that does the
Info.Scopes searching you suggest?

Thanks for looking at it.

[griesemer]

The scope of 'var x = ...' at the top-level has to be the package scope
because is is declared in the package scope, not the file scope. The file
scope is nested within the package scope.

That is, when looking something up at the package-level, it always has to
be done starting within the correct file scope, but things at the top-level
are declared in the the surrounding package scope. (But, say, a function
scope's parent scope is the surrounding file scope even though the function
itself is declared in the package scpoe. It's a real oddity.)

The type checker maintains the file scope info but then throws it away. I
suppose I could export it via an additional map. Perhaps you can file an
issue and we might get it in despite the 1.4 freeze (go.tools gets a couple
of weeks extra).

• Robert

On Sun, Sep 14, 2014 at 9:36 PM, David Symonds notifications@github.com
wrote:

I understand the scoping rules. I guess what I don't understand is why the
scope of var x = ... (at top-level) is the package scope rather than the
file scope, but your explanation makes sense from the perspective of things
searching for the identifier.

Perhaps there needs to be a way to map from an ast.Expr to a *types.Scope,
or a map from token.Pos to *types.Scope? if I have the *ast.ValueSpec,
there should be a reasonably direct way to find the *types.Scope for it.
Perhaps you could write some helper in the types package that does the
Info.Scopes searching you suggest?

Thanks for looking at it.

—
Reply to this email directly or view it on GitHub
#61 (comment).

[dsymonds]

https://code.google.com/p/go/issues/detail?id=8730 filed.