[tools] internal/lsp/analysis: warn about incorrect type in embed directive

[Robert Findley (Gerrit)]

Robert Findley submitted this change.

View Change

Approvals: Robert Findley: Looks good to me, approved Go LUCI: TryBots succeeded Alan Donovan: Looks good to me, but someone else must approve

internal/lsp/analysis: warn about incorrect type in embed directive

Report a warning if the type in the declaration
following a go:embed directive is not string,
[]byte or embed.FS.

Change-Id: Ic6add9509cbbb7d3ee016371df6b6ee13fb27dcd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/536475
LUCI-TryBot-Result: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfin...@google.com>
Reviewed-by: Alan Donovan <adon...@google.com>
---
M gopls/internal/lsp/analysis/embeddirective/embeddirective.go
M gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present.go
M gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present_go120.go
3 files changed, 96 insertions(+), 12 deletions(-)

diff --git a/gopls/internal/lsp/analysis/embeddirective/embeddirective.go b/gopls/internal/lsp/analysis/embeddirective/embeddirective.go
index b7efe47..33af72b 100644
--- a/gopls/internal/lsp/analysis/embeddirective/embeddirective.go
+++ b/gopls/internal/lsp/analysis/embeddirective/embeddirective.go
@@ -9,6 +9,7 @@
 import (
 	"go/ast"
 	"go/token"
+	"go/types"
 	"strings"
 
 	"golang.org/x/tools/go/analysis"
@@ -67,10 +68,12 @@
 			switch {
 			case spec == nil:
 				report(`go:embed directives must precede a "var" declaration`)
-			case len(spec.Names) > 1:
+			case len(spec.Names) != 1:
 				report("declarations following go:embed directives must define a single variable")
 			case len(spec.Values) > 0:
 				report("declarations following go:embed directives must not specify a value")
+			case !embeddableType(pass.TypesInfo.Defs[spec.Names[0]]):
+				report("declarations following go:embed directives must be of type string, []byte or embed.FS")
 			}
 		}
 	}
@@ -132,3 +135,28 @@
 	}
 	return spec
 }
+
+// embeddableType in go:embed directives are string, []byte or embed.FS.
+func embeddableType(o types.Object) bool {
+	if o == nil {
+		return false
+	}
+
+	// For embed.FS the underlying type is an implementation detail.
+	// As long as the named type resolves to embed.FS, it is OK.
+	if named, ok := o.Type().(*types.Named); ok {
+		obj := named.Obj()
+		if obj.Pkg() != nil && obj.Pkg().Path() == "embed" && obj.Name() == "FS" {
+			return true
+		}
+	}
+
+	switch v := o.Type().Underlying().(type) {
+	case *types.Basic:
+		return types.Identical(v, types.Typ[types.String])
+	case *types.Slice:
+		return types.Identical(v.Elem(), types.Typ[types.Byte])
+	}
+
+	return false
+}
diff --git a/gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present.go b/gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present.go
index 6d8138f..a124a58 100644
--- a/gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present.go
+++ b/gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present.go
@@ -8,62 +8,118 @@
 //go:embed embedText // want "go:embed directives must precede a \"var\" declaration"
 
 import (
+	"embed"
+	embedPkg "embed"
 	"fmt"
 
 	_ "embed"
 )
 
 //go:embed embedText // ok
-var s string
+var e1 string
 
 // The analyzer does not check for many directives using the same var.
 //
 //go:embed embedText // ok
 //go:embed embedText // ok
-var s string
+var e2 string
 
-// Comments and blank lines between are OK.
+// Comments and blank lines between are OK. All types OK.
 //
 //go:embed embedText // ok
 //
 // foo
 
-var s string
+var e3 string
+
+//go:embed embedText //ok
+var e4 []byte
+
+//go:embed embedText //ok
+var e5 embed.FS
 
 // Followed by wrong kind of decl.
 //
 //go:embed embedText // want "go:embed directives must precede a \"var\" declaration"
-func foo()
+func fooFunc() {}
 
 // Multiple variable specs.
 //
 //go:embed embedText // want "declarations following go:embed directives must define a single variable"
-var foo, bar []byte
+var e6, e7 []byte
 
 // Specifying a value is not allowed.
 //
 //go:embed embedText // want "declarations following go:embed directives must not specify a value"
-var s string = "foo"
+var e8 string = "foo"
 
 // TODO: This should not be OK, misplaced according to compiler.
 //
 //go:embed embedText // ok
 var (
-	s string
-	x string
+	e9  string
+	e10 string
 )
 
+// Type definition.
+type fooType []byte
+
+//go:embed embedText //ok
+var e11 fooType
+
+// Type alias.
+type barType = string
+
+//go:embed embedText //ok
+var e12 barType
+
+// Renamed embed package.
+
+//go:embed embedText //ok
+var e13 embedPkg.FS
+
+// Renamed embed package alias.
+type embedAlias = embedPkg.FS
+
+//go:embed embedText //ok
+var e14 embedAlias
+
 // var blocks are OK as long as the variable following the directive is OK.
 var (
 	x, y, z string
 	//go:embed embedText // ok
-	s       string
+	e20     string
 	q, r, t string
 )
 
 //go:embed embedText // want "go:embed directives must precede a \"var\" declaration"
 var ()
 
+// Incorrect types.
+
+//go:embed embedText // want `declarations following go:embed directives must be of type string, \[\]byte or embed.FS`
+var e16 byte
+
+//go:embed embedText // want `declarations following go:embed directives must be of type string, \[\]byte or embed.FS`
+var e17 []string
+
+//go:embed embedText // want `declarations following go:embed directives must be of type string, \[\]byte or embed.FS`
+var e18 embed.Foo
+
+//go:embed embedText // want `declarations following go:embed directives must be of type string, \[\]byte or embed.FS`
+var e19 foo.FS
+
+type byteAlias byte
+
+//go:embed embedText // want `declarations following go:embed directives must be of type string, \[\]byte or embed.FS`
+var e15 byteAlias
+
+// A type declaration of embed.FS is not accepted by the compiler, in contrast to an alias.
+type embedDecl embed.FS
+
+//go:embed embedText // want `declarations following go:embed directives must be of type string, \[\]byte or embed.FS`
+var e16 embedDecl
+
 // This is main function
 func main() {
 	fmt.Println(s)
diff --git a/gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present_go120.go b/gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present_go120.go
index f88babd..2eaad23 100644
--- a/gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present_go120.go
+++ b/gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present_go120.go
@@ -11,7 +11,7 @@
 	// Okay directive wise but the compiler will complain that
 	// imports must appear before other declarations.
 	//go:embed embedText // ok
-	"foo"
+	foo string
 )
 
 import (

To view, visit change 536475. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: merged

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: Ic6add9509cbbb7d3ee016371df6b6ee13fb27dcd

Gerrit-Change-Number: 536475

Gerrit-PatchSet: 10

Gerrit-Owner: Viktor Blomqvist <veblo...@gmail.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>