internal/lsp/analysis: embed directive analyzer check following decl

Extend the embed directive analyzer. Verify that embed directives
are followed by a single variable declaration.

Updates golang/go#50262

Change-Id: Ib69bbfb3aab586a349360d501ceb714a5434e8bd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/504295
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>

gopls/internal/lsp/analysis/embeddirective/embeddirective.go

@@ -7,6 +7,7 @@ package embeddirective
 
 import (
 	"go/ast"
+	"go/token"
 	"strings"
 
 	"golang.org/x/tools/go/analysis"
@@ -27,32 +28,98 @@ var Analyzer = &analysis.Analyzer{
 
 func run(pass *analysis.Pass) (interface{}, error) {
 	for _, f := range pass.Files {
-		com := hasEmbedDirectiveComment(f)
-		if com != nil {
-			assertEmbedImport(pass, com, f)
+		comments := embedDirectiveComments(f)
+		if len(comments) == 0 {
+			continue // nothing to check
+		}
+
+		hasEmbedImport := false
+		for _, imp := range f.Imports {
+			if imp.Path.Value == `"embed"` {
+				hasEmbedImport = true
+				break
+			}
+		}
+
+		for _, c := range comments {
+			report := func(msg string) {
+				pass.Report(analysis.Diagnostic{
+					Pos:     c.Pos(),
+					End:     c.Pos() + token.Pos(len("//go:embed")),
+					Message: msg,
+				})
+			}
+
+			if !hasEmbedImport {
+				report(`must import "embed" when using go:embed directives`)
+			}
+
+			spec := nextVarSpec(c, f)
+			switch {
+			case spec == nil:
+				report(`go:embed directives must precede a "var" declaration`)
+			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")
+			}
 		}
 	}
 	return nil, nil
 }
 
-// Check if the comment contains //go:embed directive.
-func hasEmbedDirectiveComment(f *ast.File) *ast.Comment {
+// embedDirectiveComments returns all comments in f that contains a //go:embed directive.
+func embedDirectiveComments(f *ast.File) []*ast.Comment {
+	comments := []*ast.Comment{}
 	for _, cg := range f.Comments {
 		for _, c := range cg.List {
 			if strings.HasPrefix(c.Text, "//go:embed ") {
-				return c
+				comments = append(comments, c)
 			}
 		}
 	}
-	return nil
+	return comments
 }
 
-// Verifies that "embed" import exists for //go:embed directive.
-func assertEmbedImport(pass *analysis.Pass, com *ast.Comment, f *ast.File) {
-	for _, imp := range f.Imports {
-		if "\"embed\"" == imp.Path.Value {
-			return
+// nextVarSpec returns the ValueSpec for the variable declaration immediately following
+// the go:embed comment, or nil if the next declaration is not a variable declaration.
+func nextVarSpec(com *ast.Comment, f *ast.File) *ast.ValueSpec {
+	// Embed directives must be followed by a declaration of one variable with no value.
+	// There may be comments and empty lines between the directive and the declaration.
+	var nextDecl ast.Decl
+	for _, d := range f.Decls {
+		if com.End() < d.End() {
+			nextDecl = d
+			break
 		}
 	}
-	pass.Report(analysis.Diagnostic{Pos: com.Pos(), End: com.Pos() + 10, Message: "The \"embed\" package must be imported when using go:embed directives."})
+	if nextDecl == nil || nextDecl.Pos() == token.NoPos {
+		return nil
+	}
+	decl, ok := nextDecl.(*ast.GenDecl)
+	if !ok {
+		return nil
+	}
+	if decl.Tok != token.VAR {
+		return nil
+	}
+
+	// var declarations can be both freestanding and blocks (with parenthesis).
+	// Only the first variable spec following the directive is interesting.
+	var nextSpec ast.Spec
+	for _, s := range decl.Specs {
+		if com.End() < s.End() {
+			nextSpec = s
+			break
+		}
+	}
+	if nextSpec == nil {
+		return nil
+	}
+	spec, ok := nextSpec.(*ast.ValueSpec)
+	if !ok {
+		// Invalid AST, but keep going.
+		return nil
+	}
+	return spec
 }

gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_missing.go

@@ -0,0 +1,17 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package a
+
+import (
+	"fmt"
+)
+
+//go:embed embedtext // want "must import \"embed\" when using go:embed directives"
+var s string
+
+// This is main function
+func main() {
+	fmt.Println(s)
+}

gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present.go

@@ -0,0 +1,73 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package a
+
+// Misplaced, above imports.
+//go:embed embedText // want "go:embed directives must precede a \"var\" declaration"
+
+import (
+	"fmt"
+
+	_ "embed"
+)
+
+//go:embed embedText // ok
+var s string
+
+// The analyzer does not check for many directives using the same var.
+//
+//go:embed embedText // ok
+//go:embed embedText // ok
+var s string
+
+// Comments and blank lines between are OK.
+//
+//go:embed embedText // ok
+//
+// foo
+
+var s string
+
+// Followed by wrong kind of decl.
+//
+//go:embed embedText // want "go:embed directives must precede a \"var\" declaration"
+func foo()
+
+// Multiple variable specs.
+//
+//go:embed embedText // want "declarations following go:embed directives must define a single variable"
+var foo, bar []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"
+
+// TODO: This should not be OK, misplaced according to compiler.
+//
+//go:embed embedText // ok
+var (
+	s string
+	x string
+)
+
+// 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
+	q, r, t string
+)
+
+//go:embed embedText // want "go:embed directives must precede a \"var\" declaration"
+var ()
+
+// This is main function
+func main() {
+	fmt.Println(s)
+}
+
+// No declaration following.
+//go:embed embedText // want "go:embed directives must precede a \"var\" declaration"

gopls/internal/lsp/analysis/embeddirective/testdata/src/a/import_present_go120.go

@@ -0,0 +1,26 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build go1.20
+// +build go1.20
+
+package a
+
+var (
+	// Okay directive wise but the compiler will complain that
+	// imports must appear before other declarations.
+	//go:embed embedText // ok
+	"foo"
+)
+
+import (
+	"fmt"
+
+	_ "embed"
+)
+
+// This is main function
+func main() {
+	fmt.Println(s)
+}