feat(vet): Allow selective disabling of rules per query (#3620)

* Extended the `@sqlc-vet-disable` flag to support the skipping of provided rules.
This is an update to the original behaviour which had an "all-or-nothing" behaviour, either running all the rules on a query or skipping them all.
Specified rules which aren't declared in the sqlc config file get rejected.

* Updated tests to cover new vet disabling behaviour.

* Relocated constants for query flags to a dedicated package to allow for safer reuse.

* Renamed the metadata `ParseParamsAndFlags()` function to `ParseCommentFlags()` in order to be more generic as it now returns the rule skip list and could potentially return more data in the future.

* Updated docs to reflect the support of rule names for the vet disable annotation.

* Slight term change in vet docs.

* Renamed test function for query rule skiplist.

Author: rhodeon

docs/howto/vet.md

@@ -259,7 +259,24 @@ rules:
 ### Opting-out of lint rules
 
 For any query, you can tell `sqlc vet` not to evaluate lint rules using the
-`@sqlc-vet-disable` query annotation.
+`@sqlc-vet-disable` query annotation. The annotation accepts a list of rules to ignore.
+
+```sql
+/* name: GetAuthor :one */
+/* @sqlc-vet-disable sqlc/db-prepare no-pg */
+SELECT * FROM authors
+WHERE id = ? LIMIT 1;
+```
+The rules can also be split across lines.
+```sql
+/* name: GetAuthor :one */
+/* @sqlc-vet-disable sqlc/db-prepare */
+/* @sqlc-vet-disable no-pg */
+SELECT * FROM authors
+WHERE id = ? LIMIT 1;
+```
+
+To skip all rules for a query, you can provide the `@sqlc-vet-disable` annotation without any parameters. 
 
 ```sql
 /* name: GetAuthor :one */

internal/cmd/vet.go

@@ -6,11 +6,13 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"github.com/sqlc-dev/sqlc/internal/constants"
 	"io"
 	"log"
 	"os"
 	"path/filepath"
 	"runtime/trace"
+	"slices"
 	"strings"
 	"time"
 
@@ -37,9 +39,6 @@ var ErrFailedChecks = errors.New("failed checks")
 
 var pjson = protojson.UnmarshalOptions{AllowPartial: true, DiscardUnknown: true}
 
-const RuleDbPrepare = "sqlc/db-prepare"
-const QueryFlagSqlcVetDisable = "@sqlc-vet-disable"
-
 func NewCmdVet() *cobra.Command {
 	return &cobra.Command{
 		Use:   "vet",
@@ -109,7 +108,7 @@ func Vet(ctx context.Context, dir, filename string, opts *Options) error {
 	}
 
 	rules := map[string]rule{
-		RuleDbPrepare: {NeedsPrepare: true},
+		constants.QueryRuleDbPrepare: {NeedsPrepare: true},
 	}
 
 	for _, c := range conf.Rules {
@@ -538,11 +537,23 @@ func (c *checker) checkSQL(ctx context.Context, s config.SQL) error {
 	req := codeGenRequest(result, combo)
 	cfg := vetConfig(req)
 	for i, query := range req.Queries {
-		if result.Queries[i].Metadata.Flags[QueryFlagSqlcVetDisable] {
-			if debug.Active {
-				log.Printf("Skipping vet rules for query: %s\n", query.Name)
+		md := result.Queries[i].Metadata
+		if md.Flags[constants.QueryFlagSqlcVetDisable] {
+			// If the vet disable flag is specified without any rules listed, all rules are ignored.
+			if len(md.RuleSkiplist) == 0 {
+				if debug.Active {
+					log.Printf("Skipping all vet rules for query: %s\n", query.Name)
+				}
+				continue
+			}
+
+			// Rules which are listed to be disabled but not declared in the config file are rejected.
+			for r := range md.RuleSkiplist {
+				if !slices.Contains(s.Rules, r) {
+					fmt.Fprintf(c.Stderr, "%s: %s: rule-check error: rule %q does not exist in the config file\n", query.Filename, query.Name, r)
+					errored = true
+				}
 			}
-			continue
 		}
 
 		evalMap := map[string]any{
@@ -551,74 +562,81 @@ func (c *checker) checkSQL(ctx context.Context, s config.SQL) error {
 		}
 
 		for _, name := range s.Rules {
-			rule, ok := c.Rules[name]
-			if !ok {
-				return fmt.Errorf("type-check error: a rule with the name '%s' does not exist", name)
-			}
-
-			if rule.NeedsPrepare {
-				if prep == nil {
-					fmt.Fprintf(c.Stderr, "%s: %s: %s: error preparing query: database connection required\n", query.Filename, query.Name, name)
-					errored = true
-					continue
+			if _, skip := md.RuleSkiplist[name]; skip {
+				if debug.Active {
+					log.Printf("Skipping vet rule %q for query: %s\n", name, query.Name)
 				}
-				prepName := fmt.Sprintf("sqlc_vet_%d_%d", time.Now().Unix(), i)
-				if err := prep.Prepare(ctx, prepName, query.Text); err != nil {
-					fmt.Fprintf(c.Stderr, "%s: %s: %s: error preparing query: %s\n", query.Filename, query.Name, name, err)
-					errored = true
-					continue
+			} else {
+				rule, ok := c.Rules[name]
+				if !ok {
+					return fmt.Errorf("type-check error: a rule with the name '%s' does not exist", name)
 				}
-			}
-
-			// short-circuit for "sqlc/db-prepare" rule which doesn't have a CEL program
-			if rule.Program == nil {
-				continue
-			}
 
-			// Get explain output for this query if we need it
-			_, pgsqlOK := evalMap["postgresql"]
-			_, mysqlOK := evalMap["mysql"]
-			if rule.NeedsExplain && !(pgsqlOK || mysqlOK) {
-				if expl == nil {
-					fmt.Fprintf(c.Stderr, "%s: %s: %s: error explaining query: database connection required\n", query.Filename, query.Name, name)
-					errored = true
-					continue
+				if rule.NeedsPrepare {
+					if prep == nil {
+						fmt.Fprintf(c.Stderr, "%s: %s: %s: error preparing query: database connection required\n", query.Filename, query.Name, name)
+						errored = true
+						continue
+					}
+					prepName := fmt.Sprintf("sqlc_vet_%d_%d", time.Now().Unix(), i)
+					if err := prep.Prepare(ctx, prepName, query.Text); err != nil {
+						fmt.Fprintf(c.Stderr, "%s: %s: %s: error preparing query: %s\n", query.Filename, query.Name, name, err)
+						errored = true
+						continue
+					}
 				}
-				engineOutput, err := expl.Explain(ctx, query.Text, query.Params...)
-				if err != nil {
-					fmt.Fprintf(c.Stderr, "%s: %s: %s: error explaining query: %s\n", query.Filename, query.Name, name, err)
-					errored = true
+
+				// short-circuit for "sqlc/db-prepare" rule which doesn't have a CEL program
+				if rule.Program == nil {
 					continue
 				}
 
-				evalMap["postgresql"] = engineOutput.PostgreSQL
-				evalMap["mysql"] = engineOutput.MySQL
-			}
+				// Get explain output for this query if we need it
+				_, pgsqlOK := evalMap["postgresql"]
+				_, mysqlOK := evalMap["mysql"]
+				if rule.NeedsExplain && !(pgsqlOK || mysqlOK) {
+					if expl == nil {
+						fmt.Fprintf(c.Stderr, "%s: %s: %s: error explaining query: database connection required\n", query.Filename, query.Name, name)
+						errored = true
+						continue
+					}
+					engineOutput, err := expl.Explain(ctx, query.Text, query.Params...)
+					if err != nil {
+						fmt.Fprintf(c.Stderr, "%s: %s: %s: error explaining query: %s\n", query.Filename, query.Name, name, err)
+						errored = true
+						continue
+					}
+
+					evalMap["postgresql"] = engineOutput.PostgreSQL
+					evalMap["mysql"] = engineOutput.MySQL
+				}
 
-			if debug.Debug.DumpVetEnv {
-				fmt.Printf("vars for rule '%s' evaluating against query '%s':\n", name, query.Name)
-				debug.DumpAsJSON(evalMap)
-			}
+				if debug.Debug.DumpVetEnv {
+					fmt.Printf("vars for rule '%s' evaluating against query '%s':\n", name, query.Name)
+					debug.DumpAsJSON(evalMap)
+				}
 
-			out, _, err := (*rule.Program).Eval(evalMap)
-			if err != nil {
-				return err
-			}
-			tripped, ok := out.Value().(bool)
-			if !ok {
-				return fmt.Errorf("expression returned non-bool value: %v", out.Value())
-			}
-			if tripped {
-				// TODO: Get line numbers in the output
-				if rule.Message == "" {
-					fmt.Fprintf(c.Stderr, "%s: %s: %s\n", query.Filename, query.Name, name)
-				} else {
-					fmt.Fprintf(c.Stderr, "%s: %s: %s: %s\n", query.Filename, query.Name, name, rule.Message)
+				out, _, err := (*rule.Program).Eval(evalMap)
+				if err != nil {
+					return err
+				}
+				tripped, ok := out.Value().(bool)
+				if !ok {
+					return fmt.Errorf("expression returned non-bool value: %v", out.Value())
+				}
+				if tripped {
+					// TODO: Get line numbers in the output
+					if rule.Message == "" {
+						fmt.Fprintf(c.Stderr, "%s: %s: %s\n", query.Filename, query.Name, name)
+					} else {
+						fmt.Fprintf(c.Stderr, "%s: %s: %s: %s\n", query.Filename, query.Name, name, rule.Message)
+					}
+					errored = true
 				}
-				errored = true
 			}
 		}
 	}
+
 	if errored {
 		return ErrFailedChecks
 	}

internal/compiler/parse.go

@@ -65,7 +65,7 @@ func (c *Compiler) parseQuery(stmt ast.Node, src string, o opts.Parser) (*Query,
 		return nil, err
 	}
 
-	md.Params, md.Flags, err = metadata.ParseParamsAndFlags(cleanedComments)
+	md.Params, md.Flags, md.RuleSkiplist, err = metadata.ParseCommentFlags(cleanedComments)
 	if err != nil {
 		return nil, err
 	}

internal/constants/query.go

@@ -0,0 +1,12 @@
+package constants
+
+// Flags
+const (
+	QueryFlagParam          = "@param"
+	QueryFlagSqlcVetDisable = "@sqlc-vet-disable"
+)
+
+// Rules
+const (
+	QueryRuleDbPrepare = "sqlc/db-prepare"
+)

internal/endtoend/testdata/vet_disable/query.sql

@@ -1,6 +1,29 @@
--- name: SkipVet :exec
+-- name: RunVetAll :exec
+SELECT true;
+
+-- name: SkipVetAll :exec
 -- @sqlc-vet-disable
 SELECT true;
 
--- name: RunVet :exec
+-- name: SkipVetSingleLine :exec
+-- @sqlc-vet-disable always-fail no-exec
+SELECT true;
+
+-- name: SkipVetMultiLine :exec
+-- @sqlc-vet-disable always-fail
+-- @sqlc-vet-disable no-exec
+SELECT true;
+
+-- name: SkipVet_always_fail :exec
+-- @sqlc-vet-disable always-fail
+SELECT true;
+
+-- name: SkipVet_no_exec :exec
+-- @sqlc-vet-disable no-exec
+SELECT true;
+
+-- name: SkipVetInvalidRule :exec
+-- @sqlc-vet-disable always-fail
+-- @sqlc-vet-disable block-delete
+-- @sqlc-vet-disable no-exec
 SELECT true;

internal/endtoend/testdata/vet_disable/sqlc.yaml

@@ -9,7 +9,13 @@ sql:
         out: "db"
     rules:
       - always-fail
+      - no-exec
 rules:
   - name: always-fail
     message: "Fail"
     rule: "true"
+
+  - name: no-exec
+    message: "don't use exec"
+    rule: |
+      query.cmd == "exec"

internal/endtoend/testdata/vet_disable/stderr.txt

@@ -1 +1,5 @@
-query.sql: RunVet: always-fail: Fail
+query.sql: RunVetAll: always-fail: Fail
+query.sql: RunVetAll: no-exec: don't use exec
+query.sql: SkipVet_always_fail: no-exec: don't use exec
+query.sql: SkipVet_no_exec: always-fail: Fail
+query.sql: SkipVetInvalidRule: rule-check error: rule "block-delete" does not exist in the config file

internal/metadata/meta.go

@@ -3,6 +3,7 @@ package metadata
 import (
 	"bufio"
 	"fmt"
+	"github.com/sqlc-dev/sqlc/internal/constants"
 	"strings"
 	"unicode"
 
@@ -18,6 +19,10 @@ type Metadata struct {
 	Params   map[string]string
 	Flags    map[string]bool
 
+	// RuleSkiplist contains the names of rules to disable vetting for.
+	// If the map is empty, but the disable vet flag is specified, then all rules are ignored.
+	RuleSkiplist map[string]struct{}
+
 	Filename string
 }
 
@@ -113,9 +118,12 @@ func ParseQueryNameAndType(t string, commentStyle CommentSyntax) (string, string
 	return "", "", nil
 }
 
-func ParseParamsAndFlags(comments []string) (map[string]string, map[string]bool, error) {
+// ParseCommentFlags processes the comments provided with queries to determine the metadata params, flags and rules to skip.
+// All flags in query comments are prefixed with `@`, e.g. @param, @@sqlc-vet-disable.
+func ParseCommentFlags(comments []string) (map[string]string, map[string]bool, map[string]struct{}, error) {
 	params := make(map[string]string)
 	flags := make(map[string]bool)
+	ruleSkiplist := make(map[string]struct{})
 
 	for _, line := range comments {
 		s := bufio.NewScanner(strings.NewReader(line))
@@ -129,7 +137,7 @@ func ParseParamsAndFlags(comments []string) (map[string]string, map[string]bool,
 		}
 
 		switch token {
-		case "@param":
+		case constants.QueryFlagParam:
 			s.Scan()
 			name := s.Text()
 			var rest []string
@@ -138,14 +146,27 @@ func ParseParamsAndFlags(comments []string) (map[string]string, map[string]bool,
 				rest = append(rest, paramToken)
 			}
 			params[name] = strings.Join(rest, " ")
+
+		case constants.QueryFlagSqlcVetDisable:
+			flags[token] = true
+
+			// Vet rules can all be disabled in the same line or split across lines .i.e.
+			// /* @sqlc-vet-disable sqlc/db-prepare delete-without-where */
+			// is equivalent to:
+			// /* @sqlc-vet-disable sqlc/db-prepare */
+			// /* @sqlc-vet-disable delete-without-where */
+			for s.Scan() {
+				ruleSkiplist[s.Text()] = struct{}{}
+			}
+
 		default:
 			flags[token] = true
 		}
 
 		if s.Err() != nil {
-			return params, flags, s.Err()
+			return params, flags, ruleSkiplist, s.Err()
 		}
 	}
 
-	return params, flags, nil
+	return params, flags, ruleSkiplist, nil
 }