Skip to content

Commit

Permalink
Convert other errors to diagnostics when possible (#183)
Browse files Browse the repository at this point in the history 
* Use a diagnostic for script not found errors.

I was thinking that we wanted a Location in case we wanted to combine this error with information about what's requesting the script, but we can always pull the Location off of the Diagnostic in that case, and in the meantime we get a nice diagnostic.

* Convert duplicate dep error to diagnostic

* Convert JSON syntax error to diagnostic

* Convert cycle error to diagnostic

It was sad to lose the cool ascii diagrams, but all of the wonderful logic got reused, and in the case of multi package cycles I think this will be clearer.

* Remove CachingPackageJsonReaderError

We're already passing in the script placeholder, so it can just throw a proper WireitError now.

Also remove invalid-package-json error, as it's now replaced by json-syntax-error.

* Fix tests on windows.

* Address review feedback
  • Loading branch information
@rictic
rictic authored May 6, 2022
1 parent 3644a58 commit 287f5eb
Show file tree
Hide file tree
Showing 7 changed files with 260 additions and 184 deletions.
144 changes: 104 additions & 40 deletions src/analyzer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,15 @@
*/

import * as pathlib from 'path';
import {WireitError} from './error.js';
import {Diagnostic, MessageLocation, WireitError} from './error.js';
import {
CachingPackageJsonReader,
JsonFile,
} from './util/package-json-reader.js';
import {scriptReferenceToString, stringToScriptReference} from './script.js';
import {scriptReferenceToString} from './script.js';
import {AggregateError} from './util/aggregate-error.js';
import {findNamedNodeAtLocation, findNodeAtLocation} from './util/ast.js';

import type {CachingPackageJsonReaderError} from './util/package-json-reader.js';
import type {
ScriptConfig,
ScriptReference,
Expand Down Expand Up @@ -117,28 +116,10 @@ export class Analyzer {
* upgraded; dependencies are upgraded asynchronously.
*/
async #upgradePlaceholder(placeholder: PlaceholderConfig): Promise<void> {
let packageJson;
try {
packageJson = await this.#packageJsonReader.read(
placeholder.packageDir,
placeholder
);
} catch (error) {
const reason = (error as CachingPackageJsonReaderError).reason;
if (
reason === 'missing-package-json' ||
reason === 'invalid-package-json'
) {
// Add extra context to make this exception a full WireitError.
throw new WireitError({
type: 'failure',
reason,
script: placeholder,
});
} else {
throw error;
}
}
const packageJson = await this.#packageJsonReader.read(
placeholder.packageDir,
placeholder
);

const scriptsSection = findNamedNodeAtLocation(
packageJson.ast,
Expand Down Expand Up @@ -171,11 +152,15 @@ export class Analyzer {
type: 'failure',
reason: 'script-not-found',
script: placeholder,
locationOfScriptsSection: {
file: packageJson,
range: {
offset: scriptsSection.offset,
length: scriptsSection.length,
diagnostic: {
severity: 'error',
message: `Script "${placeholder.name}" not found in the scripts section of this package.json.`,
location: {
file: packageJson,
range: {
offset: scriptsSection.name.offset,
length: scriptsSection.name.length,
},
},
},
});
Expand Down Expand Up @@ -268,8 +253,26 @@ export class Analyzer {
reason: 'duplicate-dependency',
script: placeholder,
dependency: resolved,
astNode: unresolved,
duplicate,
diagnostic: {
severity: 'error',
message: `This dependency is listed multiple times`,
location: {
file: packageJson,
range: {offset: unresolved.offset, length: unresolved.length},
},
supplementalLocations: [
{
message: `The dependency was first listed here.`,
location: {
file: packageJson,
range: {
offset: duplicate.offset,
length: duplicate.length,
},
},
},
],
},
});
}
uniqueDependencies.set(uniqueKey, unresolved);
Expand Down Expand Up @@ -439,27 +442,88 @@ export class Analyzer {
trail: Set<ScriptReferenceString>
) {
const trailKey = scriptReferenceToString(config);
const supplementalLocations: MessageLocation[] = [];
if (trail.has(trailKey)) {
// Found a cycle.
const trailArray = [];
let cycleStart = 0;
// Trail is in graph traversal order because JavaScript Set iteration
// Trail is in graph traversal order because JavaScript Map iteration
// order matches insertion order.
let i = 0;
for (const visited of trail) {
trailArray.push(stringToScriptReference(visited));
if (visited === trailKey) {
for (const visitedKey of trail) {
if (visitedKey === trailKey) {
cycleStart = i;
}
i++;
}
trailArray.push({packageDir: config.packageDir, name: config.name});
const trailArray = [...trail].map((key) => {
const placeholder = this.#placeholders.get(key);
if (placeholder == null) {
throw new Error(
`Internal error: placeholder not found for ${key} during cycle detection`
);
}
return placeholder as ScriptConfig;
});
trailArray.push(config);
const cycleEnd = trailArray.length - 1;
for (let i = cycleStart; i < cycleEnd; i++) {
const current = trailArray[i];
const next = trailArray[i + 1];
const nextIdx = current.dependencies.indexOf(next);
const dependencyNode = current.dependenciesAst?.children?.[nextIdx];
// Use the actual value in the array, because this could refer to
// a script in another package.
const nextName =
dependencyNode?.value ?? next?.name ?? trailArray[cycleStart].name;
const message =
next === trailArray[cycleStart]
? `${JSON.stringify(current.name)} points back to ${JSON.stringify(
nextName
)}`
: `${JSON.stringify(current.name)} points to ${JSON.stringify(
nextName
)}`;

const culpritNode =
// This should always be present
dependencyNode ??
// But failing that, fall back to the best node we have.
current.configAstNode?.name ??
current.scriptAstNode?.name;
supplementalLocations.push({
message,
location: {
file: current.declaringFile,
range: {
offset: culpritNode.offset,
length: culpritNode.length,
},
},
});
}
const diagnostic: Diagnostic = {
severity: 'error',
message: `Cycle detected in dependencies of ${JSON.stringify(
config.name
)}.`,
location: {
file: config.declaringFile,
range: {
length:
config.configAstNode?.name.length ??
config.scriptAstNode?.name.length,
offset:
config.configAstNode?.name.offset ??
config.scriptAstNode?.name.length,
},
},
supplementalLocations,
};
throw new WireitError({
type: 'failure',
reason: 'cycle',
script: config,
length: trail.size - cycleStart,
trail: trailArray,
diagnostic,
});
}
if (config.dependencies.length > 0) {
Expand Down
29 changes: 5 additions & 24 deletions src/event.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
* SPDX-License-Identifier: Apache-2.0
*/

import {Diagnostic, Location} from './error.js';
import {Diagnostic} from './error.js';
import type {
ScriptConfig,
ScriptReference,
PackageReference,
} from './script.js';
import type {JsonAstNode, ParseError} from './util/ast.js';

/**
* Something that happened during Wireit execution. Includes successes,
Expand Down Expand Up @@ -72,7 +71,6 @@ export type Failure =
| SpawnError
| LaunchedIncorrectly
| MissingPackageJson
| InvalidPackageJson
| NoScriptsSectionInPackageJson
| PackageJsonParseError
| ScriptNotFound
Expand Down Expand Up @@ -131,14 +129,7 @@ export interface MissingPackageJson extends ErrorBase<ScriptReference> {
*/
export interface PackageJsonParseError extends ErrorBase<ScriptReference> {
reason: 'invalid-json-syntax';
errors: ParseError[];
}

/**
* A package.json file was invalid.
*/
export interface InvalidPackageJson extends ErrorBase<ScriptReference> {
reason: 'invalid-package-json';
diagnostics: Diagnostic[];
}

/**
Expand All @@ -154,7 +145,7 @@ export interface NoScriptsSectionInPackageJson
*/
export interface ScriptNotFound extends ErrorBase<ScriptReference> {
reason: 'script-not-found';
locationOfScriptsSection: Location;
diagnostic: Diagnostic;
}

/**
Expand Down Expand Up @@ -187,8 +178,7 @@ export interface DuplicateDependency extends ErrorBase<ScriptReference> {
* The dependency that is duplicated.
*/
dependency: ScriptReference;
astNode: JsonAstNode;
duplicate: JsonAstNode;
diagnostic: Diagnostic;
}

/**
Expand All @@ -197,16 +187,7 @@ export interface DuplicateDependency extends ErrorBase<ScriptReference> {
export interface Cycle extends ErrorBase<ScriptReference> {
reason: 'cycle';

/**
* The number of edges in the cycle (e.g. "A -> B -> A" is 2).
*/
length: number;

/**
* The walk that was taken that resulted in the cycle being detected, starting
* from the root script.
*/
trail: ScriptReference[];
diagnostic: Diagnostic;
}

// -------------------------------
Expand Down
61 changes: 9 additions & 52 deletions src/logging/default-logger.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,9 @@ export class DefaultLogger implements Logger {
break;
}
case 'invalid-json-syntax': {
console.error(
`❌${prefix} Invalid JSON syntax in package.json file in ${event.script.packageDir}`
);
break;
}
case 'invalid-package-json': {
console.error(
`❌${prefix} Invalid JSON in package.json file in ${event.script.packageDir}`
);
for (const diagnostic of event.diagnostics) {
console.error(this.#diagnosticPrinter.print(diagnostic));
}
break;
}

Expand All @@ -132,17 +126,11 @@ export class DefaultLogger implements Logger {
);
break;
}
case 'script-not-found': {
console.error(
`❌${prefix} No script named "${event.script.name}" was found in ${event.script.packageDir}`
);
break;
}
case 'script-not-wireit': {
console.error(this.#diagnosticPrinter.print(event.diagnostic));
break;
}
case 'invalid-config-syntax': {
case 'script-not-found':
case 'duplicate-dependency':
case 'script-not-wireit':
case 'invalid-config-syntax':
case 'cycle': {
console.error(this.#diagnosticPrinter.print(event.diagnostic));
break;
}
Expand All @@ -156,12 +144,7 @@ export class DefaultLogger implements Logger {
);
break;
}
case 'duplicate-dependency': {
console.error(
`❌${prefix} The dependency "${event.dependency.name}" was declared multiple times`
);
break;
}

case 'signal': {
console.error(`❌${prefix} Failed with signal ${event.signal}`);
break;
Expand All @@ -170,32 +153,6 @@ export class DefaultLogger implements Logger {
console.error(`❌${prefix} Process spawn error: ${event.message}`);
break;
}
case 'cycle': {
console.error(`❌${prefix} Cycle detected`);
// Display the trail of scripts and indicate where the loop is, like
// this:
//
// a
// .-> b
// | c
// `-- b
const cycleEnd = event.trail.length - 1;
const cycleStart = cycleEnd - event.length;
for (let i = 0; i < event.trail.length; i++) {
if (i < cycleStart) {
process.stderr.write(' ');
} else if (i === cycleStart) {
process.stderr.write(`.-> `);
} else if (i !== cycleEnd) {
process.stderr.write('| ');
} else {
process.stderr.write('`-- ');
}
process.stderr.write(this.#label(event.trail[i]));
process.stderr.write('\n');
}
break;
}
}
break;
}
Expand Down
2 changes: 1 addition & 1 deletion src/script.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export interface ScriptConfig extends ScriptReference {
* }
* ```
*/
scriptAstNode: NamedAstNode<string> | undefined;
scriptAstNode: NamedAstNode<string>;

/**
* The entire config in the wireit section. i.e.:
Expand Down
Loading

0 comments on commit 287f5eb

Please sign in to comment.