refactor(linter/plugins): enable TS strictNullChecks (#15886)

Adapted from #15868. Enable `strictNullChecks` in TSConfig for `apps/oxlint`.

Rather than using `!` all over the place, I've added a function `assertIsNonNull`, which performs the same function. Idea is that we can later on produce a debug build where `assertIsNonNull` does an actual runtime assertion (rather than just a TS `asserts`) that the value is not `null` or `undefined`. We can use the debug build in tests, and catch any logic errors.

Unfortunately, TypeScript doesn't seem able to track side effects, so there seems to be no way to tell it "after calling `initAst()`, `ast` is always non-null". Don't want to switch to a having a `getAst` function which returns a non-null value, as that'd introduce unnecessary temp vars, and function calls, on hot paths. So this seems to be the best we can do without incurring runtime cost.

Author: overlookmotel

apps/oxlint/src-js/cli.ts

@@ -24,7 +24,7 @@ function loadPluginWrapper(path: string, packageName: string | null): Promise<st
       return loadPlugin(path, packageName);
     });
   }
-  return loadPlugin(path, packageName);
+  return loadPlugin!(path, packageName);
 }
 
 /**

apps/oxlint/src-js/index.ts

@@ -1,8 +1,11 @@
+import { assertIsNonNull } from './plugins/utils.js';
+
 import type { Context, FileContext, LanguageOptions } from './plugins/context.ts';
 import type { CreateOnceRule, Plugin, Rule } from './plugins/load.ts';
 import type { Settings } from './plugins/settings.ts';
 import type { SourceCode } from './plugins/source_code.ts';
 import type { BeforeHook, Visitor, VisitorWithHooks } from './plugins/types.ts';
+import type { SetNullable } from './plugins/utils.ts';
 
 export type * as ESTree from './generated/types.d.ts';
 export type { Context, LanguageOptions } from './plugins/context.ts';
@@ -114,6 +117,7 @@ export function defineRule(rule: Rule): Rule {
     if (context === null) {
       ({ context, visitor, beforeHook } = createContextAndVisitor(rule));
     }
+    assertIsNonNull(visitor);
 
     // Copy properties from ESLint's context object to `context`.
     // ESLint's context object is an object of form `{ id, options, report }`, with all other properties
@@ -131,7 +135,7 @@ export function defineRule(rule: Rule): Rule {
     }
 
     // Return same visitor each time
-    return visitor!;
+    return visitor;
   };
 
   return rule;
@@ -231,9 +235,13 @@ function createContextAndVisitor(rule: CreateOnceRule): {
     report: { value: null, enumerable: true, configurable: true },
   });
 
-  let { before: beforeHook, after: afterHook, ...visitor } = createOnce.call(rule, context) as VisitorWithHooks;
+  let {
+    before: beforeHook,
+    after: afterHook,
+    ...visitor
+  } = createOnce.call(rule, context) as SetNullable<VisitorWithHooks, 'before' | 'after'>;
 
-  if (beforeHook === void 0) {
+  if (beforeHook === undefined) {
     beforeHook = null;
   } else if (beforeHook !== null && typeof beforeHook !== 'function') {
     throw new Error('`before` property of visitor must be a function if defined');

apps/oxlint/src-js/plugins/comments.ts

@@ -3,6 +3,7 @@
  */
 
 import { ast, initAst, sourceText } from './source_code.js';
+import { assertIsNonNull } from './utils.js';
 
 import type { Comment, Node, NodeOrToken } from './types.ts';
 
@@ -15,6 +16,8 @@ const WHITESPACE_ONLY_REGEXP = /^\s*$/;
  */
 export function getAllComments(): Comment[] {
   if (ast === null) initAst();
+  assertIsNonNull(ast);
+
   // `comments` property is a getter. Comments are deserialized lazily.
   return ast.comments;
 }
