code review 110880043: go.tools/go/types: expose operand modes

go.tools/go/types: provide TypeAndValue predicates

[gri, 2014-07-07 23:03:41]

Hello adonovan@google.com, pcc@google.com (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.tools

[adonovan, 2014-07-08 07:44:19]

Ship it!

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode126
go/types/api.go:126: type OperandMode int
Mode?

or ExprMode?  (since it's a property of an ast.Expr)

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode132
go/types/api.go:132: NoValue  = OperandMode(novalue)  // operand represents no
value (result of a function call w/o result)
It's not really an operand at all.

// result of a function call without result

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode133
go/types/api.go:133: BuiltIn  = OperandMode(builtin)  // operand is a built-in
function
Sometimes I wish that Go had a little bit of "stuttering" in the use of
enum-style constants, so that external clients would have to mention "Mode" when
they refer to BuiltIn, Constant, or Variable, to avoid confusion with similarly
named (and more important) elements of the API.

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode135
go/types/api.go:135: Constant = OperandMode(constant) // operand is a constant;
the operand's typ is a Basic type
This comment refers to an unexported field.

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode137
go/types/api.go:137: MapIndex = OperandMode(mapindex) // operand is a map index
expression (acts like a variable on lhs, commaok on rhs of an assignment)
"LHS, CommaOK on RHS"

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode139
go/types/api.go:139: Commaok  = OperandMode(commaok)  // like value, but operand
may be used in a comma,ok expression
CommaOK = ...

"in a value,ok expression"

https://codereview.appspot.com/110880043/diff/80001/go/types/check.go
File go/types/check.go (right):

https://codereview.appspot.com/110880043/diff/80001/go/types/check.go#newcode23
go/types/check.go:23: // exprInfo stores information for an untyped expression.
"information about"

https://codereview.appspot.com/110880043/diff/80001/go/types/check.go#newcode26
go/types/check.go:26: mode  operandMode
Consider using a type smaller than int to represent the (consolidated)
operandMode so that it packs better.

https://codereview.appspot.com/110880043/diff/80001/go/types/check.go#newcode254
go/types/check.go:254: assert(val != nil && isConstType(typ))
Two separate assertions would lead to more informative failures.

[adonovan, 2014-07-08 10:51:04]

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode133
go/types/api.go:133: BuiltIn  = OperandMode(builtin)  // operand is a built-in
function
Do we actually want to expose BuiltIn?   I believe this can only occur when the
Expr is an Ident, in which case Uses[expr] provides the *Builtin object and
Types[expr] has no value.  I may be wrong though.

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode140
go/types/api.go:140: )
Another approach we should consider is whether to expose only a small set of
predicates to the client, instead of a larger set of modes.  The predicates
could be methods of TypeAndValue; we could continue to use the existing modes
internally.

This has the benefits of fewer names in the package scope, a better place to put
documentation, and potentially a more regular API based on behaviors, not
representations, and avoids the more obscure cases like
novalue/mapindex/commaok.

Something like this:

// IsType reports whether the ast.Expr is a type.
// true for typeexpr
func (tv TypeAndValue) IsType() bool

// IsExpr reports whether the ast.Expr denotes some kind of value.
// true for novalue, constant, variable, mapindex, value, commaok
func (tv TypeAndValue) IsExpr() bool

// Addressable reports whether the expression is a variable with an address.
// (=>IsExpr)
// true for variable
func (tv TypeAndValue) Addressable() bool

// Assignable reports whether the expression may appear on the LHS of
assignment.
// (=>IsExpr)
// true for variable, mapindex
func (tv TypeAndValue) Assignable() bool

// HasOK reports whether the expression may return a second (ok bool) result,
// and thus may appear on the RHS of an assignment such as x, ok := expr.
// (=>IsExpr)
// true for mapindex and commaok
func (tv TypeAndValue) HasOK() bool

Clients can reconstruct all the original cases from these predicates should they
need to:

if tv.IsType() {
    return typeexpr
} 
if tv.Value != nil {
    return constant
}
if tv.Addressable() {
    return variable
}
if tv.Assignable() {
    return mapindex
}
if tv.Type is empty tuple {
    return novalue
}
if tv.HasOK() {
    return commaok
}
return value

Just a thought.

[gri, 2014-07-08 22:29:17]

PTAL

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode126
go/types/api.go:126: type OperandMode int
On 2014/07/08 07:44:18, adonovan wrote:
> Mode?
> 
> or ExprMode?  (since it's a property of an ast.Expr)

Mode is too generic. The expressions are really operands, and so if we like the
info, I prefer to call it OperandMode, like it's called internally now (and
eventually get rid of the internal decl).

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode132
go/types/api.go:132: NoValue  = OperandMode(novalue)  // operand represents no
value (result of a function call w/o result)
On 2014/07/08 07:44:18, adonovan wrote:
> It's not really an operand at all.
> 
> // result of a function call without result

Done.

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode133
go/types/api.go:133: BuiltIn  = OperandMode(builtin)  // operand is a built-in
function
On 2014/07/08 07:44:19, adonovan wrote:
> Sometimes I wish that Go had a little bit of "stuttering" in the use of
> enum-style constants, so that external clients would have to mention "Mode"
when
> they refer to BuiltIn, Constant, or Variable, to avoid confusion with
similarly
> named (and more important) elements of the API.

ACK. It can always be done via convention:

NoValueMode or ModeNoValue, etc.

I've tried to avoid that, though. I'd have to change it pretty pervasively to
keep in the same style.

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode133
go/types/api.go:133: BuiltIn  = OperandMode(builtin)  // operand is a built-in
function
On 2014/07/08 10:51:04, adonovan wrote:
> Do we actually want to expose BuiltIn?   I believe this can only occur when
the
> Expr is an Ident, in which case Uses[expr] provides the *Builtin object and
> Types[expr] has no value.  I may be wrong though.

At the moment we do. You're right that this is inconsistent and perhaps should
be fixed. Added TODO.

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode135
go/types/api.go:135: Constant = OperandMode(constant) // operand is a constant;
the operand's typ is a Basic type
On 2014/07/08 07:44:19, adonovan wrote:
> This comment refers to an unexported field.

Done.

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode139
go/types/api.go:139: Commaok  = OperandMode(commaok)  // like value, but operand
may be used in a comma,ok expression
On 2014/07/08 07:44:18, adonovan wrote:
> CommaOK = ...
> 
> "in a value,ok expression"

Done.

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode140
go/types/api.go:140: )
On 2014/07/08 10:51:04, adonovan wrote:
> Another approach we should consider is whether to expose only a small set of
> predicates to the client, instead of a larger set of modes.  The predicates
> could be methods of TypeAndValue; we could continue to use the existing modes
> internally.
> 
> This has the benefits of fewer names in the package scope, a better place to
put
> documentation, and potentially a more regular API based on behaviors, not
> representations, and avoids the more obscure cases like
> novalue/mapindex/commaok.
> 
> Something like this:
> 
> // IsType reports whether the ast.Expr is a type.
> // true for typeexpr
> func (tv TypeAndValue) IsType() bool
> 
> // IsExpr reports whether the ast.Expr denotes some kind of value.
> // true for novalue, constant, variable, mapindex, value, commaok
> func (tv TypeAndValue) IsExpr() bool
> 
> // Addressable reports whether the expression is a variable with an address.
> // (=>IsExpr)
> // true for variable
> func (tv TypeAndValue) Addressable() bool
> 
> // Assignable reports whether the expression may appear on the LHS of
> assignment.
> // (=>IsExpr)
> // true for variable, mapindex
> func (tv TypeAndValue) Assignable() bool
> 
> // HasOK reports whether the expression may return a second (ok bool) result,
> // and thus may appear on the RHS of an assignment such as x, ok := expr.
> // (=>IsExpr)
> // true for mapindex and commaok
> func (tv TypeAndValue) HasOK() bool
> 
> Clients can reconstruct all the original cases from these predicates should
they
> need to:
> 
> if tv.IsType() {
>     return typeexpr
> } 
> if tv.Value != nil {
>     return constant
> }
> if tv.Addressable() {
>     return variable
> }
> if tv.Assignable() {
>     return mapindex
> }
> if tv.Type is empty tuple {
>     return novalue
> }
> if tv.HasOK() {
>     return commaok
> }
> return value
> 
> Just a thought.

