search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.93k stars 17.66k forks source link

x/tools/astutil: use goimports import path to import name heuristic in UsesImport #30331

Open josharian opened 5 years ago

josharian commented 5 years ago

goimports has a sophisticated heuristic to guess a package import name from its import path. We might even eventually enshrine that heuristic in the language (https://github.com/golang/go/issues/29036).

astutil.UsesImport uses a much less sophisticated heuristic. We should move the goimports heuristic to somewhere exported (maybe in astutil?) and use it in astutil.UsesImport.

(astutil.UsesImport could also use some expanded docs, but that is another matter.)

cc @heschik

heschi commented 5 years ago

Ian's been making changes to that heuristic under the assumption that it only affects goimports, where the worst case is that some user code churns. Dunno how safe it is to export it.

@ianthehat

mvdan commented 5 years ago

Perhaps we can export it once the heuristic has stabilized?

dmitshur commented 5 years ago

I think the original issue isn't about astutil.UsesImport primarily, but since it is mentioned, I want to provide the following information about it.

I have a local WIP commit that mentions astutil.UsesImport. I discovered a fundamental issue with it. Fixing the issue may only be possible by breaking the API. As a result, it may make more sense to deprecate the current UsesImport. I wasn't sure if many people use it, and whether it'd be worthwhile to deal with it, so the WIP commit left sitting behind.

I'll copy the commit message here, since it's relevant:

commit 16fa2d7c70732cd3ee2b4e007b4cda9e4ed52e6a
Author: Dmitri Shuralyov <dmitshur@golang.org>
Date:   Thu Nov 1 00:37:20 2018 -0400

    WIP: go/ast/astutil: try to fix UsesImport

    UsesImport is broken in that it will return true or false
    on the same input if you just reorder the imports.

    UsesImport is hard to fix properly without breaking its API.
    To make some sort of sense, it'd need to accept both import name
    and path. Not just path.

    So, before doing anything about it, see if it's even worth it.
    Maybe the best way forward is to deprecate it or remove it.

    Change-Id: If878f625fec48ae36d510a93b11285e0c6003fd6

 go/ast/astutil/imports.go      | 61 +++++++++++++++++++++++-------------------
 go/ast/astutil/imports_test.go | 12 +++++++++
 2 files changed, 46 insertions(+), 27 deletions(-)
ianthehat commented 5 years ago

The heuristic was deliberately extracted to the function importPathToAssumedName so that it could possibly be exposed one day. Probably needs some more discussion about whether the heuristic is reasonable and complete.