Resolves #70: add stricter comment directive scan

comment.go

@@ -1,27 +1,74 @@
 package exhaustive
 
 import (
+	"fmt"
 	"go/ast"
 	"go/token"
 	"strings"
 )
 
 const (
-	ignoreComment                     = "//exhaustive:ignore"
-	enforceComment                    = "//exhaustive:enforce"
-	ignoreDefaultCaseRequiredComment  = "//exhaustive:ignore-default-case-required"
-	enforceDefaultCaseRequiredComment = "//exhaustive:enforce-default-case-required"
+	exhaustiveComment                 = "//exhaustive:"
+	ignoreComment                     = "ignore"
+	enforceComment                    = "enforce"
+	ignoreDefaultCaseRequiredComment  = "ignore-default-case-required"
+	enforceDefaultCaseRequiredComment = "enforce-default-case-required"
 )
 
-func hasCommentPrefix(comments []*ast.CommentGroup, comment string) bool {
-	for _, c := range comments {
-		for _, cc := range c.List {
-			if strings.HasPrefix(cc.Text, comment) {
-				return true
+type directive int64
+
+const (
+	ignoreDirective = 1 << iota
+	enforceDirective
+	ignoreDefaultCaseRequiredDirective
+	enforceDefaultCaseRequiredDirective
+)
+
+type directiveSet int64
+
+func parseDirectives(commentGroups []*ast.CommentGroup) (directiveSet, error) {
+	var out directiveSet
+	for _, commentGroup := range commentGroups {
+		for _, comment := range commentGroup.List {
+			commentLine := comment.Text
+			if !strings.HasPrefix(commentLine, exhaustiveComment) {
+				continue
+			}
+			directive := commentLine[len(exhaustiveComment):]
+			if whiteSpaceIndex := strings.IndexAny(directive, " \t"); whiteSpaceIndex != -1 {
+				directive = directive[:whiteSpaceIndex]
+			}
+			switch directive {
+			case ignoreComment:
+				out |= ignoreDirective
+			case enforceComment:
+				out |= enforceDirective
+			case ignoreDefaultCaseRequiredComment:
+				out |= ignoreDefaultCaseRequiredDirective
+			case enforceDefaultCaseRequiredComment:
+				out |= enforceDefaultCaseRequiredDirective
+			default:
+				return out, fmt.Errorf("invalid directive %q", directive)
 			}
 		}
 	}
-	return false
+	return out, out.validate()
+}
+
+func (d directiveSet) has(directive directive) bool {
+	return int64(d)&int64(directive) != 0
+}
+
+func (d directiveSet) validate() error {
+	enforceConflict := ignoreDirective | enforceDirective
+	if d&(directiveSet(enforceConflict)) == directiveSet(enforceConflict) {
+		return fmt.Errorf("conflicting directives %q and %q", ignoreComment, enforceComment)
+	}
+	defaultCaseRequiredConflict := ignoreDefaultCaseRequiredDirective | enforceDefaultCaseRequiredDirective
+	if d&(directiveSet(defaultCaseRequiredConflict)) == directiveSet(defaultCaseRequiredConflict) {
+		return fmt.Errorf("conflicting directives %q and %q", ignoreDefaultCaseRequiredComment, enforceDefaultCaseRequiredComment)
+	}
+	return nil
 }
 
 func fileCommentMap(fset *token.FileSet, file *ast.File) ast.CommentMap {

comment_test.go

@@ -1,6 +1,7 @@
 package exhaustive
 
 import (
+	"go/ast"
 	"go/parser"
 	"go/token"
 	"testing"
@@ -32,3 +33,151 @@ func f() {
 		}
 	})
 }
+
+func TestDirectives(t *testing.T) {
+	t.Run("no directives", func(t *testing.T) {
+		commentGroups := []*ast.CommentGroup{
+			{
+				List: []*ast.Comment{
+					{
+						Text: "//exhauster:foo",
+					},
+				},
+			},
+			{
+				List: []*ast.Comment{
+					{
+						Text: "//bar:value",
+					},
+					{
+						Text: "Another comment",
+					},
+				},
+			},
+		}
+		directives, err := parseDirectives(commentGroups)
+		if err != nil {
+			t.Errorf("unexpected error: %s", err)
+		}
+		if directives != 0 {
+			t.Errorf("unexpected directives: %d", directives)
+		}
+	})
+
+	t.Run("invalid directive", func(t *testing.T) {
+		commentGroups := []*ast.CommentGroup{
+			{
+				List: []*ast.Comment{
+					{
+						Text: "//exhauster:foo",
+					},
+				},
+			},
+			{
+				List: []*ast.Comment{
+					{
+						Text: "//exhaustive:enfocre",
+					},
+				},
+			},
+		}
+		_, err := parseDirectives(commentGroups)
+		if err == nil {
+			t.Errorf("expectedly no error on invalid directive")
+			return
+		}
+		if err.Error() != `invalid directive "enfocre"` {
+			t.Errorf("unexpected error: %s", err)
+		}
+	})
+
+	t.Run("conflicting enforcement directives", func(t *testing.T) {
+		commentGroups := []*ast.CommentGroup{
+			{
+				List: []*ast.Comment{
+					{
+						Text: "//exhaustive:ignore",
+					},
+					{
+						Text: "//exhaustive:enforce",
+					},
+				},
+			},
+			{
+				List: []*ast.Comment{
+					{
+						Text: "//exhaustive:ignore-default-case-required",
+					},
+				},
+			},
+		}
+		_, err := parseDirectives(commentGroups)
+		if err == nil {
+			t.Errorf("unexpectedly no error on conflicting directives")
+			return
+		}
+		if err.Error() != `conflicting directives "ignore" and "enforce"` {
+			t.Errorf("unexpected error: %s", err)
+		}
+	})
+
+	t.Run("conflicting default case enforcement directives", func(t *testing.T) {
+		commentGroups := []*ast.CommentGroup{
+			{
+				List: []*ast.Comment{
+					{
+						Text: "//exhaustive:enforce",
+					},
+				},
+			},
+			{
+				List: []*ast.Comment{
+					{
+						Text: "//exhaustive:ignore-default-case-required",
+					},
+					{
+						Text: "//exhaustive:enforce-default-case-required",
+					},
+					{
+						Text: "//exhaustive:ignore-default-case-required",
+					},
+				},
+			},
+		}
+		_, err := parseDirectives(commentGroups)
+		if err == nil {
+			t.Errorf("unexpectedly no error on conflicting directives")
+			return
+		}
+
+	})
+
+	t.Run("directives with trailing text and surrounding non-directives", func(t *testing.T) {
+		commentGroups := []*ast.CommentGroup{
+			{
+				List: []*ast.Comment{
+					{
+						Text: "Nevermind this comment",
+					},
+					{
+						Text: "//exhaustive:ignore foo",
+					},
+					{
+						Text: "//exhaustive:enforce-default-case-required\tbar",
+					},
+					{
+						Text: "This comment doesn't matter",
+					},
+				},
+			},
+		}
+
+		directives, err := parseDirectives(commentGroups)
+		if err != nil {
+			t.Errorf("unexpected error: %s", err)
+		}
+		if directives != ignoreDirective|enforceDefaultCaseRequiredDirective {
+			t.Errorf("unexpected directives: %d", directives)
+		}
+	})
+}

map.go

@@ -77,13 +77,18 @@ func mapChecker(pass *analysis.Pass, cfg mapConfig, generated boolCache, comment
 			}
 		}
 
-		if !cfg.explicit && hasCommentPrefix(relatedComments, ignoreComment) {
+		directives, err := parseDirectives(relatedComments)
+		if err != nil {
+			pass.Report(makeInvalidDirectiveDiagnostic(lit, err))
+		}
+
+		if !cfg.explicit && directives.has(ignoreDirective) {
 			// Skip checking of this map literal due to ignore
 			// comment. Still return true because there may be nested
 			// map literals that are not to be ignored.
 			return true, resultIgnoreComment
 		}
-		if cfg.explicit && !hasCommentPrefix(relatedComments, enforceComment) {
+		if cfg.explicit && !directives.has(enforceDirective) {
 			return true, resultNoEnforceComment
 		}
 

switch.go

@@ -5,7 +5,6 @@ import (
 	"go/ast"
 	"go/types"
 	"regexp"
-	"strings"
 
 	"golang.org/x/tools/go/analysis"
 )
@@ -60,41 +59,6 @@ type switchConfig struct {
 	ignoreType                 *regexp.Regexp // can be nil
 }
 
-// There are few possibilities, and often none, so we use a possibly-nil slice
-func userDirectives(comments []*ast.CommentGroup) []string {
-	var directives []string
-	for _, c := range comments {
-		for _, cc := range c.List {
-			// The order matters here: we always want to check the longest first.
-			for _, d := range []string{
-				enforceDefaultCaseRequiredComment,
-				ignoreDefaultCaseRequiredComment,
-				enforceComment,
-				ignoreComment,
-			} {
-				if strings.HasPrefix(cc.Text, d) {
-					directives = append(directives, d)
-					// The break here is important: once we associate a comment
-					// with a particular (longest-possible) directive, we don't want
-					// to map to another!
-					break
-				}
-			}
-		}
-	}
-	return directives
-}
-
-// Can be replaced with slices.Contains with go1.21
-func directivesIncludes(directives []string, d string) bool {
-	for _, ud := range directives {
-		if ud == d {
-			return true
-		}
-	}
-	return false
-}
-
 // switchChecker returns a node visitor that checks exhaustiveness of
 // enum switch statements for the supplied pass, and reports
 // diagnostics. The node visitor expects only *ast.SwitchStmt nodes.
@@ -118,30 +82,34 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c
 		sw := n.(*ast.SwitchStmt)
 
 		switchComments := comments.get(pass.Fset, file)[sw]
-		uDirectives := userDirectives(switchComments)
-		if !cfg.explicit && directivesIncludes(uDirectives, ignoreComment) {
+		uDirectives, err := parseDirectives(switchComments)
+		if err != nil {
+			pass.Report(makeInvalidDirectiveDiagnostic(sw, err))
+		}
+
+		if !cfg.explicit && uDirectives.has(ignoreDirective) {
 			// Skip checking of this switch statement due to ignore
 			// comment. Still return true because there may be nested
 			// switch statements that are not to be ignored.
 			return true, resultIgnoreComment
 		}
-		if cfg.explicit && !directivesIncludes(uDirectives, enforceComment) {
+		if cfg.explicit && !uDirectives.has(enforceDirective) {
 			// Skip checking of this switch statement due to missing
 			// enforce comment.
 			return true, resultNoEnforceComment
 		}
 		requireDefaultCase := cfg.defaultCaseRequired
-		if directivesIncludes(uDirectives, ignoreDefaultCaseRequiredComment) {
+		if uDirectives.has(ignoreDefaultCaseRequiredDirective) {
 			requireDefaultCase = false
 		}
-		if directivesIncludes(uDirectives, enforceDefaultCaseRequiredComment) {
+		if uDirectives.has(enforceDefaultCaseRequiredDirective) {
 			// We have "if" instead of "else if" here in case of conflicting ignore/enforce directives.
 			// In that case, because this is second, we will default to enforcing.
 			requireDefaultCase = true
 		}
 
 		if sw.Tag == nil {
-			return true, resultNoSwitchTag // never possible for valid Go program?
+			return true, resultNoSwitchTag
 		}
 
 		t := pass.TypesInfo.Types[sw.Tag]
@@ -169,6 +137,7 @@ func switchChecker(pass *analysis.Pass, cfg switchConfig, generated boolCache, c
 			// to have a default case. We check this first to avoid
 			// early-outs
 			pass.Report(makeMissingDefaultDiagnostic(sw, dedupEnumTypes(toEnumTypes(es))))
+
 			return true, resultMissingDefaultCase
 		}
 		if len(checkl.remaining()) == 0 {
@@ -234,3 +203,14 @@ func makeMissingDefaultDiagnostic(sw *ast.SwitchStmt, enumTypes []enumType) anal
 		),
 	}
 }
+
+func makeInvalidDirectiveDiagnostic(node ast.Node, err error) analysis.Diagnostic {
+	return analysis.Diagnostic{
+		Pos: node.Pos(),
+		End: node.End(),
+		Message: fmt.Sprintf(
+			"failed to parse directives: %s",
+			err.Error(),
+		),
+	}
+}