This would be fine with me as well. I think it depends on how it's used. If you
mostly check one or perhaps two accessors per use site, accessors work well. If
there's often switching depending on the mode, exposing the mode might be more
convenient.

More input on that?

https://codereview.appspot.com/110880043/diff/80001/go/types/check.go
File go/types/check.go (right):

https://codereview.appspot.com/110880043/diff/80001/go/types/check.go#newcode23
go/types/check.go:23: // exprInfo stores information for an untyped expression.
On 2014/07/08 07:44:19, adonovan wrote:
> "information about"

Done.

https://codereview.appspot.com/110880043/diff/80001/go/types/check.go#newcode26
go/types/check.go:26: mode  operandMode
On 2014/07/08 07:44:19, adonovan wrote:
> Consider using a type smaller than int to represent the (consolidated)
> operandMode so that it packs better.

Done.

https://codereview.appspot.com/110880043/diff/80001/go/types/check.go#newcode254
go/types/check.go:254: assert(val != nil && isConstType(typ))
On 2014/07/08 07:44:19, adonovan wrote:
> Two separate assertions would lead to more informative failures.

Done.

[adonovan, 2014-07-08 22:54:41]

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode133
go/types/api.go:133: BuiltIn  = OperandMode(builtin)  // operand is a built-in
function
On 2014/07/08 22:29:16, gri wrote:
> On 2014/07/08 10:51:04, adonovan wrote:
> > Do we actually want to expose BuiltIn?   I believe this can only occur when
> the
> > Expr is an Ident, in which case Uses[expr] provides the *Builtin object and
> > Types[expr] has no value.  I may be wrong though.
> 
> At the moment we do. You're right that this is inconsistent and perhaps should
> be fixed. Added TODO.

