Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/cmd/guru: does 'implements' really require a scope? #14748

Open
dominikh opened this issue Mar 10, 2016 · 10 comments
Open

x/tools/cmd/guru: does 'implements' really require a scope? #14748

dominikh opened this issue Mar 10, 2016 · 10 comments
Labels
Milestone

Comments

@dominikh
Copy link
Member

@dominikh dominikh commented Mar 10, 2016

Currently, running the 'implements' query requires setting a scope. Is this necessary? Why is pointer analysis required for checking the implements relation? Why can't it default to the entire workspace, like 'referrers'?

Also, there's at least a bug here: implements.go checks if a scope has been provided, and if not, uses a different way of determining the packages. However, len(q.Scope) is always greater than 0. If no scope is provided, it's a slice with one element, the empty string.

/cc @alandonovan

@dominikh

This comment has been minimized.

Copy link
Member Author

@dominikh dominikh commented Mar 10, 2016

Probably related to #13457

@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Mar 10, 2016
@adonovan

This comment has been minimized.

Copy link

@adonovan adonovan commented Mar 25, 2016

[I thought I'd already responded but I can't find any evidence of it. Apologies if you get deja vu reading it.]

The implements and referrers queries might in principle have inspect the entire workspace---that is, the reverse transitive closure of import dependencies, starting from the selected declaration. In a very large workspace this doesn't scale if you search for say fmt.Print, although it does pretty well for declarations in less commonly used standard packages like go/types. So I think there is a notion of "type analysis" scope wanted here, distinct from "pointer analysis scope" even though my change conflates them. We could introduce two separate scopes, but I feel that a single scope is already almost too much configuration for most users. There may be other ways to bound the amount of work, e.g. stopping after 100 reports, or after 100 packages have been inspected, or after 10 seconds.

@adonovan

This comment has been minimized.

Copy link

@adonovan adonovan commented Mar 25, 2016

Ah, this related bug contains the comment I was thinking of:
#14747

@dominikh

This comment has been minimized.

Copy link
Member Author

@dominikh dominikh commented Mar 25, 2016

I think it's fine to overload a single -scope flag with two meanings. The user doesn't really care if it's about pointer analysis or some other way of limiting guru's scope. All -scope really means is "consider these packages when doing queries`.

I'd prefer not to have arbitrary limits to the amount of reports or packages. That would ultimately need a "give me everything" flag when I need the 101st package.

@alandonovan

This comment has been minimized.

Copy link
Contributor

@alandonovan alandonovan commented Mar 25, 2016

On 24 March 2016 at 22:38, Dominik Honnef notifications@github.com wrote:

I think it's fine to overload a single -scope flag with two meanings. The
user doesn't really care if it's about pointer analysis or some other way
of limiting guru's scope. All -scope really means is "consider these
packages when doing queries`.

Well, sort of. The pointer analysis scope is really an optimization
because computing PTA for the entire workspace is prohibitively expensive,
but if it was free I would have -scope=... all the time so that I never
have to think about updating my scope as I switch from one program to
another. The implements/referrers queries are much cheaper than PTA
(relatively) and generate results in streaming fashion, making it feasible
and useful to have -scope=... for medium or even large-sized workspaces.

I'd prefer not to have arbitrary limits to the amount of reports or

packages. That would ultimately need a "give me everything" flag when I
need the 101st package.

Agreed. Each additional flag is a UI nightmare when there's an editor in
the way. I like simply providing a stream of results and letting the user
kill the task when they've had enough. :)

@dominikh

This comment has been minimized.

Copy link
Member Author

@dominikh dominikh commented Mar 25, 2016

making it feasible and useful to have -scope=... for medium or even large-sized workspaces

I'd be perfectly fine with implements/referrers defaulting to a ... scope. However, the current situation is such: referrers doesn't take a scope argument at all, and implements claims that it optionally takes one, but is broken if you don't provide it one.

Personally I'd like to standardize both queries on "default to all, but do accept the -scope flag".

I like simply providing a stream of results and letting the user kill the task when they've had enough

But that doesn't really help when there's thousands of matches before the ones I care about appear. It also needs a lot more attention by the user now. Unless the editor provides a way to filter the list, but that's even more UI.

@adonovan

This comment has been minimized.

Copy link

@adonovan adonovan commented Apr 26, 2016

We agree that implements and referrers should be restricted by a scope flag whose default is ....

I maintain that this must be a different flag from the -scope flag that governs the pointer analysis, because most users do not want to restrict implements and referrers, but all users must restrict the pointer analysis. I reluctantly conclude we need a second scope flag.

What should the two flags be called? If compatibility were no concern, I would propose -ptascope and -scope, with the former defaulting to "nothing" and the latter defaulting to "everything". (Some of the query modes that use pointer analysis can be altered to work without a scope, at least some of the time. For example, a callers query for an unexported function that is not address-taken doesn't need pointer analysis, and callers could simply print an "incomplete results; set -ptascope" warning for other cases.)

I would generally not be concerned with compatibility just yet, but this week both vim-go and GoSublime announced their new support for the Guru.

@dominikh

This comment has been minimized.

Copy link
Member Author

@dominikh dominikh commented Apr 26, 2016

From the standpoint of a purist, I can see why you'd want two different flags. I, however, believe that it requires less mental overhead to manage one flag instead of two. As a user, I don't really care about pointer analysis. I mostly care about how fast an operation is, and what results I get. I think it would be a lot easier to change a scope between "..." and something more restrictive whenever needed, instead of having to set two flags and understand which one applies to which operation.

In particular when you say that some modes sometimes don't need PTA and sometimes do that really causes confusion. PTA isn't something most people think about, or know for that matter. They'd like a query to work. If it's too slow, they'll restrict the scope. If it doesn't cover everything they care about, they expand the scope. The way I see it, having two flags has only two advantages: 1) Purity 2) Not having to change one of the flags as often. I don't think that 1 is worth it, and 2 can be fixed in the UI instead (provide a toggle between "all" and "last value", for example).

@adonovan

This comment has been minimized.

Copy link

@adonovan adonovan commented Apr 26, 2016

The problem is that the correct defaults for these flags are diametrically opposed: except for a few users with enormous workspaces, everyone will want the default -scope of "everything" so that 'implements' and 'referrers' just work. ('referrers' and 'describe' are the commands I use the most.) But with a --ptascope of "everything", the pointer analysis would never finish, and those users that bother to set a restricted -ptascope would then not be able to use 'referrers' again without changing the scope.

@dominikh

This comment has been minimized.

Copy link
Member Author

@dominikh dominikh commented Apr 27, 2016

I feel like this is entirely a UI issue and should be solved in the editors. From the CLI standpoint, there is no persistent scope setting. Each invocation of the utility needs to be provided the/both scope parameter(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.