Optimize sorting of diagnostics, avoid invalidating dependents unless analyze shape has changed
MODIFY
packages/@romejs/core/master/dependencies/DependencyGraph.ts
packages/@romejs/core/master/dependencies/DependencyGraph.ts
@@ -204,9 +204,14 @@ export default class DependencyGraph {
}
}
- validate(node: DependencyNode, diagnosticsProcessor: DiagnosticsProcessor) {
+ validate(
+ node: DependencyNode,
+ diagnosticsProcessor: DiagnosticsProcessor,
+ ): boolean {
const resolvedImports = node.resolveImports();
- diagnosticsProcessor.addDiagnostics(resolvedImports.diagnostics);
+ return (
+ diagnosticsProcessor.addDiagnostics(resolvedImports.diagnostics).length > 0
+ );
}
validateTransitive(

MODIFY
packages/@romejs/core/master/fs/MemoryFileSystem.ts
packages/@romejs/core/master/fs/MemoryFileSystem.ts
@@ -598,6 +598,23 @@ export default class MemoryFileSystem {
return changed;
}
+ async waitIfInitializingWatch(projectFolderPath: AbsoluteFilePath) {
+ // Defer if we're initializing a parent folder
+ for (const {promise, path} of this.watchPromises.values()) {
+ if (projectFolderPath.isRelativeTo(path)) {
+ await promise;
+ return undefined;
+ }
+ }
+
+ // Wait if we're initializing descendents
+ for (const {path, promise} of this.watchPromises.values()) {
+ if (path.isRelativeTo(projectFolderPath)) {
+ await promise;
+ }
+ }
+ }
+
async watch(
projectFolderPath: AbsoluteFilePath,
projectConfig: ProjectConfig,
@@ -627,20 +644,8 @@ export default class MemoryFileSystem {
}
}
- // Defer if we're initializing a parent folder
- for (const {promise, path} of this.watchPromises.values()) {
- if (projectFolderPath.isRelativeTo(path)) {
- await promise;
- return undefined;
- }
- }
-
- // Wait if we're initializing descendents
- for (const {path, promise} of this.watchPromises.values()) {
- if (path.isRelativeTo(projectFolderPath)) {
- await promise;
- }
- }
+ // Wait for other initializations
+ await this.waitIfInitializingWatch(projectFolderPath);
// New watch target
logger.info(`[MemoryFileSystem] Adding new project folder ${projectFolder}`);

MODIFY
packages/@romejs/core/master/linter/Linter.ts
packages/@romejs/core/master/linter/Linter.ts
@@ -21,6 +21,8 @@ import {AbsoluteFilePathSet, AbsoluteFilePathMap} from '@romejs/path';
import {DiagnosticsPrinter} from '@romejs/cli-diagnostics';
import DependencyGraph from '../dependencies/DependencyGraph';
import {ReporterProgressOptions, ReporterProgress} from '@romejs/cli-reporter';
+import DependencyNode from '../dependencies/DependencyNode';
+import {areAnalyzeDependencyResultsEqual} from '@romejs/js-compiler';
type LintWatchChanges = Array<{
filename: undefined | string;
@@ -126,8 +128,11 @@ class LintRunner {
this.fix = fix;
this.events = events;
this.compilerDiagnosticsCache = new AbsoluteFilePathMap();
+ this.hadDependencyValidationErrors = new AbsoluteFilePathMap();
}
+ hadDependencyValidationErrors: AbsoluteFilePathMap<boolean>;
+
compilerDiagnosticsCache: AbsoluteFilePathMap<{
diagnostics: Diagnostics;
suppressions: DiagnosticSuppressions;
@@ -189,68 +194,111 @@ class LintRunner {
): Promise<AbsoluteFilePathSet> {
const {graph} = this;
- const validateDependencyPaths: AbsoluteFilePathSet =
- new AbsoluteFilePathSet();
-
+ // Get all the current dependency nodes for the evicted files, and invalidate their nodes
+ const oldEvictedNodes: AbsoluteFilePathMap<DependencyNode> =
+ new AbsoluteFilePathMap();
for (const path of evictedPaths) {
- validateDependencyPaths.add(path);
-
- // Will be undefined if this is the first time initializing it in the graph
const node = graph.maybeGetNode(path);
if (node !== undefined) {
- for (const depNode of node.getDependents()) {
- validateDependencyPaths.add(depNode.path);
+ oldEvictedNodes.set(path, node);
+ graph.deleteNode(path);
}
}
+
+ // Refresh only the evicted paths
+ const progress = this.events.createProgress({
+ title: 'Analyzing changed files',
+ });
+ await graph.seed({
+ paths: Array.from(evictedPaths),
+ diagnosticsProcessor: processor,
+ validate: false,
+ analyzeProgress: progress,
+ });
+
+ // Maintain a list of all the dependencies we revalidated
+ const validatedDependencyPaths: AbsoluteFilePathSet =
+ new AbsoluteFilePathSet();
+
+ // Maintain a list of all the dependents that need to be revalidated
+ const validatedDependencyPathDependents: AbsoluteFilePathSet =
+ new AbsoluteFilePathSet();
+
+ // Build a list of dependents to recheck
+ for (const path of evictedPaths) {
+ validatedDependencyPaths.add(path);
+
+ const newNode = graph.getNode(path);
+
+ // Get the previous node and see if the exports have actually changed
+ const oldNode = oldEvictedNodes.get(path);
+ const sameShape = oldNode !== undefined &&
+ areAnalyzeDependencyResultsEqual(oldNode.analyze, newNode.analyze);
+
+ for (const depNode of newNode.getDependents()) {
+ // If the old node has the same shape as the new one, only revalidate the dependent if it had dependency errors
+ if (sameShape &&
+ this.hadDependencyValidationErrors.get(depNode.path) === false) {
+ continue;
}
- for (const path of validateDependencyPaths) {
- graph.deleteNode(path);
+ validatedDependencyPaths.add(depNode.path);
+ validatedDependencyPathDependents.add(depNode.path);
+ }
}
- const progress = this.events.createProgress({title: 'Analyzing'});
+ // Revalidate dependents
+ if (validatedDependencyPathDependents.size > 0) {
+ const progress = this.events.createProgress({
+ title: 'Analyzing dependents',
+ });
await graph.seed({
- paths: Array.from(validateDependencyPaths),
+ paths: Array.from(validatedDependencyPaths),
diagnosticsProcessor: processor,
validate: false,
analyzeProgress: progress,
});
+ }
- for (const path of validateDependencyPaths) {
- graph.validate(graph.getNode(path), processor);
+ // Validate connections
+ for (const path of validatedDependencyPaths) {
+ const hasValidationErrors = graph.validate(graph.getNode(path), processor);
+ this.hadDependencyValidationErrors.set(path, hasValidationErrors);
}
- return validateDependencyPaths;
+ return validatedDependencyPaths;
}
computeChanges(
- {evictedPaths: changedPaths, processor}: LintRunOptions,
- validateDependencyPaths: AbsoluteFilePathSet,
+ {evictedPaths, processor}: LintRunOptions,
+ validatedDependencyPaths: AbsoluteFilePathSet,
): LintWatchChanges {
const {master} = this;
const changes: LintWatchChanges = [];
const updatedPaths: AbsoluteFilePathSet = new AbsoluteFilePathSet([
- ...validateDependencyPaths,
+ ...validatedDependencyPaths,
]);
+ const diagnosticsByFilename = processor.getDiagnosticsByFilename();
+
// In case we pushed on any diagnostics that aren't from the input paths, try to resolve them
const includedFilenamesInDiagnostics =
master.projectManager.normalizeFilenamesToFilePaths(
- processor.getDiagnosticFilenames(),
+ diagnosticsByFilename.keys(),
);
for (const path of includedFilenamesInDiagnostics.absolutes) {
updatedPaths.add(path);
}
// If we validated the diagnostics of the dependents, then we need to also push their previous compiler diagnostics
- for (const path of validateDependencyPaths) {
- if (!changedPaths.has(path)) {
+ for (const path of validatedDependencyPaths) {
+ if (!evictedPaths.has(path)) {
const compilerDiagnostics = this.compilerDiagnosticsCache.get(path);
if (compilerDiagnostics !== undefined) {
- processor.addDiagnostics(compilerDiagnostics.diagnostics);
processor.addSuppressions(compilerDiagnostics.suppressions);
+ processor.addDiagnostics(compilerDiagnostics.diagnostics);
}
}
}
@@ -258,7 +306,10 @@ class LintRunner {
// We can't just use getDiagnosticFilenames as we need to produce empty arrays for removed diagnostics
for (const path of updatedPaths) {
const ref = this.request.master.projectManager.getFileReference(path);
- const diagnostics = processor.getDiagnosticsForFile(ref.uid);
+ const diagnostics: Diagnostics = [
+ ...(diagnosticsByFilename.get(ref.uid) || []),
+ ...(diagnosticsByFilename.get(ref.real.join()) || []),
+ ];
changes.push({
filename: ref.uid,
@@ -276,7 +327,7 @@ class LintRunner {
changes.push({
filename,
ref: undefined,
- diagnostics: processor.getDiagnosticsForFile(filename),
+ diagnostics: diagnosticsByFilename.get(filename) || [],
});
}
@@ -286,13 +337,13 @@ class LintRunner {
async run(opts: LintRunOptions): Promise<WatchResults> {
this.events.onRunStart();
const {fixedCount} = await this.runLint(opts);
- const validateDependencyPaths = await this.runGraph(opts);
- const changes = await this.computeChanges(opts, validateDependencyPaths);
+ const validatedDependencyPaths = await this.runGraph(opts);
+ const changes = await this.computeChanges(opts, validatedDependencyPaths);
return {
evictedPaths: opts.evictedPaths,
changes,
fixedCount,
- totalCount: validateDependencyPaths.size,
+ totalCount: validatedDependencyPaths.size,
runner: this,
};
}

MODIFY
packages/@romejs/diagnostics/DiagnosticsProcessor.ts
packages/@romejs/diagnostics/DiagnosticsProcessor.ts
@@ -16,7 +16,7 @@ import {
import {addOriginsToDiagnostics} from './derive';
import {naturalCompare} from '@romejs/string-utils';
import {DiagnosticsError} from './errors';
-import {add} from '@romejs/ob1';
+import {add, get0} from '@romejs/ob1';
import {DiagnosticCategoryPrefix} from './categories';
import {descriptions} from './descriptions';
@@ -43,19 +43,22 @@ const DEFAULT_UNIQUE: UniqueRules = [
['category', 'filename', 'message', 'start.line', 'start.column'],
];
+type DiagnosticsByFilename = Map<undefined | string, Diagnostics>;
+
export default class DiagnosticsProcessor {
constructor(options: CollectorOptions = {}) {
- this.diagnostics = [];
this.filters = [];
- this.allowedUnusedSuppressionPrefixes = new Set();
- this.usedSuppressions = new Set();
- this.suppressions = new Set();
this.options = options;
this.includedKeys = new Set();
this.unique = options.unique === undefined ? DEFAULT_UNIQUE : options.unique;
this.throwAfter = undefined;
- this.cachedDiagnostics = undefined;
this.origins = options.origins === undefined ? [] : [...options.origins];
+ this.allowedUnusedSuppressionPrefixes = new Set();
+ this.usedSuppressions = new Set();
+ this.suppressions = new Set();
+
+ this.diagnostics = [];
+ this.cachedDiagnostics = undefined;
}
unique: UniqueRules;
@@ -67,9 +70,10 @@ export default class DiagnosticsProcessor {
suppressions: Set<DiagnosticSuppression>;
options: CollectorOptions;
throwAfter: undefined | number;
- cachedDiagnostics: undefined | Diagnostics;
origins: Array<DiagnosticOrigin>;
+ cachedDiagnostics: undefined | Diagnostics;
+
static createImmediateThrower(
origins: Array<DiagnosticOrigin>,
): DiagnosticsProcessor {
@@ -272,14 +276,21 @@ export default class DiagnosticsProcessor {
return added;
}
- getDiagnosticFilenames(): Set<undefined | string> {
- return new Set(this.getDiagnostics().map((diag) => diag.location.filename));
+ getDiagnosticsByFilename(): DiagnosticsByFilename {
+ const byFilename: DiagnosticsByFilename = new Map();
+
+ for (const diag of this.getDiagnostics()) {
+ const {filename} = diag.location;
+
+ let filenameDiagnostics = byFilename.get(filename);
+ if (filenameDiagnostics === undefined) {
+ filenameDiagnostics = [];
+ byFilename.set(filename, filenameDiagnostics);
+ }
+ filenameDiagnostics.push(diag);
}
- getDiagnosticsForFile(filename: undefined | string): Diagnostics {
- return this.getDiagnostics().filter((diag) =>
- diag.location.filename === filename
- );
+ return byFilename;
}
getDiagnostics(): Diagnostics {
@@ -313,20 +324,41 @@ export default class DiagnosticsProcessor {
}
getSortedDiagnostics(): Diagnostics {
- // Sort files by filename to ensure they're always in the same order
+ const diagnosticsByFilename = this.getDiagnosticsByFilename();
- // TODO also sort by line/column
- return this.getDiagnostics().sort((a, b) => {
- if (a.location.filename === undefined || b.location.filename === undefined) {
+ // Get all filenames and sort them
+ const filenames: Array<undefined | string> = Array.from(
+ diagnosticsByFilename.keys(),
+ ).sort((a, b) => {
+ if (a === undefined || b === undefined) {
return 0;
} else {
- return naturalCompare(a.location.filename, b.location.filename);
+ return naturalCompare(a, b);
}
});
+
+ let sortedDiagnostics: Diagnostics = [];
+
+ for (const filename of filenames) {
+ const fileDiagnostics = diagnosticsByFilename.get(filename);
+ if (fileDiagnostics === undefined) {
+ throw new Error('We use keys() so should be present');
}
- clear() {
- this.includedKeys = new Set();
- this.diagnostics = [];
+ // Sort all file diagnostics by location start index
+ const sortedFileDiagnostics = fileDiagnostics.sort((a, b) => {
+ const aStart = a.location.start;
+ const bStart = b.location.start;
+ if (aStart === undefined || bStart === undefined) {
+ return 0;
+ } else {
+ return get0(aStart.index) - get0(bStart.index);
+ }
+ });
+
+ sortedDiagnostics = [...sortedDiagnostics, ...sortedFileDiagnostics];
+ }
+
+ return sortedDiagnostics;
}
}

MODIFY
packages/@romejs/js-compiler/api/analyzeDependencies/utils.ts
packages/@romejs/js-compiler/api/analyzeDependencies/utils.ts
@@ -12,6 +12,7 @@ import {
BindingIdentifier,
FlowTypeParameter,
ReferenceIdentifier,
+ ConstProgramSyntax,
} from '@romejs/js-ast';
import {SourceLocation} from '@romejs/parser-core';
import {
@@ -21,7 +22,13 @@ import {
FunctionBinding,
TypeBinding,
} from '@romejs/js-compiler';
-import {AnalyzeExportValueType} from '@romejs/core';
+import {
+ AnalyzeExportValueType,
+ AnalyzeDependencyResult,
+ AnyAnalyzeExport,
+ AnalyzeDependency,
+ AnalyzeDependencyName,
+} from '@romejs/core';
export function isOptional(path: Path): boolean {
for (const {node} of path.ancestryPaths) {
@@ -138,3 +145,97 @@ export function getDeclarationLoc(
return node.loc;
}
+
+function arraySame<T>(
+ a: Array<T>,
+ b: Array<T>,
+ callback: (a: T, b: T) => boolean,
+): boolean {
+ if (a.length !== b.length) {
+ return false;
+ }
+
+ for (let i = 0; i < a.length; i++) {
+ if (!callback(a[i], b[i])) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
+function syntaxSame(a: ConstProgramSyntax, b: ConstProgramSyntax): boolean {
+ return a === b;
+}
+
+function exportsSame(a: AnyAnalyzeExport, b: AnyAnalyzeExport): boolean {
+ if (a.type !== b.type) {
+ return false;
+ }
+
+ if (a.kind !== b.kind) {
+ return false;
+ }
+
+ switch (a.type) {
+ case 'local':
+ return b.type === 'local' && a.name === b.name;
+
+ case 'external':
+ return b.type === 'external' && a.imported === b.imported && a.exported ==
+ b.exported && a.source === b.source;
+
+ case 'externalAll':
+ return b.type === 'externalAll' && a.source === b.source;
+
+ case 'externalNamespace':
+ return b.type === 'externalNamespace' && a.source === b.source &&
+ a.exported === b.exported;
+ }
+}
+
+function dependencyNameSame(
+ a: AnalyzeDependencyName,
+ b: AnalyzeDependencyName,
+): boolean {
+ return a.kind === b.kind && a.name === b.name;
+}
+
+function dependenciesSame(a: AnalyzeDependency, b: AnalyzeDependency): boolean {
+ return a.all === b.all && a.async === b.async && a.optional === b.optional &&
+ a.source === b.source && a.type === b.type && arraySame(
+ a.names,
+ b.names,
+ dependencyNameSame,
+ );
+}
+
+// Check if the shape of two analyzeDependencyResults are equal. Ignoring location information
+export function areAnalyzeDependencyResultsEqual(
+ a: AnalyzeDependencyResult,
+ b: AnalyzeDependencyResult,
+): boolean {
+ if (a.firstTopAwaitLocation === undefined && b.firstTopAwaitLocation !==
+ undefined || b.firstTopAwaitLocation === undefined &&
+ a.firstTopAwaitLocation !== undefined) {
+ return false;
+ }
+
+ if (a.moduleType !== b.moduleType) {
+ return false;
+ }
+
+ if (!arraySame(a.syntax, b.syntax, syntaxSame)) {
+ return false;
+ }
+
+ if (!arraySame(a.exports, b.exports, exportsSame)) {
+ return false;
+ }
+
+ if (!arraySame(a.dependencies, b.dependencies, dependenciesSame)) {
+ return false;
+ }
+
+ return true;
+}

MODIFY
packages/@romejs/js-compiler/index.ts
packages/@romejs/js-compiler/index.ts
@@ -29,6 +29,7 @@ export * from './scope/bindings';
// utils
export * from './constants';
+export {areAnalyzeDependencyResultsEqual} from './api/analyzeDependencies/utils';
export {
getPrefixedNamespace as getPrefixedBundleNamespace,
} from './transforms/compileForBundle/_utils';

Optimize sorting of diagnostics, avoid invalidating dependents unless analyze shape has changed