Ah, I remember why we do it this way: if we report Builtins only via Uses, then
the underlying Object is the same for all uses, and thus must have the same
type, but if we report it via Types, we can ascribe a different Signature to
each call to print, append, len, etc.

It should probably be documented.

https://codereview.appspot.com/110880043/diff/80001/go/types/api.go#newcode140
go/types/api.go:140: )
On 2014/07/08 22:29:17, gri wrote:
> On 2014/07/08 10:51:04, adonovan wrote:
> > Another approach we should consider is whether to expose only a small set of
> > predicates to the client, instead of a larger set of modes.  The predicates
> > could be methods of TypeAndValue; we could continue to use the existing
modes
> > internally.
> > 
> > This has the benefits of fewer names in the package scope, a better place to
> put
> > documentation, and potentially a more regular API based on behaviors, not
> > representations, and avoids the more obscure cases like
> > novalue/mapindex/commaok.
> > 
> > Something like this:
> > 
> > // IsType reports whether the ast.Expr is a type.
> > // true for typeexpr
> > func (tv TypeAndValue) IsType() bool
> > 
> > // IsExpr reports whether the ast.Expr denotes some kind of value.
> > // true for novalue, constant, variable, mapindex, value, commaok
> > func (tv TypeAndValue) IsExpr() bool
> > 
> > // Addressable reports whether the expression is a variable with an address.
> > // (=>IsExpr)
> > // true for variable
> > func (tv TypeAndValue) Addressable() bool
> > 
> > // Assignable reports whether the expression may appear on the LHS of
> > assignment.
> > // (=>IsExpr)
> > // true for variable, mapindex
> > func (tv TypeAndValue) Assignable() bool
> > 
> > // HasOK reports whether the expression may return a second (ok bool)
result,
> > // and thus may appear on the RHS of an assignment such as x, ok := expr.
> > // (=>IsExpr)
> > // true for mapindex and commaok
> > func (tv TypeAndValue) HasOK() bool
> > 
> > Clients can reconstruct all the original cases from these predicates should
> they
> > need to:
> > 
> > if tv.IsType() {
> >     return typeexpr
> > } 
> > if tv.Value != nil {
> >     return constant
> > }
> > if tv.Addressable() {
> >     return variable
> > }
> > if tv.Assignable() {
> >     return mapindex
> > }
> > if tv.Type is empty tuple {
> >     return novalue
> > }
> > if tv.HasOK() {
> >     return commaok
> > }
> > return value
> > 
> > Just a thought.
> 
> This would be fine with me as well. I think it depends on how it's used. If
you
> mostly check one or perhaps two accessors per use site, accessors work well.
If
> there's often switching depending on the mode, exposing the mode might be more
> convenient.
> 
> More input on that?

