Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert debug namespace into a module, direct import #51455

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ export default tseslint.config(
"local/jsdoc-format": "error",
"local/js-extensions": "error",
"local/no-array-mutating-method-expressions": "error",
"local/prefer-direct-import": "error",
},
},
{
Expand Down Expand Up @@ -222,6 +223,12 @@ export default tseslint.config(
"local/no-direct-import": "off",
},
},
{
files: ["src/harness/**", "src/testRunner/**", "src/tsserver/**", "src/typingsInstaller/**"],
rules: {
"local/prefer-direct-import": "off",
},
},
{
files: ["src/lib/*.d.ts"],
...tseslint.configs.disableTypeChecked,
Expand Down
3 changes: 3 additions & 0 deletions scripts/eslint/rules/no-direct-import.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ module.exports = createRule({

// These appear in place of public API imports.
if (p.endsWith("../typescript/typescript.js")) return;
// Okay to direct import, for now.
// TODO(jakebailey): we should consider where this is safe to ignore
if (p.endsWith("/debug.js")) return;

// The below is similar to https://github.com/microsoft/DefinitelyTyped-tools/blob/main/packages/eslint-plugin/src/rules/no-bad-reference.ts

Expand Down
124 changes: 124 additions & 0 deletions scripts/eslint/rules/prefer-direct-import.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
const { AST_NODE_TYPES } = require("@typescript-eslint/utils");
const { createRule } = require("./utils.cjs");
const path = require("path");

const srcRoot = path.resolve(__dirname, "../../../src");

const tsNamespaceBarrelRegex = /_namespaces\/ts(?:\.ts|\.js|\.mts|\.mjs|\.cts|\.cjs)?$/;

/**
* @type {Array<{ name: string; path: string; }>}
*/
const modules = [
{
name: "Debug",
path: "compiler/debug",
},
];

module.exports = createRule({
name: "prefer-direct-import",
meta: {
docs: {
description: ``,
},
messages: {
importError: `{{ name }} should be imported directly from {{ path }}`,
},
schema: [],
type: "problem",
fixable: "code",
},
defaultOptions: [],

create(context) {
/**
* @param {string} p
*/
function getImportPath(p) {
let out = path.relative(path.dirname(context.filename), path.join(srcRoot, p)).replace(/\\/g, "/");
if (!out.startsWith(".")) out = "./" + out;
return out;
}

/** @type {any} */
let program;
let addedImport = false;

return {
Program: node => {
program = node;
},
ImportDeclaration: node => {
if (node.importKind !== "value" || !tsNamespaceBarrelRegex.test(node.source.value)) return;

for (const specifier of node.specifiers) {
if (specifier.type !== AST_NODE_TYPES.ImportSpecifier || specifier.importKind !== "value") continue;

for (const mod of modules) {
if (specifier.imported.name !== mod.name) continue;

context.report({
messageId: "importError",
data: { name: mod.name, path: mod.path },
node: specifier,
fix: fixer => {
const newCode = `import * as ${mod.name} from "${getImportPath(mod.path)}";`;
const fixes = [];
if (node.specifiers.length === 1) {
if (addedImport) {
fixes.push(fixer.remove(node));
}
else {
fixes.push(fixer.replaceText(node, newCode));
addedImport = true;
}
}
else {
const comma = context.sourceCode.getTokenAfter(specifier, token => token.value === ",");
if (!comma) throw new Error("comma is null");
const prevNode = context.sourceCode.getTokenBefore(specifier);
if (!prevNode) throw new Error("prevNode is null");
fixes.push(
fixer.removeRange([prevNode.range[1], specifier.range[0]]),
fixer.remove(specifier),
fixer.remove(comma),
);
if (!addedImport) {
fixes.push(fixer.insertTextBefore(node, newCode + "\r\n"));
addedImport = true;
}
}

return fixes;
},
});
}
}
},
MemberExpression: node => {
if (node.object.type !== AST_NODE_TYPES.Identifier || node.object.name !== "ts") return;

for (const mod of modules) {
if (node.property.type !== AST_NODE_TYPES.Identifier || node.property.name !== mod.name) continue;

context.report({
messageId: "importError",
data: { name: mod.name, path: mod.path },
node,
fix: fixer => {
const fixes = [fixer.replaceText(node, mod.name)];

if (!addedImport) {
fixes.push(fixer.insertTextBefore(program, `import * as ${mod.name} from "${getImportPath(mod.path)}";\r\n`));
addedImport = true;
}

return fixes;
},
});
}
},
};
},
});
79 changes: 79 additions & 0 deletions scripts/eslint/tests/prefer-direct-import.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
const { RuleTester } = require("./support/RuleTester.cjs");
const rule = require("../rules/prefer-direct-import.cjs");

const ruleTester = new RuleTester({
languageOptions: {
parserOptions: {
warnOnUnsupportedTypeScriptVersion: false,
},
},
});

ruleTester.run("no-ts-debug", rule, {
valid: [
{
code: `
import * as Debug from "./debug";
`,
},
{
code: `
import type { Debug } from "./_namespaces/ts";
`,
},
{
code: `
import { type Debug } from "./_namespaces/ts";
`,
},
],

invalid: [
{
filename: "src/compiler/checker.ts",
code: `
import { Debug } from "./_namespaces/ts";
`.replace(/\r?\n/g, "\r\n"),
errors: [{ messageId: "importError", data: { name: "Debug", path: "compiler/debug" } }],
output: `
import * as Debug from "./debug";
`.replace(/\r?\n/g, "\r\n"),
},
{
filename: "src/compiler/transformers/ts.ts",
code: `
import { Debug } from "../_namespaces/ts";
`.replace(/\r?\n/g, "\r\n"),
errors: [{ messageId: "importError", data: { name: "Debug", path: "compiler/debug" } }],
output: `
import * as Debug from "../debug";
`.replace(/\r?\n/g, "\r\n"),
},
// TODO(jakebailey): the rule probably needs to handle .js extensions
{
filename: "src/compiler/checker.ts",
code: `
import { Debug } from "./_namespaces/ts.js";
`.replace(/\r?\n/g, "\r\n"),
errors: [{ messageId: "importError", data: { name: "Debug", path: "compiler/debug" } }],
output: `
import * as Debug from "./debug";
`.replace(/\r?\n/g, "\r\n"),
},
{
filename: "src/compiler/checker.ts",
code: `
import * as ts from "./_namespaces/ts";

ts.Debug.assert(true);
`.replace(/\r?\n/g, "\r\n"),
errors: [{ messageId: "importError", data: { name: "Debug", path: "compiler/debug" } }],
output: `
import * as Debug from "./debug";
import * as ts from "./_namespaces/ts";

Debug.assert(true);
`.replace(/\r?\n/g, "\r\n"),
},
],
});
3 changes: 2 additions & 1 deletion src/compiler/_namespaces/ts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

export * from "../corePublic.js";
export * from "../core.js";
export * from "../debug.js";
import * as Debug from "../debug.js";
export { Debug };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that I may actually be able to write:

export { /** @internal */ Debug }

And the comment is preserved and applies, which generally may be a better way to do this in the future.

export * from "../semver.js";
export * from "../performanceCore.js";
export * from "../tracing.js";
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import {
createFileDiagnostic,
createQueue,
createSymbolTable,
Debug,
Declaration,
declarationNameToString,
DeleteExpression,
Expand Down Expand Up @@ -321,6 +320,7 @@ import {
WithStatement,
} from "./_namespaces/ts.js";
import * as performance from "./_namespaces/ts.performance.js";
import * as Debug from "./debug.js";

/** @internal */
export const enum ModuleInstanceState {
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
createModuleNotFoundChain,
createProgram,
CustomTransformers,
Debug,
Diagnostic,
DiagnosticCategory,
DiagnosticMessageChain,
Expand Down Expand Up @@ -90,6 +89,7 @@ import {
WriteFileCallback,
WriteFileCallbackData,
} from "./_namespaces/ts.js";
import * as Debug from "./debug.js";

/** @internal */
export interface ReusableDiagnostic extends ReusableDiagnosticRelatedInformation {
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/builderState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
CompilerOptions,
computeSignatureWithDiagnostics,
CustomTransformers,
Debug,
EmitOnly,
EmitOutput,
emptyArray,
Expand Down Expand Up @@ -34,6 +33,7 @@ import {
toPath,
TypeChecker,
} from "./_namespaces/ts.js";
import * as Debug from "./debug.js";

/** @internal */
export function getFileEmitOutput(
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ import {
createSymbolTable,
createSyntacticTypeNodeBuilder,
createTextWriter,
Debug,
Declaration,
DeclarationName,
declarationNameToString,
Expand Down Expand Up @@ -1124,6 +1123,7 @@ import {
} from "./_namespaces/ts.js";
import * as moduleSpecifiers from "./_namespaces/ts.moduleSpecifiers.js";
import * as performance from "./_namespaces/ts.performance.js";
import * as Debug from "./debug.js";

const ambientModuleSymbolRegex = /^".+"$/;
const anon = "(anonymous)" as __String & string;
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {
createCompilerDiagnostic,
createDiagnosticForNodeInSourceFile,
createGetCanonicalFileName,
Debug,
Diagnostic,
DiagnosticArguments,
DiagnosticMessage,
Expand Down Expand Up @@ -122,6 +121,7 @@ import {
WatchFileKind,
WatchOptions,
} from "./_namespaces/ts.js";
import * as Debug from "./debug.js";

const compileOnSaveCommandLineOption: CommandLineOption = {
name: "compileOnSave",
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import {
CharacterCodes,
Comparer,
Comparison,
Debug,
EqualityComparer,
MapLike,
Queue,
SortedArray,
SortedReadonlyArray,
TextSpan,
} from "./_namespaces/ts.js";
import * as Debug from "./debug.js";

/* eslint-disable @typescript-eslint/prefer-for-of */

Expand Down Expand Up @@ -1917,7 +1917,7 @@ export function memoizeOne<A extends string | number | boolean | undefined, T>(c
};
}

/** @internal */
/** @internal @knipignore */
export const enum AssertionLevel {
None = 0,
Normal = 1,
Expand Down
Loading