Repository: DestinyItemManager/DIM (ts)

Issue:
Clashing filter keywords `light`, `power`
This filter:

https://github.com/DestinyItemManager/DIM/blob/eb3b6e0a6505552463265eed745956cf7c5cb6d5/src/app/search/search-filters/range-overload.tsx#L98-L109

clashes with and ends up overriding keywords of these filters:

https://github.com/DestinyItemManager/DIM/blob/eb3b6e0a6505552463265eed745956cf7c5cb6d5/src/app/search/search-filters/known-values.tsx#L139-L143

https://github.com/DestinyItemManager/DIM/blob/eb3b6e0a6505552463265eed745956cf7c5cb6d5/src/app/search/search-filters/known-values.tsx#L67-L77

In practice this means that `is:light` and `is:power` are invalid filter syntaxes and throw an error because the first linked filter is expecting range syntax, despite autocompletion and filters help suggesting them.

Pull request description:
Separate filter namespaces between `is:` and `key:value` filters
This means that filter definitions with overlapping keywords can coexist as long as one of them uses only the `is:` format and the other doesn't do that, and as such fixes #7813. This removes the `is:light` variant of the `haslight`/`haspower` filter for symmetry reasons -- it has been broken for a while anyway.

In terms of validation, this also starts rejecting `key:value` syntax for `is:` filters and `is:` syntax for `key:value` filters.

Finally, this adds some tests so that filters that conflict in some way don't silently do something unexpected,