I think calling a specific accessor would be more common; certainly that is true
within go/ssa.

[gri, 2014-07-09 00:22:27]

PTAL

[adonovan, 2014-07-09 06:47:40]

Nice, I prefer this way.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode130
go/types/api.go:130: // expressions, their values. Invalid expressions are
omitted.
> Invalid expressions are omitted.

It's worth noting that the zero value of TypeAndValue has Type==nil, is not a
constant, and has mode==invalid, so answers false to all the new mode
predicates.  So "if Types[expr].Value == nil" is a save test for constants even
in the presence of invalid expressions, for example.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode206
go/types/api.go:206: // corresponding expression.
", and mode information such as whether it is a type, builtin or value, and
whether the value is addressable."

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode238
go/types/api.go:238: // IsVariable reports whether the corresponding expression
is a variable
This predicate actually reports whether the corresponding expression is an
addressable location, such as a variable, or a subelement of a variable of
aggregate type (not just array elements but struct fields too).  

Addressable seems like the right name.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode245
go/types/api.go:245: // Addressable reports whether the corresponding expression
is addressable;
The spec says that variables are addressable but map index expressions are not. 
Map index expressions are, however, permitted on the lhs of an assignment.

So Assignable or IsLValue or isLHS seems like the right name.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode251
go/types/api.go:251: // HasOk reports whether the corresponding expression is
used on the rhs
HasOK

[adonovan, 2014-07-09 09:45:04]

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode227
go/types/api.go:227: // IsValue reports whether the corresponding expression is
a value.
Another thing: references to 'nil' are counted as values, which makes sense
because they "can be stored in variables".  However, they are not constants,
which also makes sense according to the spec.

But:
(a) the type of references to nil is "untyped nil", and
(b) the only way to know that a TypeAndValue denotes a reference to nil is to
see if the syntax is an Ident and the Uses[e] contains a *Nil.

This deserves some documentation at least.

[gri, 2014-07-09 23:21:05]

PTAL

Everything addressed. Added API tests.

Note that Types actually contains identifiers (that are not in a declaration) as
well. That is, identifiers are actually reported in both, Uses and Types.

