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/{analysis,packages}: expose TypeErrors []types.Error fields #54619

Closed
adonovan opened this issue Aug 23, 2022 · 12 comments
Closed

x/tools/go/{analysis,packages}: expose TypeErrors []types.Error fields #54619

adonovan opened this issue Aug 23, 2022 · 12 comments
Assignees
Labels
Proposal Proposal-Accepted Proposal-FinalCommentPeriod Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Aug 23, 2022

The go/analysis package defines a standard interface between analysis drivers (e.g. go vet) and checkers (e.g. the printf format string checker) that allows static analyzers to be written once and run in a wide range of tools, environments, and build systems. Most analyzers assume well-typed code, but it is possible for an analyzer to indicate that it is safe to run despite type errors. The gopls tool makes extensive use of such analyzers, and in particular their ability to suggest a quick fix to the code that would repair a mistake. However, the go/analysis framework doesn't provide access to the errors detected during type checking, even to analyzers that have indicated they are safe to run in the presence of such errors. gopls currently uses its privileged location in the x/tools repo to obtain backdoor access to these fields, but we consider this a hack.

We propose to expose the list of type errors, alongside all the other existing results of type checking, in the go/analysis Pass structure, as a new field, TypeErrors []types.Error. Because this interface forms the contract between driver and analysis, this change implies that all drivers that support the RunDespiteErrors feature must in due course populate this field. In the meantime, we expect that third-party drivers that do not populate the new field may report fewer diagnostics or fixes from analyzers that rely on the new field but will otherwise behave gracefully.

For the go/packages-based driver to populate this field, it too would need to expose a similar field in its Packages struct. We propose a similar change there too.

The necessary code change can be seen in https://go.dev/cl/425095.

@gopherbot gopherbot added this to the Proposal milestone Aug 23, 2022
@findleyr findleyr removed their assignment Aug 29, 2022
@findleyr
Copy link
Contributor

findleyr commented Aug 29, 2022

Removing my assignment as this still goes through the proposal committee, even though it is an x/tools API change.

On first principles it makes sense to me that analyzers setting RunDespiteErrors receive the full results of type checking, including errors. However, I have concerns that this API encourages a structured interpretation of type-checker error messages, which should be discouraged. We interpret error messages in gopls, but this is a source of fragility and friction.

In short, I think we should do this -- gopls should not have privileged access to go/analysis internals -- but now is perhaps a good time to also revisit efforts aimed at adding additional structure to go/types.Error.

@gopherbot
Copy link

Change https://go.dev/cl/425095 mentions this issue: go/analysis: add Pass.TypeErrors field

@timothy-king
Copy link
Contributor

If we do add structure to go/types.Error, the proposed change will forward additional information to the analyzers transparently. If we do not add more structure to go/types.Error, having access to this information still feels like 'better than nothing'. gopls using it (fragility and friction and all) feels like evidence of this value.

@timothy-king
Copy link
Contributor

Can we think of a reason we would not be able to provide go/types.Errors in a consistent way across different drivers for analysis, internal/checker, go cmd, gopls, bazel, ... ? That could lead to inconsistent experiences across tools.

I have not thought of one, but maybe someone else can?

@adonovan
Copy link
Member Author

adonovan commented Sep 22, 2022

@rsc I think this proposal is waiting for committee review, but do let me know if there's anything I need to do.

@adonovan
Copy link
Member Author

adonovan commented Oct 6, 2022

Can we think of a reason we would not be able to provide go/types.Errors in a consistent way across different drivers for analysis, internal/checker, go cmd, gopls, bazel, ... ?

No, I cannot even imagine an obstacle here.

@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

Does anyone object to accepting this proposal?

@adonovan
Copy link
Member Author

Can't say for sure, but no-one was motivated to comment on the CL or proposal in the last eight weeks.

@rsc
Copy link
Contributor

rsc commented Nov 9, 2022

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

@adonovan
Copy link
Member Author

adonovan commented Nov 9, 2022

The hour of victory is close at hand. ;-)

@rsc
Copy link
Contributor

rsc commented Nov 16, 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: x/tools/go/{analysis,packages}: expose TypeErrors []types.Error fields x/tools/go/{analysis,packages}: expose TypeErrors []types.Error fields Nov 16, 2022
@rsc rsc modified the milestones: Proposal, Backlog Nov 16, 2022
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 16, 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
Labels
Proposal Proposal-Accepted Proposal-FinalCommentPeriod Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants