x/tools/internal/typesinternal: FileQualifier needs to add imports, and reject unnameable types

[adonovan]

The job of FileQualifier is to help TypeString print types in valid Go syntax, respecting the local lexical environment and the file's set of existing imports. However, in general this may require (a) adding new imports for types belonging to packages that are not currently imported and (b) reporting that the task cannot be completed because one or more types are members of another package and are inaccessible either because they are unexported or because that package is internal relative to this one.

The task of this issue is to augment FileQualifier to accumulate a list of import edits, and an error when it sees a type it cannot print. This should fix a number of bugs across all our refactoring tools. It could also use some good tests.

[JunyangShao]

@golang/tools-team

[findleyr]

So something like this, where ImportEdit is some representation of new imports (maybe just *ast.ImportSpec, as we use in some places)?

type FileQualifier struct {
  file *ast.File
  pkg *types.Package
  
  imports map[string]string // import path -> name
  newImports []ImportEdit
}

func (*FileQualifier) Qualify(*types.Package) string

func (*FileQualifier) NewImports() []ImportEdit

[adonovan]

It will need a public Err error field too, or NewImports should return an error. Also it needs to know the current package, so that it can call analysisinternal.CanImport on each new import. And it will need a token.Pos for the current position and a types.Info for the lexical environment, since it will need to call analysisinternal.AddImport. (Some of these decls will need to move.) The best way to design this thing will be to implement it, I suspect.

[findleyr]

Agreed (the pkg field is the current package, but you're right it needs types.Info and token.Pos).

I take it you're implementing?

Do you know concrete bugs this will fix? I believe you that they exist, but I'm curious.

[adonovan]

I take it you're implementing?

Not if someone else wants to.

Do you know concrete bugs this will fix? I believe you that they exist, but I'm curious.

I've seen occasional failures of various refactoring operations to fix up the imports correctly that I believe are due to this. They're pretty minor bugs because they're hard to miss and easy to fix, but I think the resulting API will become a very widely used building block across our tools (as AddImport has done).

[findleyr]

@madelinekalil this might be a good one for you to pick up. It's used in extraction, and I think you'd looked into extract bugs recently.

[adonovan]

Two thoughts:

1. We may need to rewrite types.TypeString since there are types it will happily print that cannot be denoted in Go syntax, for example the type struct{x int} declared in another package, so it will need to report errors in such cases.
2. We should factor the common parts of the new FileQualifier and AddImport using a common "accumulator" type that accumulates added import declarations, so that a series of calls to print types and/or access imported symbols can produce one consistent set of edits.