code review 107160049: go.tools/go/loader: Add Program.FilePath convenience me...

go.tools/go/loader: Add Program.FilePath convenience method for getting the full path of a source file.

[gmk, 2014-06-18 13:11:58]

Hello golang-codereviews@googlegroups.com,

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

[gmk, 2014-06-23 11:05:20]

added gri and adonovan as reviewers

[gri, 2014-06-23 22:12:28]

LGTM

[gri, 2014-06-23 22:13:10]

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

go.tools/go/loader: Add Program.FilePath convenience method for getting the full
path of a source file.

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

Committer: Robert Griesemer <gri@golang.org>

[adonovan, 2014-07-07 09:51:03]

On 2014/06/23 22:13:10, gri wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=9abbb23a14f6&repo=tools ***
> 
> go.tools/go/loader: Add Program.FilePath convenience method for getting the
full
> path of a source file.
> 
> LGTM=gri
> R=gri, adonovan
> CC=golang-codereviews
> https://codereview.appspot.com/107160049
> 
> Committer: Robert Griesemer <mailto:gri@golang.org>

I'm bewildered by this change.  The new function is not called anywhere in this
repo, nor is it covered by tests, nor does it have anything to do with
go/loader.  If this utility method belongs in any package, that package is
astutil, but I'm still not convinced.

Please revert.

[gmk, 2014-07-07 10:48:03]

On 2014/07/07 09:51:03, adonovan wrote:
> I'm bewildered by this change.

I'm bewildered by your bewilderment.  :-)  Let's hope clarity be nigh.

> The new function is not called anywhere in this
> repo

Little did you know, there are packages outside this repo that use go/loader.

> , nor is it covered by tests

Just like PackageInfo.{ObjectOf, TypeOf, VarOf, TypeCaseVar, and ImportSpecPkg},
it is a trivial function that hardly needs tests.

> , nor does it have anything to do with
> go/loader.  If this utility method belongs in any package, that package is
> astutil, but I'm still not convinced.

It has as much to do with go/loader as the other convenience methods listed
above.  Maybe they should all be deleted, but I don't think so.

When you load a program using Config.Import, the only way to find the source
file paths is via Program.Fset.  The expression
"prog.Fset.File(file.Package).Name()" didn't read very clearly, so I
encapsulated it.  It seemed to me that many clients would be interested in this
method -- for example if they want to indicate the source file where an object
was defined, or to rewrite a source file (in the case of a refactoring tool,
which is my use case) -- so I shared it.

I considered changing PackageInfo.Files from a slice to a map[string]*ast.File,
but that seemed too pervasive.

> Please revert.

What do you think, gri?

[adonovan, 2014-07-07 11:03:48]

On 7 July 2014 06:48, <gordon.klaus@gmail.com> wrote:

> The new function is not called anywhere in this
>> repo
>>
>
> Little did you know, there are packages outside this repo that use
> go/loader.


Of course, but the utility methods of those clients belong in those clients
unless there is a demonstrated benefit to promoting them to the go/loader
package.


 , nor is it covered by tests
>>
>
> Just like PackageInfo.{ObjectOf, TypeOf, VarOf, TypeCaseVar, and
> ImportSpecPkg}, it is a trivial function that hardly needs tests.


These functions are exercised by the SSA builder.  Modify them and the
tests will fail.


 , nor does it have anything to do with
>> go/loader.  If this utility method belongs in any package, that
>>
> package is
>
>> astutil, but I'm still not convinced.
>>
>
> It has as much to do with go/loader as the other convenience methods
> listed above.  Maybe they should all be deleted, but I don't think so.
>

Actually I think many of these should be deleted, and I plan to do just
that when the go/types.Info.Types mapping also includes the operand mode
(e.g. rvalue, lvalue, type, constant),



When you load a program using Config.Import, the only way to find the
> source file paths is via Program.Fset.  The expression
> "prog.Fset.File(file.Package).Name()" didn't read very clearly, so I
> encapsulated it. It seemed to me that many clients would be interested

in this method -- for example if they want to indicate the source file
> where an object was defined, or to rewrite a source file (in the case of
> a refactoring tool, which is my use case) -- so I shared it.
>

I have no problem with you encapsulating it, but sharing it was premature.

I considered changing PackageInfo.Files from a slice to a
> map[string]*ast.File, but that seemed too pervasive.
>

And incorrect, since the slice records the order in which the files are
presented to the compiler, which is sometimes significant.