@@ -38,6 +41,8 @@ export function getAllComments(): Comment[] {
  */
 export function getCommentsBefore(nodeOrToken: NodeOrToken): Comment[] {
   if (ast === null) initAst();
+  assertIsNonNull(ast);
+  assertIsNonNull(sourceText);
 
   const { comments } = ast,
     commentsLength = comments.length;
@@ -93,6 +98,8 @@ export function getCommentsBefore(nodeOrToken: NodeOrToken): Comment[] {
  */
 export function getCommentsAfter(nodeOrToken: NodeOrToken): Comment[] {
   if (ast === null) initAst();
+  assertIsNonNull(ast);
+  assertIsNonNull(sourceText);
 
   const { comments } = ast,
     commentsLength = comments.length;
@@ -135,6 +142,7 @@ export function getCommentsAfter(nodeOrToken: NodeOrToken): Comment[] {
  */
 export function getCommentsInside(node: Node): Comment[] {
   if (ast === null) initAst();
+  assertIsNonNull(ast);
 
   const { comments } = ast,
     commentsLength = comments.length;
@@ -178,6 +186,7 @@ export function getCommentsInside(node: Node): Comment[] {
  */
 export function commentsExistBetween(nodeOrToken1: NodeOrToken, nodeOrToken2: NodeOrToken): boolean {
   if (ast === null) initAst();
+  assertIsNonNull(ast);
 
   // Find the first comment after `nodeOrToken1` ends.
   const { comments } = ast,

apps/oxlint/src-js/plugins/context.ts

@@ -29,6 +29,7 @@
 import { ast, initAst, SOURCE_CODE } from './source_code.js';
 import { report } from './report.js';
 import { settings, initSettings } from './settings.js';
+import { assertIsNonNull } from './utils.js';
 
 import type { Options, RuleDetails } from './load.ts';
 import type { Diagnostic } from './report.ts';
@@ -78,6 +79,8 @@ const PARSER_OPTIONS = freeze({
     // in case it's used in `create` to return an empty visitor if wrong type.
     // TODO: ESLint also has `commonjs` option.
     if (ast === null) initAst();
+    assertIsNonNull(ast);
+
     return ast.sourceType;
   },
 });
@@ -92,6 +95,8 @@ const LANGUAGE_OPTIONS = freeze({
     // in case it's used in `create` to return an empty visitor if wrong type.
     // TODO: ESLint also has `commonjs` option.
     if (ast === null) initAst();
+    assertIsNonNull(ast);
+
     return ast.sourceType;
   },
 
@@ -250,7 +255,10 @@ const FILE_CONTEXT = freeze({
    */
   get settings(): Readonly<Settings> {
     if (filePath === null) throw new Error('Cannot access `context.settings` in `createOnce`');
+
     if (settings === null) initSettings();
+    assertIsNonNull(settings);
+
     return settings;
   },
 

apps/oxlint/src-js/plugins/lint.ts

@@ -3,7 +3,7 @@ import { registeredRules } from './load.js';
 import { diagnostics } from './report.js';
 import { setSettingsForFile, resetSettings } from './settings.js';
 import { ast, initAst, resetSourceAndAst, setupSourceForFile } from './source_code.js';
-import { assertIs, getErrorMessage } from './utils.js';
+import { assertIs, assertIsNonNull, getErrorMessage } from './utils.js';
 import { addVisitorToCompiled, compiledVisitor, finalizeCompiledVisitor, initCompiledVisitor } from './visitor.js';
 
 // Lazy implementation
@@ -136,6 +136,7 @@ function lintFileImpl(
     let { visitor } = ruleDetails;
     if (visitor === null) {
       // Rule defined with `create` method
+      assertIsNonNull(ruleDetails.rule.create);
       visitor = ruleDetails.rule.create(ruleDetails.context);
     } else {
       // Rule defined with `createOnce` method

apps/oxlint/src-js/plugins/load.ts

@@ -8,6 +8,7 @@ import type { Context } from './context.ts';
 import type { JsonValue } from './json.ts';
 import type { RuleMeta } from './rule_meta.ts';
 import type { AfterHook, BeforeHook, Visitor, VisitorWithHooks } from './types.ts';
+import type { SetNullable } from './utils.ts';
 
 const ObjectKeys = Object.keys;
 
@@ -181,7 +182,7 @@ async function loadPluginImpl(path: string, packageName: string | null): Promise
     // Create `RuleDetails` object for rule.
     const ruleDetails: RuleDetails = {
       rule: rule as CreateRule, // Could also be `CreateOnceRule`, but just to satisfy type checker
-      context: null as Readonly<Context>, // Filled in below
+      context: null!, // Filled in below
       isFixable,
       messages,
       ruleIndex: 0,
@@ -197,7 +198,7 @@ async function loadPluginImpl(path: string, packageName: string | null): Promise
 
     if ('createOnce' in rule) {
       // TODO: Compile visitor object to array here, instead of repeating compilation on each file
-      let visitorWithHooks = rule.createOnce(context);
+      let visitorWithHooks = rule.createOnce(context) as SetNullable<VisitorWithHooks, 'before' | 'after'>;
       if (typeof visitorWithHooks !== 'object' || visitorWithHooks === null) {
         throw new TypeError('`createOnce` must return an object');
       }
@@ -219,9 +220,9 @@ async function loadPluginImpl(path: string, packageName: string | null): Promise
         afterHook = null;
       }
 
-      (ruleDetails as Writable<CreateOnceRuleDetails>).visitor = visitor;
-      (ruleDetails as Writable<CreateOnceRuleDetails>).beforeHook = beforeHook;
-      (ruleDetails as Writable<CreateOnceRuleDetails>).afterHook = afterHook;
+      (ruleDetails as unknown as Writable<CreateOnceRuleDetails>).visitor = visitor;
+      (ruleDetails as unknown as Writable<CreateOnceRuleDetails>).beforeHook = beforeHook;
+      (ruleDetails as unknown as Writable<CreateOnceRuleDetails>).afterHook = afterHook;
     }
 
     registeredRules.push(ruleDetails);

apps/oxlint/src-js/plugins/location.ts

@@ -4,6 +4,7 @@
  */
 
 import { initSourceText, sourceText } from './source_code.js';
+import { assertIsNonNull } from './utils.js';
 
 import type { Node } from './types.ts';
 
@@ -60,6 +61,7 @@ const lineStartOffsets: number[] = [0];
  */
 export function initLines(): void {
   if (sourceText === null) initSourceText();
+  assertIsNonNull(sourceText);
 
   // This implementation is based on the one in ESLint.
   // TODO: Investigate if using `String.prototype.matchAll` is faster.
@@ -109,6 +111,7 @@ export function getLineColumnFromOffset(offset: number): LineColumn {
   // Build `lines` and `lineStartOffsets` tables if they haven't been already.
   // This also decodes `sourceText` if it wasn't already.
   if (lines.length === 0) initLines();
+  assertIsNonNull(sourceText);
 
   if (offset > sourceText.length) {
     throw new RangeError(
@@ -158,6 +161,7 @@ export function getOffsetFromLineColumn(loc: LineColumn): number {
       // Build `lines` and `lineStartOffsets` tables if they haven't been already.
       // This also decodes `sourceText` if it wasn't already.
       if (lines.length === 0) initLines();
+      assertIsNonNull(sourceText);
 
       const linesCount = lineStartOffsets.length;
       if (line <= 0 || line > linesCount) {

apps/oxlint/src-js/plugins/report.ts

@@ -84,7 +84,7 @@ export function report(diagnostic: Diagnostic, ruleDetails: RuleDetails): void {
   }
 
   // TODO: Validate `diagnostic`
-  let start: number, end: number, loc: Location;
+  let start: number, end: number, loc: Location | undefined;
 
   if (hasOwn(diagnostic, 'loc') && (loc = diagnostic.loc) != null) {
     // `loc`

apps/oxlint/src-js/plugins/scope.ts

@@ -8,7 +8,7 @@ import {
   type ScopeManager as TSESLintScopeManager,
 } from '@typescript-eslint/scope-manager';
 import { ast, initAst } from './source_code.js';
-import { assertIs } from './utils.js';
+import { assertIs, assertIsNonNull } from './utils.js';
 
 import type * as ESTree from '../generated/types.d.ts';
 import type { SetNullable } from './utils.ts';
@@ -109,6 +109,7 @@ const analyzeOptions: SetNullable<AnalyzeOptions, 'sourceType'> = {
  */
 function initTsScopeManager() {
   if (ast === null) initAst();
+  assertIsNonNull(ast);
 
   analyzeOptions.sourceType = ast.sourceType;
   assertIs<AnalyzeOptions>(analyzeOptions);
@@ -199,6 +200,7 @@ export function isGlobalReference(node: ESTree.Node): boolean {
   if (node.type !== 'Identifier') return false;
 
   if (tsScopeManager === null) initTsScopeManager();
+  assertIsNonNull(tsScopeManager);
 
   const { scopes } = tsScopeManager;
   if (scopes.length === 0) return false;
@@ -229,6 +231,8 @@ export function isGlobalReference(node: ESTree.Node): boolean {
 export function getDeclaredVariables(node: ESTree.Node): Variable[] {
   // ref: https://github.com/eslint/eslint/blob/e7cda3bdf1bdd664e6033503a3315ad81736b200/lib/languages/js/source-code/source-code.js#L904
   if (tsScopeManager === null) initTsScopeManager();
+  assertIsNonNull(tsScopeManager);
+
   // @ts-expect-error // TODO: Our types don't quite align yet
   return tsScopeManager.getDeclaredVariables(node);
 }
@@ -243,6 +247,7 @@ export function getScope(node: ESTree.Node): Scope {
   if (!node) throw new TypeError('Missing required argument: `node`');
 
   if (tsScopeManager === null) initTsScopeManager();
+  assertIsNonNull(tsScopeManager);
 
   const inner = node.type !== 'Program';
 
@@ -254,6 +259,7 @@ export function getScope(node: ESTree.Node): Scope {
       return scope.type === 'function-expression-name' ? scope.childScopes[0] : scope;
     }
 
+    // @ts-expect-error - Don't want to create a new variable just to make it nullable
     node = node.parent;
   } while (node !== null);
 

apps/oxlint/src-js/plugins/selector.ts

@@ -54,7 +54,7 @@ const EMPTY_TYPE_IDS_ARRAY: NodeTypeId[] = [];
 export function parseSelector(key: string): Selector {
   // Used cached object if we've parsed this key before
   let selector = cache.get(key);
-  if (selector !== void 0) return selector;
+  if (selector !== undefined) return selector;
 
   // Parse with `esquery` and analyse
   const esquerySelector = esqueryParse(key);
@@ -97,7 +97,7 @@ function analyzeSelector(esquerySelector: EsquerySelector, selector: Selector):
       // If the type is invalid, just treat this selector as not matching any types.
       // But still increment `identifierCount`.
       // This matches ESLint's behavior.
-      return typeId === void 0 ? EMPTY_TYPE_IDS_ARRAY : [typeId];
+      return typeId === undefined ? EMPTY_TYPE_IDS_ARRAY : [typeId];
     }
 
     case 'not':
@@ -144,7 +144,7 @@ function analyzeSelector(esquerySelector: EsquerySelector, selector: Selector):
         } else {
           // Selector only matches intersection of all child selectors.
           // TODO: Could make this faster if `analyzeSelector` always returned an ordered array.
-          nodeTypes = childNodeTypes.filter((nodeType) => nodeTypes.includes(nodeType));
+          nodeTypes = childNodeTypes.filter((nodeType) => nodeTypes!.includes(nodeType));
         }
       }
       return nodeTypes;