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/go/packages: provide alternative to deprecated LoadAllSyntax #32365

Closed
dominikh opened this issue May 31, 2019 · 5 comments
Closed

x/tools/go/packages: provide alternative to deprecated LoadAllSyntax #32365

dominikh opened this issue May 31, 2019 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@dominikh
Copy link
Member

A fair number of tools will need the equivalent of the now deprecated LoadAllSyntax constant. Instead of forcing everyone to write their own bitwise OR of 9 constants, it would be nice if we could provide a constant for this. Unfortunately we can't call it NeedEverything, as LoadAllSyntax doesn't include NeedExportsFile, and including it unnecessarily probably causes unnecessary work by the compiler.

/cc @matloob

@dominikh dominikh added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 31, 2019
@gopherbot gopherbot added this to the Unreleased milestone May 31, 2019
@matloob
Copy link
Contributor

matloob commented Jun 19, 2019

I'm actually a bit surprised by this. There are that many tools that need to be able to see (almost) all the fields on the package struct? My instinct is to avoid adding a NeedX field that has more than one bit set if we can avoid doing so

@dominikh
Copy link
Member Author

It's possible that the tools I looked at used LoadAllSyntax out of convenience, not necessity.

However, intuitively, it seems to me that all tools that do full-fledged source based analysis will need the majority of fields in Package. They will need ASTs, they will need to resolve imports, and they will need type information. This amounts to LoadAllSyntax. Tools that need this amount of information include go/analysis runners, pre-go/analysis linters (golint, errcheck, ...) and, to a degree, code manipulation tools like goreturns, though these may not need NeedDeps, but then I'd argue that LoadAllSyntax ^ NeedDeps is easier than ORing 8 constants.

Am I missing something? Do most tools actually need much less information than I am thinking?

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@matloob
Copy link
Contributor

matloob commented Oct 29, 2019

I think you're right that tools need this information. However I'd prefer to leave things as is unless we find a more pressing need for a LoadAllSyntax alternative. I think it's reasonable for tool authors to think through whether they need all the fields, and if they do, or-ing the 8 constants.

@dominikh
Copy link
Member Author

dominikh commented May 3, 2020

I don't personally need this anymore, and I haven't seen anyone else ask for it. @matloob should we close this?

@matloob
Copy link
Contributor

matloob commented May 4, 2020

Yeah closing sounds good.

@dominikh dominikh closed this as completed May 4, 2020
@golang golang locked and limited conversation to collaborators May 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

3 participants