Should we change that? (I'd make this independent of this CL)

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode130
go/types/api.go:130: // expressions, their values. Invalid expressions are
omitted.
On 2014/07/09 06:47:40, adonovan wrote:
> > Invalid expressions are omitted.
> 
> It's worth noting that the zero value of TypeAndValue has Type==nil, is not a
> constant, and has mode==invalid, so answers false to all the new mode
> predicates.  So "if Types[expr].Value == nil" is a save test for constants
even
> in the presence of invalid expressions, for example.

Documented

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode135
go/types/api.go:135: // is invalid. All other identifiers are recorded via the
Defs
Note that identifiers (that are not in declarations) actually show up in this
map. We can avoid that if we want. Should we?

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode206
go/types/api.go:206: // corresponding expression.
On 2014/07/09 06:47:40, adonovan wrote:
> ", and mode information such as whether it is a type, builtin or value, and
> whether the value is addressable."

Done.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode227
go/types/api.go:227: // IsValue reports whether the corresponding expression is
a value.
On 2014/07/09 09:45:03, adonovan wrote:
> Another thing: references to 'nil' are counted as values, which makes sense
> because they "can be stored in variables".  However, they are not constants,
> which also makes sense according to the spec.
> 
> But:
> (a) the type of references to nil is "untyped nil", and
> (b) the only way to know that a TypeAndValue denotes a reference to nil is to
> see if the syntax is an Ident and the Uses[e] contains a *Nil.
> 
> This deserves some documentation at least.

Added IsNil predicate.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode238
go/types/api.go:238: // IsVariable reports whether the corresponding expression
is a variable
On 2014/07/09 06:47:40, adonovan wrote:
> This predicate actually reports whether the corresponding expression is an
> addressable location, such as a variable, or a subelement of a variable of
> aggregate type (not just array elements but struct fields too).  
> 
> Addressable seems like the right name.

I've been debating this with r for years. To me, a variable _is_ a (typed)
storage location where I can store a value. That includes variables denoted by
names, indexed array locations, fields, parameters, etc. A variable is to me
what is called "lvalue" in C.

That said, I'm calling it addressable now to match the spec.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode245
go/types/api.go:245: // Addressable reports whether the corresponding expression
is addressable;
On 2014/07/09 06:47:40, adonovan wrote:
> The spec says that variables are addressable but map index expressions are
not. 
> Map index expressions are, however, permitted on the lhs of an assignment.
> 
> So Assignable or IsLValue or isLHS seems like the right name.

Done.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode251
go/types/api.go:251: // HasOk reports whether the corresponding expression is
used on the rhs
On 2014/07/09 06:47:40, adonovan wrote:
> HasOK

I prefer HasOk.

[adonovan, 2014-07-10 09:52:47]

> Note that Types actually contains identifiers (that are not in a declaration)
as
> well. That is, identifiers are actually reported in both, Uses and Types.
> Should we change that? (I'd make this independent of this CL)

I think that's fine.  They are two distinct relations (types of expressions, and
resolution of identifiers).

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode135
go/types/api.go:135: // is invalid. All other identifiers are recorded via the
Defs
On 2014/07/09 23:21:05, gri wrote:
> Note that identifiers (that are not in declarations) actually show up in this
> map. We can avoid that if we want. Should we?

It's probably better for clients if it appears in both maps; they are
expressions, after all.
The only reason not to is space.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode238
go/types/api.go:238: // IsVariable reports whether the corresponding expression
is a variable
On 2014/07/09 23:21:04, gri wrote:
> On 2014/07/09 06:47:40, adonovan wrote:
> > This predicate actually reports whether the corresponding expression is an
> > addressable location, such as a variable, or a subelement of a variable of
> > aggregate type (not just array elements but struct fields too).  
> > 
> > Addressable seems like the right name.
> 
> I've been debating this with r for years. To me, a variable _is_ a (typed)
> storage location where I can store a value. That includes variables denoted by
> names, indexed array locations, fields, parameters, etc. A variable is to me
> what is called "lvalue" in C.
> 
> That said, I'm calling it addressable now to match the spec.

I agree totally, but I thought the spec used "variable" to mean "thing declared
with var", but on closer inspection it uses it fairly consistently in the way
you intended.  I still think Addressable might be a better name here given the
existing use of "Var" in go/types.

https://codereview.appspot.com/110880043/diff/160001/go/types/api.go#newcode251
go/types/api.go:251: // HasOk reports whether the corresponding expression is
used on the rhs
On 2014/07/09 23:21:05, gri wrote:
> On 2014/07/09 06:47:40, adonovan wrote:
> > HasOK
> 
> I prefer HasOk.

I was just paraphrasing
https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Initialisms .

https://codereview.appspot.com/110880043/diff/240001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/240001/go/types/api.go#newcode212
go/types/api.go:212: // IsVoid reports wether the corresponding expression
"whether"

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go
File go/types/api_test.go (right):

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:286: func TestPredicatesInfo(t *testing.T) {
Nice.

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:293: {`package n0; func f() { f() }`, `f()`, `void`},
Is the only difference between "void" and "a value whose type is the empty
tuple" that you can't do call chaining f(g()) for void functions g?  "void"
barely seems worth distinguishing.

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:332: }
Perhaps add some "not found" tests:

- ast.Ident not in an expression context, e.g. f in func f()
- y in switch y := x.(type)       (I think?)

[gri, 2014-07-10 17:15:25]

PTAL

https://codereview.appspot.com/110880043/diff/240001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/110880043/diff/240001/go/types/api.go#newcode212
go/types/api.go:212: // IsVoid reports wether the corresponding expression
On 2014/07/10 09:52:46, adonovan wrote:
> "whether"

Done.

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go
File go/types/api_test.go (right):

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:286: func TestPredicatesInfo(t *testing.T) {
On 2014/07/10 09:52:46, adonovan wrote:
> Nice.

ack

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:293: {`package n0; func f() { f() }`, `f()`, `void`},
On 2014/07/10 09:52:46, adonovan wrote:
> Is the only difference between "void" and "a value whose type is the empty
> tuple" that you can't do call chaining f(g()) for void functions g?  "void"
> barely seems worth distinguishing.

They are the same. A "void" expression has no value and is not a type. Its type
is the empty tuple.

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:332: }
On 2014/07/10 09:52:46, adonovan wrote:
> Perhaps add some "not found" tests:
> 
> - ast.Ident not in an expression context, e.g. f in func f()
> - y in switch y := x.(type)       (I think?)
> 

Done.

The later (switch) requires that y be used at least once, and so it's found.

[adonovan, 2014-07-10 23:40:26]

LGTM

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go
File go/types/api_test.go (right):

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:293: {`package n0; func f() { f() }`, `f()`, `void`},
On 2014/07/10 17:15:24, gri wrote:
> On 2014/07/10 09:52:46, adonovan wrote:
> > Is the only difference between "void" and "a value whose type is the empty
> > tuple" that you can't do call chaining f(g()) for void functions g?  "void"
> > barely seems worth distinguishing.
> 
> They are the same. A "void" expression has no value and is not a type. Its
type
> is the empty tuple.

My question is really: why does "void" have it's own operandMode type?
It could be represented as mode==value && type is empty tuple; and it may not
warrant an IsVoid predicate.

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:332: }
On 2014/07/10 17:15:24, gri wrote:
> On 2014/07/10 09:52:46, adonovan wrote:
> > Perhaps add some "not found" tests:
> > 
> > - ast.Ident not in an expression context, e.g. f in func f()
> > - y in switch y := x.(type)       (I think?)
> > 
> 
> Done.
> 
> The later (switch) requires that y be used at least once, and so it's found.

