Skip to content

x/tools/go/analysis: Pass.All{Package,Object}Facts: document what "all" means #70351

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.

Sign up for GitHub

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

Jump to bottom
Closed
adonovan opened this issue Nov 14, 2024 · 10 comments
Closed

x/tools/go/analysis: Pass.All{Package,Object}Facts: document what "all" means #70351

adonovan opened this issue Nov 14, 2024 · 10 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@adonovan
Copy link
Member 

package analysis // golang.org/x/tools/go/analysis

type Pass struct {
	...
	// AllObjectFacts returns a new slice containing all object
	// facts of the analysis's FactTypes in unspecified order.
	AllObjectFacts func() []ObjectFact

What does "all" mean exactly? I think the intent is that it means the set of facts that you would get if you were to call ImportObjectFact on every types.Object that you are able to obtain from the Pass (and mutatis mutandi for s/Object/Package/g).

More generally the API is unclear as to whether Export(o, f) followed by Import(o) returns f even within the same package. I think the implementations have always done so.
@adonovan adonovan added Analysis Issues related to static analysis (vet, x/tools/go/analysis) Documentation Issues describing a change to documentation. labels Nov 14, 2024
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 14, 2024
@gopherbot gopherbot added this to the Unreleased milestone Nov 14, 2024
@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 15, 2024
@ninad-k
Copy link

ninad-k commented Nov 20, 2024

I am the new to this repo. I can work on it, Can someone help to fix this?

@adonovan
Copy link
Member Author

I am the new to this repo. I can work on it, Can someone help to fix this?

Sure, thanks! You can send the CL to me.

@timothy-king
Copy link
Contributor

A challenge to defining "all" is to find a pithy way to describe golang.org/x/tools/go/types/objectpath. That has a large practical influence on "all".

FWIW there are some existing disagreements between Analysis runners #47725 .

@adonovan
Copy link
Member Author

A challenge to defining "all" is to find a pithy way to describe golang.org/x/tools/go/types/objectpath.

Why is that necessary? The set of exported symbols of a package is independent of however we name them.

@timothy-king
Copy link
Contributor

The Facts for imported packages are in practice controlled by a mixture of objectpath and the runner (at least this used to be true). If you are trying to determine what "all" means in practice today, it seems like a good place to start. This may converge to something like "the set of exported symbols of a package" for imports.

@adonovan
Copy link
Member Author

The goal is not to describe the exact details of one implementation, but the contract between the analyzer and the driver.

Here's what I had in mind:

// AllObjectFacts returns a new slice containing all object
// facts of the analysis's FactTypes in unspecified order.
//
// The result includes all facts exported by packages
// whose symbols are referenced by the current package
// (by qualified identifiers or field/method selections).
// And it includes all facts exported from the current
// package by the current analysis pass.

@timothy-king
Copy link
Contributor

That seems good. (Though it may imply we have some bugs today.) Quick nit: s/symbols are referenced by/symbols may be referenced by/. We do not want to have to check that a reference exists in the current package.

@adonovan
Copy link
Member Author

s/symbols are referenced by/symbols may be referenced by/. We do not want to have to check that a reference exists in the current package.

"Includes all" implies only a lower bound, so neither wording requires us to implement such a check.

@willboland willboland mentioned this issue Dec 29, 2024
Closed
@willboland
Copy link
Contributor

@adonovan @timothy-king CL is open as I saw no activity from @ninad-k.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638278 mentions this issue: go/analysis: document AllObjectFacts return value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

x/tools/go/analysis: document AllObjectFacts return value willboland/golang-tools
6 participants
@timothy-king @adonovan @gopherbot @cherrymui @willboland @ninad-k