Add blocklist/allowlist for diagnostics

Summary: Refactor the diagnostics allowlist a bit, and support both allowlist and blocklist. This lets us implement wildcard via an empty blocklist, which is simpler and more general.

Reviewed By: madgen

Differential Revision: D65459587

fbshipit-source-id: a9ef8be95c9ac191df6a1aefa8fc2d8b99621afd

addons/isl/src/Diagnostics.tsx

@@ -6,7 +6,8 @@
  */
 
 import type {UseUncommittedSelection} from './partialSelection';
-import type {CommitInfo, Diagnostic} from './types';
+import type {CommitInfo, Diagnostic, DiagnosticAllowlist} from './types';
+import type {ExclusiveOr} from 'isl-components/Types';
 import type {Tracker} from 'isl-server/src/analytics/tracker';
 
 import {spacing} from '../../components/theme/tokens.stylex';
@@ -65,11 +66,13 @@ const styles = stylex.create({
   },
 });
 
-/** Many diagnostics are low-quality and don't reflect what would appear on CI.
- * Start with an allowlist while we validate which signals are worthwhile. */
-const allowlistedCodesBySource = Internal.allowlistedDiagnosticCodes ?? undefined;
-
-function isBlockingDiagnostic(d: Diagnostic): boolean {
+export function isBlockingDiagnostic(
+  d: Diagnostic,
+  /** Many diagnostics are low-quality and don't reflect what would appear on CI.
+   * Start with an allowlist while we validate which signals are worthwhile. */
+  allowlistedCodesBySource: undefined | DiagnosticAllowlist = Internal.allowlistedDiagnosticCodes ??
+    undefined,
+): boolean {
   if (d.source == null || d.code == null) {
     return true;
   }
@@ -86,7 +89,9 @@ function isBlockingDiagnostic(d: Diagnostic): boolean {
   const relevantAllowlist = allowlistedCodesBySource.get(d.severity)?.get(d.source);
   return (
     relevantAllowlist != null &&
-    (relevantAllowlist.has(d.code) === true || relevantAllowlist.has('*') === true)
+    (relevantAllowlist.allow
+      ? relevantAllowlist.allow.has(d.code) === true
+      : relevantAllowlist.block.has(d.code) === false)
   );
 }
 
@@ -139,7 +144,7 @@ export async function confirmNoBlockingDiagnostics(
     if (result.diagnostics.size > 0) {
       const allDiagnostics = [...result.diagnostics.values()];
       const allBlockingErrors = allDiagnostics
-        .map(value => value.filter(isBlockingDiagnostic))
+        .map(value => value.filter(d => isBlockingDiagnostic(d)))
         .flat();
       const totalErrors = allBlockingErrors.length;
 

addons/isl/src/__tests__/Diagnostics.test.tsx

@@ -0,0 +1,130 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+import type {DiagnosticAllowlist} from '../types';
+
+import {isBlockingDiagnostic} from '../Diagnostics';
+
+const range = {startLine: 1, startCol: 1, endLine: 1, endCol: 1};
+
+describe('Diagnostics', () => {
+  describe('isBlockingDiagnostic', () => {
+    it('handles allowlist', () => {
+      const policy: DiagnosticAllowlist = new Map([
+        ['error', new Map([['foo', {allow: new Set(['code1'])}]])],
+      ]);
+      expect(
+        isBlockingDiagnostic(
+          {
+            message: 'sample',
+            range,
+            severity: 'error',
+            source: 'foo',
+            code: 'code1',
+          },
+          policy,
+        ),
+      ).toBe(true);
+      expect(
+        isBlockingDiagnostic(
+          {
+            message: 'sample',
+            range,
+            severity: 'error',
+            source: 'foo',
+            code: 'code3',
+          },
+          policy,
+        ),
+      ).toBe(false);
+    });
+
+    it('handles blocklist', () => {
+      const policy: DiagnosticAllowlist = new Map([
+        ['error', new Map([['foo', {block: new Set(['code1'])}]])],
+      ]);
+      expect(
+        isBlockingDiagnostic(
+          {
+            message: 'sample',
+            range,
+            severity: 'error',
+            source: 'foo',
+            code: 'code3',
+          },
+          policy,
+        ),
+      ).toBe(true);
+      expect(
+        isBlockingDiagnostic(
+          {
+            message: 'sample',
+            range,
+            severity: 'error',
+            source: 'foo',
+            code: 'code1',
+          },
+          policy,
+        ),
+      ).toBe(false);
+    });
+
+    it('handles wildcard', () => {
+      const policy: DiagnosticAllowlist = new Map([
+        ['error', new Map([['foo', {block: new Set([])}]])],
+      ]);
+      expect(
+        isBlockingDiagnostic(
+          {
+            message: 'sample',
+            range,
+            severity: 'error',
+            source: 'foo',
+            code: 'code1',
+          },
+          policy,
+        ),
+      ).toBe(true);
+    });
+
+    it('ignores non-errors', () => {
+      const policy: DiagnosticAllowlist = new Map([
+        ['error', new Map([['foo', {block: new Set([])}]])],
+      ]);
+      expect(
+        isBlockingDiagnostic(
+          {
+            message: 'sample',
+            range,
+            severity: 'hint',
+            source: 'foo',
+            code: 'code1',
+          },
+          policy,
+        ),
+      ).toBe(false);
+    });
+
+    it('accepts warnings', () => {
+      const policy: DiagnosticAllowlist = new Map([
+        ['warning', new Map([['foo', {block: new Set([])}]])],
+      ]);
+      expect(
+        isBlockingDiagnostic(
+          {
+            message: 'sample',
+            range,
+            severity: 'warning',
+            source: 'foo',
+            code: 'code1',
+          },
+          policy,
+        ),
+      ).toBe(true);
+    });
+  });
+});

addons/isl/src/types.ts

@@ -593,6 +593,11 @@ export type Diagnostic = {
   code?: string;
 };
 
+export type DiagnosticAllowlistValue =
+  | {block: Set<string>; allow?: undefined}
+  | {allow: Set<string>; block?: undefined};
+export type DiagnosticAllowlist = Map<'warning' | 'error', Map<string, DiagnosticAllowlistValue>>;
+
 /* protocol */
 
 /**