Never mind, I was confused about the typeswitch objects that appear in
'Implicits'.

[gri, 2014-07-10 23:57:25]

FYI.

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go
File go/types/api_test.go (right):

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:293: {`package n0; func f() { f() }`, `f()`, `void`},
On 2014/07/10 23:40:26, adonovan wrote:
> On 2014/07/10 17:15:24, gri wrote:
> > On 2014/07/10 09:52:46, adonovan wrote:
> > > Is the only difference between "void" and "a value whose type is the empty
> > > tuple" that you can't do call chaining f(g()) for void functions g? 
"void"
> > > barely seems worth distinguishing.
> > 
> > They are the same. A "void" expression has no value and is not a type. Its
> type
> > is the empty tuple.
> 
> My question is really: why does "void" have it's own operandMode type?
> It could be represented as mode==value && type is empty tuple; and it may not
> warrant an IsVoid predicate.

Fair enough. I think it depends on how those scenarios play out in practical
use. I'm fine either way.
Added a TODO.

https://codereview.appspot.com/110880043/diff/240001/go/types/api_test.go#new...
go/types/api_test.go:332: }
On 2014/07/10 23:40:26, adonovan wrote:
> On 2014/07/10 17:15:24, gri wrote:
> > On 2014/07/10 09:52:46, adonovan wrote:
> > > Perhaps add some "not found" tests:
> > > 
> > > - ast.Ident not in an expression context, e.g. f in func f()
> > > - y in switch y := x.(type)       (I think?)
> > > 
> > 
> > Done.
> > 
> > The later (switch) requires that y be used at least once, and so it's found.
> 
> Never mind, I was confused about the typeswitch objects that appear in
> 'Implicits'.

ack

[gri, 2014-07-10 23:59:53]

*** Submitted as
https://code.google.com/p/go/source/detail?r=1094193fa120&repo=tools ***

go.tools/go/types: provide TypeAndValue predicates

LGTM=adonovan
R=adonovan, pcc
CC=golang-codereviews
https://codereview.appspot.com/110880043