Patch:
diff --git a/src/app/search/autocomplete.ts b/src/app/search/autocomplete.ts
index b1af6b694..d027ac22b 100644
--- a/src/app/search/autocomplete.ts
+++ b/src/app/search/autocomplete.ts
@@ -221,14 +221,12 @@ export function autocompleteTermSuggestions(
 
 function findFilter(term: string, searchConfig: SearchConfig) {
   const parts = term.split(':');
-  let filterName = parts[0];
+  const filterName = parts[0];
   const filterValue = parts[1];
   // "is:" filters are slightly special cased
-  if (filterName === 'is') {
-    filterName = filterValue;
-  }
-
-  return searchConfig.filters[filterName];
+  return filterName === 'is'
+    ? searchConfig.isFilters[filterValue]
+    : searchConfig.kvFilters[filterName];
 }
 
 // these filters might include quotes, so we search for two text segments to ignore quotes & colon
@@ -273,7 +271,7 @@ export function makeFilterComplete(searchConfig: SearchConfig) {
     // and "stat" matches "stat:" and "basestat:"
     const matchType = !mustStartWith && typedToLower.includes(':') ? 'startsWith' : 'includes';
 
-    let suggestions = searchConfig.keywords
+    let suggestions = searchConfig.suggestions
       .filter((word) => word.startsWith(mustStartWith) && word[matchType](typedToLower))
       .filter(filterLowPrioritySuggestions);
 
diff --git a/src/app/search/search-config.ts b/src/app/search/search-config.ts
index 070154ded..528390c65 100644
--- a/src/app/search/search-config.ts
+++ b/src/app/search/search-config.ts
@@ -1,7 +1,7 @@
 import { DestinyVersion } from '@destinyitemmanager/dim-api-types';
 import { destinyVersionSelector } from 'app/accounts/selectors';
 import { createSelector } from 'reselect';
-import { FilterDefinition, SuggestionsContext } from './filter-types';
+import { canonicalFilterFormats, FilterDefinition, SuggestionsContext } from './filter-types';
 import advancedFilters from './search-filters/advanced';
 import d1Filters from './search-filters/d1-filters';
 import dupeFilters from './search-filters/dupes';
@@ -47,8 +47,11 @@ export const searchConfigSelector = createSelector(
 
 export interface SearchConfig {
   allFilters: FilterDefinition[];
-  filters: Record<string, FilterDefinition>;
-  keywords: string[];
+  /* `is:keyword` filters */
+  isFilters: Record<string, FilterDefinition>;
+  /* `keyword:value` filters */
+  kvFilters: Record<string, FilterDefinition>;
+  suggestions: string[];
 }
 
 /** Builds an object that describes the available search keywords and filter definitions. */
@@ -56,28 +59,49 @@ export function buildSearchConfig(
   destinyVersion: DestinyVersion,
   suggestionsContext: SuggestionsContext = {}
 ): SearchConfig {
-  const keywords = new Set<string>();
-  const allFiltersByKeyword: Record<string, FilterDefinition> = {};
+  const suggestions = new Set<string>();
+  const isFilters: Record<string, FilterDefinition> = {};
+  const kvFilters: Record<string, FilterDefinition> = {};
   const allApplicableFilters: FilterDefinition[] = [];
   for (const filter of allFilters) {
     if (!filter.destinyVersion || filter.destinyVersion === destinyVersion) {
-      for (const keyword of generateSuggestionsForFilter(filter)) {
-        keywords.add(keyword);
+      for (const suggestion of generateSuggestionsForFilter(filter)) {
+        suggestions.add(suggestion);
       }
-      for (const keyword of filter.suggestionsGenerator?.(suggestionsContext) ?? []) {
-        keywords.add(keyword);
+      for (const suggestion of filter.suggestionsGenerator?.(suggestionsContext) ?? []) {
+        suggestions.add(suggestion);
       }
       allApplicableFilters.push(filter);
       const filterKeywords = Array.isArray(filter.keywords) ? filter.keywords : [filter.keywords];
+      const filterFormats = canonicalFilterFormats(filter.format);
+      const hasSimple = filterFormats.some((f) => f === 'simple');
+      const hasKv = filterFormats.some((f) => f !== 'simple');
+
       for (const keyword of filterKeywords) {
-        allFiltersByKeyword[keyword] = filter;
+        if (hasSimple) {
+          if ($DIM_FLAVOR === 'test' && isFilters[keyword]) {
+            throw new Error(
+              `Conflicting is:${keyword} filter -- only the last inserted filter will work.`
+            );
+          }
+          isFilters[keyword] = filter;
+        }
+        if (hasKv) {
+          if ($DIM_FLAVOR === 'test' && kvFilters[keyword]) {
+            throw new Error(
+              `Conflicting ${keyword}:value filter -- only the last inserted filter will work.`
+            );
+          }
+          kvFilters[keyword] = filter;
+        }
       }
     }
   }
 
   return {
     allFilters: allApplicableFilters,
-    keywords: Array.from(keywords),
-    filters: allFiltersByKeyword,
+    suggestions: Array.from(suggestions),
+    isFilters,
+    kvFilters,
   };
 }
diff --git a/src/app/search/search-filter.ts b/src/app/search/search-filter.ts
index ee0a951e9..81d469803 100644
--- a/src/app/search/search-filter.ts
+++ b/src/app/search/search-filter.ts
@@ -73,7 +73,7 @@ export const validateQuerySelector = createSelector(
 );
 
 function makeSearchFilterFactory(
-  { filters }: SearchConfig,
+  { isFilters, kvFilters }: SearchConfig,
   stores: DimStore[],
   allItems: DimItem[],
   currentStore: DimStore,
@@ -139,15 +139,12 @@ function makeSearchFilterFactory(
           return (item) => !fn(item);
         }
         case 'filter': {
-          let filterName = ast.type;
+          const filterName = ast.type;
           const filterValue = ast.args;
 
           // "is:" filters are slightly special cased
-          if (filterName === 'is') {
-            filterName = filterValue;
-          }
+          const filterDef = filterName === 'is' ? isFilters[filterValue] : kvFilters[filterName];
 
-          const filterDef = filters[filterName];
           if (filterDef) {
             // Each filter knows how to generate a standalone item filter function
             try {
diff --git a/src/app/search/search-filters/known-values.tsx b/src/app/search/search-filters/known-values.tsx
index 69f7c0155..8cb5d75e3 100644
--- a/src/app/search/search-filters/known-values.tsx
+++ b/src/app/search/search-filters/known-values.tsx
@@ -137,7 +137,7 @@ const knownValuesFilters: FilterDefinition[] = [
     filter: () => (item) => cosmeticTypes.includes(item.bucket.hash),
   },
   {
-    keywords: ['light', 'haslight', 'haspower'],
+    keywords: ['haslight', 'haspower'],
     description: tl('Filter.ContributePower'),
     filter: () => (item) => item.power > 0,
   },
diff --git a/src/app/search/search-utils.ts b/src/app/search/search-utils.ts
index f0ac199f3..a3ad8903d 100644
--- a/src/app/search/search-utils.ts
+++ b/src/app/search/search-utils.ts
@@ -23,16 +23,16 @@ export function validateQuery(query: QueryAST, searchConfig: SearchConfig): bool
   }
   switch (query.op) {
     case 'filter': {
-      let filterName = query.type;
+      const filterName = query.type;
       const filterValue = query.args;
 
       // "is:" filters are slightly special cased
       if (filterName === 'is') {
-        filterName = filterValue;
+        return Boolean(searchConfig.isFilters[filterValue]);
+      } else {
+        // TODO: validate that filterValue is correct
+        return Boolean(searchConfig.kvFilters[filterName]);
       }
-
-      // TODO: validate that filterValue is correct
-      return Boolean(searchConfig.filters[filterName]);
     }
     case 'not':
       return validateQuery(query.operand, searchConfig);


Test patch:
diff --git a/src/app/search/__snapshots__/search-config.test.ts.snap b/src/app/search/__snapshots__/search-config.test.ts.snap
index 2ba8b6dcf..021bebbb7 100644
--- a/src/app/search/__snapshots__/search-config.test.ts.snap
+++ b/src/app/search/__snapshots__/search-config.test.ts.snap
@@ -1,16 +1,14 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`buildSearchConfig generates a reasonable filter map 1`] = `
+exports[`buildSearchConfig generates a reasonable filter map: is filters 1`] = `
 Array [
   "arc",
   "armor",
   "armor2.0",
   "armormod",
   "autorifle",
-  "basestat",
   "blue",
   "bow",
-  "breaker",
   "chest",
   "clanbanners",
   "class",
@@ -18,16 +16,13 @@ Array [
   "common",
   "consumables",
   "cosmetic",
-  "count",
   "curated",
   "currentclass",
   "customstatlower",
-  "description",
   "dupe",
   "dupelower",
   "emblems",
   "energy",
-  "energycapacity",
   "engrams",
   "equipment",
   "equippable",
@@ -40,7 +35,6 @@ Array [
   "green",
   "grenadelauncher",
   "handcannon",
-  "hash",
   "haslight",
   "hasmod",
   "hasnotes",
@@ -49,9 +43,7 @@ Array [
   "hasshader",
   "heavy",
   "helmet",
-  "holdsmod",
   "hunter",
-  "id",
   "incurrentchar",
   "infusable",
   "infuse",
@@ -62,41 +54,28 @@ Array [
   "inpostmaster",
   "inrightchar",
   "invault",
-  "keyword",
-  "kills",
   "kinetic",
   "kineticslot",
   "leg",
   "legendary",
-  "level",
-  "light",
   "linearfusionrifle",
   "locked",
   "lostitems",
   "machinegun",
   "masterwork",
-  "maxbasestatvalue",
   "maxpower",
   "maxpowerloadout",
-  "maxstatloadout",
-  "maxstatvalue",
   "messages",
   "modded",
   "modifications",
-  "modslot",
   "movable",
-  "name",
   "new",
-  "notes",
   "onwrongclass",
   "ornamented",
-  "perk",
-  "perkname",
   "pinnaclereward",
   "postmaster",
   "power",
   "powerfulreward",
-  "powerlimit",
   "primary",
   "pulserifle",
   "purple",
@@ -105,7 +84,6 @@ Array [
   "reptoken",
   "rocketlauncher",
   "scoutrifle",
-  "season",
   "seasonalartifacts",
   "seasonaldupe",
   "shaded",
@@ -115,20 +93,16 @@ Array [
   "smg",
   "sniperrifle",
   "solar",
-  "source",
   "special",
   "specialgrenadelauncher",
   "specialorders",
-  "stack",
   "stackable",
   "stackfull",
   "stasis",
-  "stat",
   "statlower",
   "submachine",
   "sunset",
   "sword",
-  "tag",
   "tagged",
   "titan",
   "tracerifle",
@@ -142,7 +116,41 @@ Array [
   "weapon",
   "weaponmod",
   "white",
-  "year",
   "yellow",
 ]
 `;
+
+exports[`buildSearchConfig generates a reasonable filter map: key-value filters 1`] = `
+Array [
+  "basestat",
+  "breaker",
+  "count",
+  "description",
+  "energycapacity",
+  "hash",
+  "holdsmod",
+  "id",
+  "inloadout",
+  "keyword",
+  "kills",
+  "level",
+  "light",
+  "masterwork",
+  "maxbasestatvalue",
+  "maxstatloadout",
+  "maxstatvalue",
+  "modslot",
+  "name",
+  "notes",
+  "perk",
+  "perkname",
+  "power",
+  "powerlimit",
+  "season",
+  "source",
+  "stack",
+  "stat",
+  "tag",
+  "year",
+]
+`;
diff --git a/src/app/search/search-config.test.ts b/src/app/search/search-config.test.ts
index 3cc1f37fa..56375c773 100644
--- a/src/app/search/search-config.test.ts
+++ b/src/app/search/search-config.test.ts
@@ -1,6 +1,38 @@
+import { canonicalFilterFormats, FilterFormat } from './filter-types';
 import { buildSearchConfig } from './search-config';
 
-test('buildSearchConfig generates a reasonable filter map', () => {
+describe('buildSearchConfig', () => {
   const searchConfig = buildSearchConfig(2);
-  expect(Object.keys(searchConfig.filters).sort()).toMatchSnapshot();
+
+  test('generates a reasonable filter map', () => {
+    expect(Object.keys(searchConfig.isFilters).sort()).toMatchSnapshot('is filters');
+    expect(Object.keys(searchConfig.kvFilters).sort()).toMatchSnapshot('key-value filters');
+  });
+
+  test('filter formats specify unambiguous formats ', () => {
+    /*
+     * We have a bunch of filter formats for which `keyword:value`
+     * with purely alphabetic values can be valid syntax. Filters should
+     * avoid specifying more than one of these.
+     * query and freeform filters are sort of the same thing,
+     * except queries are exhaustive and freeform aren't. Overloaded
+     * stat filters can also accept single words as filter value,
+     * because `season:worthy` is actually `season:10` and we don't
+     * want these to be mistaken for queries or freeforms.
+     */
+
+    const conflictingFilterFormats: FilterFormat[] = ['query', 'freeform', 'rangeoverload'];
+    for (const filter of searchConfig.allFilters) {
+      let formats = canonicalFilterFormats(filter.format);
+
+      if (formats.length < 1) {
+        throw new Error(`filter ${filter.keywords} has no formats`);
+      }
+
+      formats = formats.filter((f) => conflictingFilterFormats.includes(f));
+      if (formats.length > 1) {
+        throw new Error(`filter ${filter.keywords} specifies ambiguous formats ${formats}`);
+      }
+    }
+  });
 });