code review 10748044: go.tools/go/types: implement Eval and EvalNode

go.tools/go/types: implement New, Eval and EvalNode

Eval and EvalNode permit the evaluation of an expression
or type literal string (or AST node in case of EvalNode)
within a given context (package and scope).

Also:
- track nested (children) scopes for all scopes
- provide a mechanism to iterate over nested scopes
- permit recursive printing of scopes

TODO: more tests

[gri, 2013-07-12 01:13:23]

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

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

[gri, 2013-07-12 04:04:08]

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

Please take another look.

[adonovan, 2013-07-12 14:28:58]

https://codereview.appspot.com/10748044/diff/30001/go/types/eval.go
File go/types/eval.go (right):

https://codereview.appspot.com/10748044/diff/30001/go/types/eval.go#newcode23
go/types/eval.go:23: // New is a convenience function to create a new type from
a given
Don't forget to update this if/when you remove the other constructors.

https://codereview.appspot.com/10748044/diff/30001/go/types/eval.go#newcode57
go/types/eval.go:57: func Eval(pkg *Package, scope *Scope, str string) (typ
Type, val exact.Value, err error) {
We need to think carefully about FileSets.  We really want to avoid the
situation where the user has to worry about the question "which fileset does
this pos belong to?" and I think that means we have to be explicit about
FileSets everywhere.

Initially I thought we could worry about this later because we don't create new
types.Objects yet---but in fact we do.   Consider:
   "interface { String() string }"
   "struct { x int }" 
String and x are objects, with positions.  Those positions should be correct,
even if they're not real files. I'd rather see this in an error message from a
tool:

  <internal>:1:13: error: problem with method String

than spend time wondering why an error is reported in the middle of an arbitrary
source file.

So I think the API should look more like this:

  var fset *token.FileSet = ...
  var texpr ast.Expr
  texpr, err := types.ParseType(fset, "internal", "interface{ String() string
}")
  
  var pkg *types.Package
  var scope *types.Scope
  var typ types.Type
  typ, err := types.ResolveType(texpr, pkg, scope)

Since this package contains not just the type checker but also the name
resolver, in the future I think it's likely we'll want additional functions:

  ParseExpr, ResolveExpr
  ParseStmt, ResolveStmt
  etc.

(Consider a debugger that lets you evaluate an expression within the scope of
current breakpoint.)

https://codereview.appspot.com/10748044/diff/30001/go/types/eval.go#newcode57
go/types/eval.go:57: func Eval(pkg *Package, scope *Scope, str string) (typ
Type, val exact.Value, err error) {
Avoid the name "Eval" since it connotes a function from string to value.

https://codereview.appspot.com/10748044/diff/30001/go/types/eval.go#newcode58
go/types/eval.go:58: node, err := parser.ParseExpr(str)
That parser.ParseExpr doesn't accept a *token.FileSet seems like a mistake.  Can
we not add another function to that package?

https://codereview.appspot.com/10748044/diff/30001/go/types/eval_test.go
File go/types/eval_test.go (right):

https://codereview.appspot.com/10748044/diff/30001/go/types/eval_test.go#newc...
go/types/eval_test.go:11: func testEval(t *testing.T, pkg *Package, scope
*Scope, str string, typ Type, typStr, valStr string) {
A docstring would be good since the use of parameters is somewhat subtle.

https://codereview.appspot.com/10748044/diff/30001/go/types/scope.go
File go/types/scope.go (right):

https://codereview.appspot.com/10748044/diff/30001/go/types/scope.go#newcode49
go/types/scope.go:49: func (s *Scope) Node() ast.Node {
TODO(gri): consider whether we want to pin ASTs in memory.
Perhaps this could be expressed like the other callbacks (or whatever they
become, e.g. a container of results separable from the types themselves)

https://codereview.appspot.com/10748044/diff/30001/go/types/scope.go#newcode158
go/types/scope.go:158: func (s *Scope) WriteTo(w io.Writer, n int, recurse bool)
{
Is there an established convention in my code for naming such methods?  I've
used DumpTo but I'm not satisfied with it; WriteTo seems better.

https://codereview.appspot.com/10748044/diff/30001/go/types/scope.go#newcode168
go/types/scope.go:168: indn1 := indn + ind
Or just:

fmt.Fprintf(w, "%s%s%s\n", indn, ind, obj)

https://codereview.appspot.com/10748044/diff/30001/go/types/types_test.go
File go/types/types_test.go (left):

https://codereview.appspot.com/10748044/diff/30001/go/types/types_test.go#old...
go/types/types_test.go:59: elems []T
What happens to this input now?

https://codereview.appspot.com/10748044/diff/30001/go/types/types_test.go
File go/types/types_test.go (right):

https://codereview.appspot.com/10748044/diff/30001/go/types/types_test.go#new...
go/types/types_test.go:37: var testTypes = []testEntry{
// These entries are also inputs to testEval in eval.go

[gri, 2013-07-12 16:38:59]

PTAL

- moved str to first position in argument list
- fixed fset computation (missing SetLinesForContent)

https://codereview.appspot.com/10748044/diff/30001/go/types/eval.go
File go/types/eval.go (right):

https://codereview.appspot.com/10748044/diff/30001/go/types/eval.go#newcode23
go/types/eval.go:23: // New is a convenience function to create a new type from
a given
On 2013/07/12 14:28:58, adonovan wrote:
> Don't forget to update this if/when you remove the other constructors.

I meant convenience in the sense that one doesn't need to call Eval. Rephrased.

https://codereview.appspot.com/10748044/diff/30001/go/types/eval.go#newcode57
go/types/eval.go:57: func Eval(pkg *Package, scope *Scope, str string) (typ
Type, val exact.Value, err error) {
On 2013/07/12 14:28:58, adonovan wrote:
> Avoid the name "Eval" since it connotes a function from string to value.

I really like Eval here, because that is exactly what it does: it's a function
from a string to a value, where the value is the type. That's the whole point of
the type checker - evaluating to types.

https://codereview.appspot.com/10748044/diff/30001/go/types/eval.go#newcode57
go/types/eval.go:57: func Eval(pkg *Package, scope *Scope, str string) (typ
Type, val exact.Value, err error) {
On 2013/07/12 14:28:58, adonovan wrote:
> We need to think carefully about FileSets.  We really want to avoid the
> situation where the user has to worry about the question "which fileset does
> this pos belong to?" and I think that means we have to be explicit about
> FileSets everywhere.
> 
> Initially I thought we could worry about this later because we don't create
new
> types.Objects yet---but in fact we do.   Consider:
>    "interface { String() string }"
>    "struct { x int }" 
> String and x are objects, with positions.  Those positions should be correct,
> even if they're not real files. I'd rather see this in an error message from a
> tool:
> 
>   <internal>:1:13: error: problem with method String
> 
> than spend time wondering why an error is reported in the middle of an
arbitrary
> source file.
> 
> So I think the API should look more like this:
> 
>   var fset *token.FileSet = ...
>   var texpr ast.Expr
>   texpr, err := types.ParseType(fset, "internal", "interface{ String() string
> }")
>   
>   var pkg *types.Package
>   var scope *types.Scope
>   var typ types.Type
>   typ, err := types.ResolveType(texpr, pkg, scope)
> 
> Since this package contains not just the type checker but also the name
> resolver, in the future I think it's likely we'll want additional functions:
> 
>   ParseExpr, ResolveExpr
>   ParseStmt, ResolveStmt
>   etc.
> 
> (Consider a debugger that lets you evaluate an expression within the scope of
> current breakpoint.)

The whole point of New and Eval is to be easy to use. Error messages will write
position information pertinent to the input string.

If a FileSet is needed, EvalNode is the function to use. It can grow in terms of
the kind of nodes it might accept in the future (statements, declarations,
etc.). We can change the argument to ast.Node if needed (API is not frozen).

I don't want to add ParseX functionality here - and also not in the parser, for
now.

[adonovan, 2013-07-12 17:58:33]

LGTM

https://codereview.appspot.com/10748044/diff/44001/go/types/eval.go
File go/types/eval.go (right):

https://codereview.appspot.com/10748044/diff/44001/go/types/eval.go#newcode22
go/types/eval.go:22: // and fields in types in Universe scope.
Please document that using any of these functions to parse types that define
names, such as String, x below:

  interface{ String() string }
  struct { x int }

will contain garbage position info for their Objects.

https://codereview.appspot.com/10748044/diff/44001/go/types/eval.go#newcode38
go/types/eval.go:38: // agrees that it's the best name for the functionality
provided.
That's putting it mildly. :)

'eval' is universally understood to mean dynamic evaluation of an program
expression or statements, not static analysis (name resolution, constant
folding, type checking).  Evaluation contains the root of the word "value",
which has a technical meaning different from "type".  Indeed, the signature of
this function also contains "val exact.Value", so the potential for confusion is
very great. 

I would prefer a name like ResolveExpr or CheckExpr.

In the interests of progres, LG for now, but we should discuss with Rob.

[gri, 2013-07-12 18:03:02]

https://codereview.appspot.com/10748044/diff/44001/go/types/eval.go
File go/types/eval.go (right):

https://codereview.appspot.com/10748044/diff/44001/go/types/eval.go#newcode22
go/types/eval.go:22: // and fields in types in Universe scope.
On 2013/07/12 17:58:33, adonovan wrote:
> Please document that using any of these functions to parse types that define
> names, such as String, x below:
> 
>   interface{ String() string }
>   struct { x int }
> 
> will contain garbage position info for their Objects.

Done.

https://codereview.appspot.com/10748044/diff/44001/go/types/eval.go#newcode38
go/types/eval.go:38: // agrees that it's the best name for the functionality
provided.
On 2013/07/12 17:58:33, adonovan wrote:
> That's putting it mildly. :)
> 
> 'eval' is universally understood to mean dynamic evaluation of an program
> expression or statements, not static analysis (name resolution, constant
> folding, type checking).  Evaluation contains the root of the word "value",
> which has a technical meaning different from "type".  Indeed, the signature of
> this function also contains "val exact.Value", so the potential for confusion
is
> very great. 
> 
> I would prefer a name like ResolveExpr or CheckExpr.
> 
> In the interests of progres, LG for now, but we should discuss with Rob.

ACK

[gri, 2013-07-12 18:03:38]

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

go.tools/go/types: implement New, Eval and EvalNode

Eval and EvalNode permit the evaluation of an expression
or type literal string (or AST node in case of EvalNode)
within a given context (package and scope).

Also:
- track nested (children) scopes for all scopes
- provide a mechanism to iterate over nested scopes
- permit recursive printing of scopes

TODO: more tests

R=adonovan
CC=golang-dev
https://codereview.appspot.com/10748044