refuse to shadow a definition the binary carries

`generate` checked the per-user directory and nothing else, so running it
on a board this binary carries a definition for would write a scaffold
that shadows it. A file in the user directory wins, so the Impact 80
would have gone from 60 named effects to `0 effects` — and the scaffold
is the file that is then read, so nothing would have reported the loss.
The user asked whether generate checks for an overwrite, and the answer
was: not this one.

The refusal names how many effects are at stake rather than only saying
no, because the decision is the user's and it needs a number to make.

`fetch` shadows a built-in definition too and keeps being allowed it,
and the asymmetry is the point: a fetched file is the manufacturer's own
and carries more than a scaffold does, so one replaces and the other
replaces with less. That is said in the code and in the README, because
it looks arbitrary from the outside.

README.md

@@ -562,6 +562,16 @@ option with an empty name is a slot without a name, so the channel reports
 board was already in. Open the file, write a name into each `options` entry, and
 the channel resolves from then on.
 
+**It refuses to run when there is something to lose.** A definition already in the
+per-user directory is left alone, because that is the file you would have run this
+command to write. So is a definition the binary carries, and that one matters more
+than it looks: a file in the per-user directory shadows the built-in one, so a
+scaffold written for the Impact 80 would take it from 60 named effects to none —
+and the scaffold is the file that would be read, so nothing would report the loss.
+`--force` does it anyway. `keyboard fetch` shadows a built-in file too and is
+allowed it, because a fetched file is the manufacturer's own and carries more than
+a scaffold does.
+
 The note carries every candidate with the number of boards that wrote it and the
 manufacturer behind most of them, because that is what separates a name from a
 guess:

cmd/qmk-rgb-tool/definition_generate.go

@@ -90,6 +90,26 @@ func runKeyboardDefinitionsGenerate(cmd *cobra.Command, args []string) error {
 			stored[0].Name, target.Device.VendorID, target.Device.ProductID, describeDataDir(stored[0].Path))
 	}
 
+	// The built-in set is checked as well, and for a different reason than the
+	// directory above. A definition the binary carries is the vendor's own file and
+	// the user directory shadows it, so a scaffold written for such a board would
+	// take it from the names it has to none — a loss the generated file could not
+	// report, because the file is the one that is read. `fetch` has the same
+	// shadowing and is allowed it, because a fetched file is the vendor's file too
+	// and carries more than a scaffold does.
+	if !generateForce {
+		for _, def := range builtInDefinitions() {
+			if !def.Matches(target.Device.VendorID, target.Device.ProductID) {
+				continue
+			}
+			return fmt.Errorf("this binary carries a definition for %s (0x%04X/0x%04X) that names %d "+
+				"effects, and a file in the definitions directory would shadow it; a generated one names "+
+				"none, so the board would go from %d names to no effect names at all, use --force to do "+
+				"that anyway, or edit the built-in file's copy in the definitions directory",
+				def.Name, def.VendorID, def.ProductID, namedEffectCount(def), namedEffectCount(def))
+		}
+	}
+
 	if len(channels) == 0 {
 		return fmt.Errorf("this keyboard exposes no VIA lighting channels, so there is nothing to " +
 			"generate a definition for")
@@ -280,3 +300,14 @@ func writeCandidates(out *strings.Builder, ch via.Channel, top int) {
 		fmt.Fprintf(out, "  no definition anywhere names an effect on this channel\n")
 	}
 }
+
+// namedEffectCount is how many effects a definition names across the channels it
+// covers. A file is a board's whole catalog, so the number the user loses is the
+// total and not the largest channel.
+func namedEffectCount(def *intrgb.Definition) int {
+	total := 0
+	for _, ch := range via.LightingChannels {
+		total += len(def.Catalog.Effects(ch))
+	}
+	return total
+}

