FileRestorer doesn't preserve import grouping

[Deiz]

This is minor, but curious whether you'd take a patch to grant users of the library control over import grouping.

Some organizations (including my own) tend to group imports beyond stdlib/non-stdlib. I've seen two permutations at different orgs:

1. stdlib, public, private
2. stdlib, public, private (shared), private (local)

goimports supports the former via its -local flag, such that:

import (
	"net/http"
	"github.com/public/pkg"
	"github.com/private/pkg"
)

when processed via goimports -local github.com/private will result in:

import (
	"net/http"

	"github.com/public/pkg"

	"github.com/private/pkg"
)

Further, goimports does not remove pre-existing groups.

I'd like dst's import management to provide the ability to retain the original grouping of imports - though this does introduce the problem of needing to find an appropriate place to insert any new imports that aren't stdlib.

Alternately, perhaps the best bet is to avoid using an import-aware FileRestorer and implement similar logic by hand.

[dave]

Hmm... the logic to determine the import block was perhaps the most tricky to get right. There's like a million edge cases, and the code that does it is by no means easy to follow or straightforward.

Also I haven't worked on this codebase in a long while and testing and validating a patch would be a really big job for me... I definitely don't have time right now for that. I think if this was a major bug I'd take it on, but for a pretty minor improvement like this I doubt I'd ever get around to it.

[Deiz]

Appreciate the quick response - That's entirely fair.

Because I get value out of import management in general and updateImports only generates new blocks when it detects the need to add imports, my workaround will be to ensure that my code generation correctly adds imports such that dst never needs to do the heavy lifting.

Thinking about this a little bit more, what do you think of FileRestorer.DisableImportAdd bool that would cause updateImports to return an error if FileRestorer detected the need to add an import? That'd satisfy my use case - it would become a visible error on my end if my code failed to add an expected import.

import (
	"fmt"
	"go/token"
	"strconv"
	"strings"

	"github.com/dave/dst"
	"github.com/dave/dst/decorator"
)

func addImports(pkg *decorator.Package, file *dst.File, imports []string) {
	for _, imp := range imports {
		addImport(pkg, file, imp)
	}
}

func addImport(pkg *decorator.Package, file *dst.File, imp string) {
	for _, pkg := range pkg.Imports {
		if pkg.Name == imp {
			return
		}
	}

	// Where to insert our import block within the file's Decl slice
	index := 0

	importSpec := &dst.ImportSpec{
		Path: &dst.BasicLit{Kind: token.STRING, Value: fmt.Sprintf("%q", imp)},
	}

	for i, node := range file.Decls {
		n, ok := node.(*dst.GenDecl)
		if !ok {
			continue
		}

		if n.Tok != token.IMPORT {
			continue
		}

		if len(n.Specs) == 1 && mustUnquote(n.Specs[0].(*dst.ImportSpec).Path.Value) == "C" {
			// If we're going to insert, it must be after the "C" import
			index = i + 1
			continue
		}

		// Insert our import into the first non-"C" import block
		for j, spec := range n.Specs {
			path := mustUnquote(spec.(*dst.ImportSpec).Path.Value)
			if !strings.Contains(path, ".") || imp > path {
				continue
			}

			importSpec.Decorations().Before = spec.Decorations().Before
			spec.Decorations().Before = dst.NewLine

			n.Specs = append(n.Specs[:j], append([]dst.Spec{importSpec}, n.Specs[j:]...)...)
			return
		}

		n.Specs = append(n.Specs, importSpec)
		return
	}

	gd := &dst.GenDecl{
		Tok:   token.IMPORT,
		Specs: []dst.Spec{importSpec},
		Decs: dst.GenDeclDecorations{
			NodeDecs: dst.NodeDecs{Before: dst.EmptyLine, After: dst.EmptyLine},
		},
	}

	file.Decls = append(file.Decls[:index], append([]dst.Decl{gd}, file.Decls[index:]...)...)
}

func mustUnquote(s string) string {
	out, err := strconv.Unquote(s)
	if err != nil {
		panic(err)
	}
	return out
}

[dave]

Is there no work-around? Seems like you could create something like FileRestorer.DisableImportAdd yourself and run it before rendering the file?

[Deiz]

Ah, sorry - I meant that as an attribute of the FileRestorer.

I.e. here it'd become:

if r.DisableImportAdd {
	return fmt.Errorf("refusing to add import %q when DisableImportAdd is set", path)
}

added = true

That said, I've got two layers of defensive code around this now:

1. A function like above, wherein the injection of generated code is matched with an attempt to add the generated code's imports to the file's imports block
2. A simple map-based resolver that only knows about existing package-level imports and those introduced via generated code

So I don't by any means need to add an attribute to FileRestorer, but it would help guarantee correct operation of my code.