From 9952c5b6b72ac6132c098fdf58cf4f2ae817ecc2 Mon Sep 17 00:00:00 2001 From: Evan Krause Date: Tue, 5 Nov 2024 10:05:47 -0800 Subject: [PATCH] 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 | 21 +-- addons/isl/src/__tests__/Diagnostics.test.tsx | 130 ++++++++++++++++++ addons/isl/src/types.ts | 5 + 3 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 addons/isl/src/__tests__/Diagnostics.test.tsx diff --git a/addons/isl/src/Diagnostics.tsx b/addons/isl/src/Diagnostics.tsx index cd6e7c10be067..1f08ad07aa350 100644 --- a/addons/isl/src/Diagnostics.tsx +++ b/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; diff --git a/addons/isl/src/__tests__/Diagnostics.test.tsx b/addons/isl/src/__tests__/Diagnostics.test.tsx new file mode 100644 index 0000000000000..60bddcdc8b341 --- /dev/null +++ b/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); + }); + }); +}); diff --git a/addons/isl/src/types.ts b/addons/isl/src/types.ts index 7fb1772e00a05..d411b234e89f2 100644 --- a/addons/isl/src/types.ts +++ b/addons/isl/src/types.ts @@ -593,6 +593,11 @@ export type Diagnostic = { code?: string; }; +export type DiagnosticAllowlistValue = + | {block: Set; allow?: undefined} + | {allow: Set; block?: undefined}; +export type DiagnosticAllowlist = Map<'warning' | 'error', Map>; + /* protocol */ /**