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

Jump to bottom

x/tools/go/analysis: specify that all facts are encoded in one gulp, preserving topology #54661

Closed
adonovan opened this issue Aug 24, 2022 · 8 comments
Closed

x/tools/go/analysis: specify that all facts are encoded in one gulp, preserving topology #54661

adonovan opened this issue Aug 24, 2022 · 8 comments
Assignees
@adonovan
Labels
Documentation Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Milestone
Unreleased

Comments

@adonovan
Copy link
Member

adonovan commented Aug 24, 2022 •
edited

The go/analysis Fact mechanism allows an analysis to associate information with the symbols defined by one package so that it may be retrieved during analysis of another package that imports these symbols. Facts must be pointers to gob-serializable data structures. In many analyses, facts naturally correspond to nodes in some domain-specific graph representation, so that two or more facts conceptually share a large common data structure such as a call graph. It would be a pity if the encoding of such facts caused the representation to explode by a linear factor.

The current specification of the analysis API permits a driver implementation to encode a collection of facts in a single gob operation, so that any shared portions are encoded exactly once and decoding preserves the original topology. The existing drivers in x/tools use the facts helper package and thus already function in this way; this has been verified experimentally.

I propose to change the specification to state that this behavior is required by all drivers. In the meantime, any existing third-party drivers that do not conform may run slower and use more memory for new analysis passes that assume this requirement, but should otherwise compute the correct answer.
@gopherbot gopherbot added this to the Proposal milestone Aug 24, 2022
@rsc
Copy link
Contributor

rsc commented Oct 20, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

I propose to change the specification to state that this behavior is required by all drivers.

Are there even any other drivers?
In any event, this sounds fine to me (as one of the designers of this API).

Does anyone object to this proposal?

@nightlyone
Copy link
Contributor

@adonovan / @rsc Did you crosscheck this with the authors (e.g. @ldez ) of golangci-lint?

I would like to understand whether that has any effects on the largest linter integration project of the go ecosystem.

@adonovan
Copy link
Member Author

adonovan commented Oct 26, 2022 •
edited

Are there even any other drivers?

I'm aware of at least two: golangci-lint, lintcmd.

Did you crosscheck this with the authors (e.g. @ldez ) of golangci-lint?
I would like to understand whether that has any effects on the largest linter integration project of the go ecosystem.

Good idea; I had not.

golangci-lint decodes a slice of facts all at once but encodes them one at a time, so it would need to be updated.

lintcmd decodes facts one at a time, so it too would need to be updated.

@rsc
Copy link
Contributor

rsc commented Nov 2, 2022

Because:

In the meantime, any existing third-party drivers that do not conform may run slower and use more memory for new analysis passes that assume this requirement, but should otherwise compute the correct answer.

This seems fine to do even though there are a couple drivers. They can update or not as necessary, but nothing will break.

@rsc
Copy link
Contributor

rsc commented Nov 2, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Nov 9, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: go/analysis: specify that all facts are encoded in one gulp, preserving topology go/analysis: specify that all facts are encoded in one gulp, preserving topology Nov 9, 2022
@rsc rsc modified the milestones: Proposal, Backlog Nov 9, 2022
@adonovan adonovan self-assigned this Nov 9, 2022
@gopherbot
Copy link

Change https://go.dev/cl/454564 mentions this issue: go/analysis: document that facts are gob encoded in one gulp

@rsc rsc changed the title go/analysis: specify that all facts are encoded in one gulp, preserving topology x/tools/go/analysis: specify that all facts are encoded in one gulp, preserving topology Dec 6, 2022
@dmitshur dmitshur modified the milestones: Backlog, Unreleased Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adonovan adonovan

Labels
Documentation Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@rsc @nightlyone @dmitshur @adonovan @gopherbot