cheers
alan

[gmk, 2014-07-07 13:24:24]

On Mon, Jul 7, 2014 at 1:03 PM, Alan Donovan <adonovan@google.com> wrote:

> On 7 July 2014 06:48, <gordon.klaus@gmail.com> wrote:
>
>> The new function is not called anywhere in this
>>> repo
>>>
>>
>> Little did you know, there are packages outside this repo that use
>> go/loader.
>
>
> Of course, but the utility methods of those clients belong in those
> clients unless there is a demonstrated benefit to promoting them to the
> go/loader package.
>

Ok, then what are the PackageInfo utility methods doing in go/loader?  They
belong in SSA and whatever other clients need them (are there others?).  I
don't see the benefit of promoting them to go/loader; in fact, there is a
cost: clients will break when you finally delete them.

>
>
>  , nor is it covered by tests
>>>
>>
>> Just like PackageInfo.{ObjectOf, TypeOf, VarOf, TypeCaseVar, and
>> ImportSpecPkg}, it is a trivial function that hardly needs tests.
>
>
> These functions are exercised by the SSA builder.  Modify them and the
> tests will fail.
>

Good, they are tested.  That says nothing about whether Program.FilePath
needs tests (it doesn't).

>
>
>  , nor does it have anything to do with
>>> go/loader.  If this utility method belongs in any package, that
>>>
>> package is
>>
>>> astutil, but I'm still not convinced.
>>>
>>
>> It has as much to do with go/loader as the other convenience methods
>> listed above.  Maybe they should all be deleted, but I don't think so.
>>
>
> Actually I think many of these should be deleted, and I plan to do just
> that when the go/types.Info.Types mapping also includes the operand mode
> (e.g. rvalue, lvalue, type, constant),
>
> Excellent, go/types seems like a much better place for these.

>
>
> When you load a program using Config.Import, the only way to find the
>> source file paths is via Program.Fset.  The expression
>> "prog.Fset.File(file.Package).Name()" didn't read very clearly, so I
>> encapsulated it. It seemed to me that many clients would be interested
>
> in this method -- for example if they want to indicate the source file
>> where an object was defined, or to rewrite a source file (in the case of
>> a refactoring tool, which is my use case) -- so I shared it.
>>
>
> I have no problem with you encapsulating it, but sharing it was premature.
>

I'm just trying to be helpful.  I had to dig for a little while before I
discovered how to get the source file paths; I figured I could save some
people the hassle.  Maybe I could have elucidated the utility of this
method in the CL, but it seemed apparent at the time (as it always does)
and Robert apparently saw it.

Instead of simply rejecting this method, you could have inquired as to its
usefulness.  Don't you think many clients will be interested in source file
paths?  Consider IDEs and refactoring tools.

Cheers,
Gordon

[adonovan, 2014-07-07 13:41:07]

On 7 July 2014 09:24, Gordon Klaus <gordon.klaus@gmail.com> wrote:

> I'm just trying to be helpful.  I had to dig for a little while before I
> discovered how to get the source file paths; I figured I could save some
> people the hassle.  Maybe I could have elucidated the utility of this
> method in the CL, but it seemed apparent at the time (as it always does)
> and Robert apparently saw it.
>
> Instead of simply rejecting this method, you could have inquired as to its
> usefulness.  Don't you think many clients will be interested in source file
> paths?  Consider IDEs and refactoring tools.
>

I know you were just trying to be helpful, but the best way to help is to
keep the API small.  You had to dig for a little while, but what you
learned (the "theory" of token.FileSet), you have to know anyway to do
serious work with the tools APIs.  Once you've understood it, you know
immediately that to look up a filename, you need a FileSet and a Pos and
you need to call File() or Position().

[gmk, 2014-07-07 14:24:21]

Good points, I appreciate having a small API.  At the same time, you
shouldn't have to understand everything about the world just in order to
use some small part of it; maybe the tools shouldn't only target those
doing "serious work".  I don't feel strongly about it.  Let's wait for
Robert since he was the committer.


On Mon, Jul 7, 2014 at 3:41 PM, Alan Donovan <adonovan@google.com> wrote:

> On 7 July 2014 09:24, Gordon Klaus <gordon.klaus@gmail.com> wrote:
>
>> I'm just trying to be helpful.  I had to dig for a little while before I
>> discovered how to get the source file paths; I figured I could save some
>> people the hassle.  Maybe I could have elucidated the utility of this
>> method in the CL, but it seemed apparent at the time (as it always does)
>> and Robert apparently saw it.
>>
>> Instead of simply rejecting this method, you could have inquired as to
>> its usefulness.  Don't you think many clients will be interested in source
>> file paths?  Consider IDEs and refactoring tools.
>>
>
> I know you were just trying to be helpful, but the best way to help is to
> keep the API small.  You had to dig for a little while, but what you
> learned (the "theory" of token.FileSet), you have to know anyway to do
> serious work with the tools APIs.  Once you've understood it, you know
> immediately that to look up a filename, you need a FileSet and a Pos and
> you need to call File() or Position().
>
>

[gri, 2014-07-07 17:58:57]

I'm happy to go with Alan's recommendation here, after all he's the
original author of that package.

The additional function seems sensible, but perhaps it is not at the
"granularity" of the rest of the API.

- gri


On Mon, Jul 7, 2014 at 7:24 AM, Gordon Klaus <gordon.klaus@gmail.com> wrote:

> Good points, I appreciate having a small API.  At the same time, you
> shouldn't have to understand everything about the world just in order to
> use some small part of it; maybe the tools shouldn't only target those
> doing "serious work".  I don't feel strongly about it.  Let's wait for
> Robert since he was the committer.
>
>
> On Mon, Jul 7, 2014 at 3:41 PM, Alan Donovan <adonovan@google.com> wrote:
>
>> On 7 July 2014 09:24, Gordon Klaus <gordon.klaus@gmail.com> wrote:
>>
>>> I'm just trying to be helpful.  I had to dig for a little while before I
>>> discovered how to get the source file paths; I figured I could save some
>>> people the hassle.  Maybe I could have elucidated the utility of this
>>> method in the CL, but it seemed apparent at the time (as it always does)
>>> and Robert apparently saw it.
>>>
>>> Instead of simply rejecting this method, you could have inquired as to
>>> its usefulness.  Don't you think many clients will be interested in source
>>> file paths?  Consider IDEs and refactoring tools.
>>>
>>
>> I know you were just trying to be helpful, but the best way to help is to
>> keep the API small.  You had to dig for a little while, but what you
>> learned (the "theory" of token.FileSet), you have to know anyway to do
>> serious work with the tools APIs.  Once you've understood it, you know
>> immediately that to look up a filename, you need a FileSet and a Pos and
>> you need to call File() or Position().
>>
>>
>

[gmk, 2014-07-07 19:55:56]

reverted in https://codereview.appspot.com/107570043


On Mon, Jul 7, 2014 at 7:58 PM, Robert Griesemer <gri@golang.org> wrote:

> I'm happy to go with Alan's recommendation here, after all he's the
> original author of that package.
>
> The additional function seems sensible, but perhaps it is not at the
> "granularity" of the rest of the API.
>
> - gri
>
>
> On Mon, Jul 7, 2014 at 7:24 AM, Gordon Klaus <gordon.klaus@gmail.com>
> wrote:
>
>> Good points, I appreciate having a small API.  At the same time, you
>> shouldn't have to understand everything about the world just in order to
>> use some small part of it; maybe the tools shouldn't only target those
>> doing "serious work".  I don't feel strongly about it.  Let's wait for
>> Robert since he was the committer.
>>
>>
>> On Mon, Jul 7, 2014 at 3:41 PM, Alan Donovan <adonovan@google.com> wrote:
>>
>>> On 7 July 2014 09:24, Gordon Klaus <gordon.klaus@gmail.com> wrote:
>>>
>>>> I'm just trying to be helpful.  I had to dig for a little while before
>>>> I discovered how to get the source file paths; I figured I could save some
>>>> people the hassle.  Maybe I could have elucidated the utility of this
>>>> method in the CL, but it seemed apparent at the time (as it always does)
>>>> and Robert apparently saw it.
>>>>
>>>> Instead of simply rejecting this method, you could have inquired as to
>>>> its usefulness.  Don't you think many clients will be interested in source
>>>> file paths?  Consider IDEs and refactoring tools.
>>>>
>>>
>>> I know you were just trying to be helpful, but the best way to help is
>>> to keep the API small.  You had to dig for a little while, but what you
>>> learned (the "theory" of token.FileSet), you have to know anyway to do
>>> serious work with the tools APIs.  Once you've understood it, you know
>>> immediately that to look up a filename, you need a FileSet and a Pos and
>>> you need to call File() or Position().
>>>
>>>
>>
>