cmd/qmk-rgb-tool/definition_generate_test.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -31,12 +32,16 @@ func (g generateStub) DetectChannels() ([]intvia.Channel, error) { return nil, n
 
 func (g generateStub) Close() error { return nil }
 
-func stubGenerateTarget(t *testing.T, channels []intvia.Channel, tops map[intvia.Channel]int) {
+// stubGenerateTarget makes the connected keyboard a fixed one. The identifiers
+// are a parameter because a board the binary carries a definition for is the
+// case `generate` has to refuse, and a stub for the wrong board would test
+// nothing.
+func stubGenerateTarget(t *testing.T, vendorID, productID uint16, channels []intvia.Channel, tops map[intvia.Channel]int) {
 	t.Helper()
 	original := openTarget
 	t.Cleanup(func() { openTarget = original })
 	openTarget = func(string) (rgbProtocol, targetDeviceData, []intvia.Channel, error) {
-		return generateStub{tops: tops}, stubTargetData(0x36B0, 0x309F), channels, nil
+		return generateStub{tops: tops}, stubTargetData(vendorID, productID), channels, nil
 	}
 }
 
@@ -55,7 +60,7 @@ func TestGeneratedDefinitionNamesNoEffectUntilTheUserFillsItIn(t *testing.T) {
 	t.Cleanup(definitionFlagRestore(t))
 	t.Cleanup(forceDefinitionsDir(t, dir))
 	t.Cleanup(forceGenerateRestore(t))
-	stubGenerateTarget(t, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
+	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
 
 	cmd := NewKeyboardDefinitionsGenerateCmd()
 	var out, errOut strings.Builder
@@ -146,9 +151,9 @@ func TestGenerateDoesNotReplaceAStoredDefinition(t *testing.T) {
 	t.Cleanup(definitionFlagRestore(t))
 	t.Cleanup(forceDefinitionsDir(t, dir))
 	t.Cleanup(forceGenerateRestore(t))
-	stubGenerateTarget(t, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
+	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
 
-	written := `{"name":"Test Board","vendorId":"0x36B0","productId":"0x309F","menus":[],"note":"hand written"}`
+	written := `{"name":"Test Board","vendorId":"0x1234","productId":"0x5678","menus":[],"note":"hand written"}`
 	if err := os.WriteFile(filepath.Join(dir, "test_board.json"), []byte(written), 0o644); err != nil {
 		t.Fatal(err)
 	}
@@ -178,7 +183,7 @@ func TestGeneratedNoteCarriesTheSpellingsAndWhoWroteThem(t *testing.T) {
 	t.Cleanup(definitionFlagRestore(t))
 	t.Cleanup(forceDefinitionsDir(t, dir))
 	t.Cleanup(forceGenerateRestore(t))
-	stubGenerateTarget(t, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
+	stubGenerateTarget(t, 0x1234, 0x5678, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
 
 	cmd := NewKeyboardDefinitionsGenerateCmd()
 	var out, errOut strings.Builder
@@ -230,3 +235,45 @@ func mustRead(t *testing.T, path string) string {
 	}
 	return string(data)
 }
+
+// A generated scaffold names no effect, so writing one for a board the binary
+// carries a definition for takes that board from 46 named effects to none — and
+// the file in the user directory is the one that is read. This is the whole
+// reason the built-in set is checked and not only the directory.
+func TestGenerateDoesNotShadowABuiltInDefinition(t *testing.T) {
+	dir := t.TempDir()
+	t.Cleanup(definitionFlagRestore(t))
+	t.Cleanup(forceDefinitionsDir(t, dir))
+	t.Cleanup(forceGenerateRestore(t))
+	// The Impact 80, whose definition is built into the binary.
+	stubGenerateTarget(t, 0x36B0, 0x309F, []intvia.Channel{intvia.ChannelRgbMatrix}, map[intvia.Channel]int{intvia.ChannelRgbMatrix: 45})
+
+	builtIn := 0
+	for _, def := range builtInDefinitions() {
+		if def.Matches(0x36B0, 0x309F) {
+			builtIn = namedEffectCount(def)
+		}
+	}
+	if builtIn == 0 {
+		t.Fatal("the binary carries no definition for 0x36B0/0x309F, so there is nothing to shadow")
+	}
+
+	cmd := NewKeyboardDefinitionsGenerateCmd()
+	var out, errOut strings.Builder
+	cmd.SetOut(&out)
+	cmd.SetErr(&errOut)
+
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("definitions generate = nil error, want a refusal to shadow a built-in definition")
+	}
+	if !strings.Contains(err.Error(), "carries a definition") {
+		t.Errorf("error = %q, want it to say the definition is one the binary carries", err)
+	}
+	if !strings.Contains(err.Error(), strconv.Itoa(builtIn)) {
+		t.Errorf("error = %q, want it to name how many effects would be lost (%d)", err, builtIn)
+	}
+	if entries, _ := filepath.Glob(filepath.Join(dir, "*.json")); len(entries) != 0 {
+		t.Errorf("definitions dir = %v, want nothing written", entries)
+	}
+}