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.

Sign up for GitHub

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 other errors to diagnostics when possible #183

Merged
merged 7 commits into from
May 6, 2022
Merged

Convert other errors to diagnostics when possible #183

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a6e167f
Use a diagnostic for script not found errors.
rictic May 4, 2022
6910969
Convert duplicate dep error to diagnostic
rictic May 4, 2022
e02c6bb
Convert JSON syntax error to diagnostic
rictic May 4, 2022
f42c30b
Convert cycle error to diagnostic
rictic May 4, 2022
3930f02
Remove CachingPackageJsonReaderError
rictic May 4, 2022
0b77f65
Fix tests on windows.
rictic May 4, 2022
8de3d79
Address review feedback
rictic May 6, 2022
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
142 changes: 99 additions & 43 deletions src/analyzer.ts
View file
Open in desktop
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 @@ -88,7 +87,7 @@ export class Analyzer {
// We can safely assume all placeholders have now been upgraded to full
// configs.
const rootConfig = rootPlaceholder as ScriptConfig;
this.#checkForCyclesAndSortDependencies(rootConfig, new Set());
this.#checkForCyclesAndSortDependencies(rootConfig, new Map());
return rootConfig;
}

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 @@ -436,30 +439,83 @@ export class Analyzer {

#checkForCyclesAndSortDependencies(
config: ScriptConfig,
trail: Set<ScriptReferenceString>
trail: Map<ScriptReferenceString, ScriptConfig>
rictic marked this conversation as resolved.
Show resolved Hide resolved
) {
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.keys()) {
if (visitedKey === trailKey) {
cycleStart = i;
}
i++;
}
trailArray.push({packageDir: config.packageDir, name: config.name});
const trailArray = [...trail.values()];
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 =
Copy link
Member

@aomarks aomarks May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like a difference here is that we used to generate a display label for each script that was always relative to the current package (see DefaultLogger.#label ), so that everything was phrased relative to where the user currently is. But now it uses the verbatim specifier from the dependency array, which is relative to each package.

Also, we'll probably support $WORKSPACES:build syntax (#23), to mean "all build scripts in this package's workspaces". So in that case, the literal dependency specifier wouldn't actually tell you what the specific package is.

So I think it might be good to preserve the DefaultLogger.#label method behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but the printed trail has the filenames relative to the current package so that you can follow along. Like, if we there was a cycle of "foo" -> "build" -> "$WORKSPACES:build" -> "./child:build" -> "foo" then the diagnostic printer would print (leaving out line and column numbers):

X package.json Cycle detected in dependencies of "foo"
    "foo": {
    ~~~~~

    package.json "foo" points to "build"
        "dependencies": ["build"],
                         ~~~~~~~

    package.json "build" points to "$WORKSPACES:build"
        "dependencies": ["$WORKSPACES:build"],
                         ~~~~~~~~~~~~~~~~~~~

    ./child/package.json "build" points back to "..:foo"
        "dependencies": ["..:foo"],
                         ~~~~~~~~

Or in the case where the loop is more like "./child:build" -> "./child:foo" -> "build" -> "$WORKSPACES:build" and starting from the child directory then it would like:

X package.json Cycle detected in dependencies of "build"
    "build": {
    ~~~~~~~

    package.json "build" points to "foo"
        "dependencies": ["foo"],
                         ~~~~~

    package.json "foo" points to "..:build"
        "dependencies": ["..:build"],
                         ~~~~~~~~~~

    ../package.json "build" points back to "$WORKSPACES:build"
        "dependencies": ["$WORKSPACES:build"],
                         ~~~~~~~~~~~~~~~~~~~

That's a bit harder to read. Maybe we'd want that last line to read more like this?

    ../package.json "build" points back to "$WORKSPACES:build" which includes "child:build"

What would your ideal error message look like?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could generate a diagnostic for the IDE with all the supplemental locations but keep the previous error formatting in the logger

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I see it is not so simple.

Was sort of thinking something like this:

X package.json Cycle detected in dependencies of "build"
    "build": {
    ~~~~~~~

    "build" points to "foo"
        "dependencies": ["foo"],
                         ~~~~~

    "foo" points to "..:build"
        "dependencies": ["..:build"],
                         ~~~~~~~~~~

    "..:build" points back to "build"
        "dependencies": ["$WORKSPACES:build"],
                         ~~~~~~~~~~~~~~~~~~~

I do like that it's just a single, consistent syntax for referring to a script, which matches exactly how you would specify that script in the package you started from.

But then we lose the package.json references, which are required and/or useful? If you keep them but use the consistent script specifier, then it seems sort of confusing because it's no longer relative to each line's package.json.

X package.json Cycle detected in dependencies of "build"
    "build": {
    ~~~~~~~

    package.json "build" points to "foo"
        "dependencies": ["foo"],
                         ~~~~~

    package.json "foo" points to "..:build"
        "dependencies": ["..:build"],
                         ~~~~~~~~~~

    ../package.json "..:build" points back to "build"
        "dependencies": ["$WORKSPACES:build"],
                         ~~~~~~~~~~~~~~~~~~~

Meh, I'm also fine with it the way it already was in this PR.

I kind of liked the cycle diagram (which is actually taken exactly from Bazel), but it's also really nice to have symmetrical errors on the console vs in the editor, so I'm fine with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

The package.json references are useful so that someone can cmd-click on them to jump directly to that specific location, as well as to power the supplemental locations feature in the language server provider, which does fancy presentation of the diagnostic in the editor.

One other consideration is that this way of displaying it is much wordier and longer than the cycle diagram, but I feel like cycles are rare in the wild, and when you do have one it's better for it to be longer like this.

dependencyNode?.value ?? next?.name ?? trailArray[cycleStart].name;
const message =
next == trailArray[cycleStart]
rictic marked this conversation as resolved.
Show resolved Hide resolved
? `${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 All @@ -474,7 +530,7 @@ export class Analyzer {
}
return a.name.localeCompare(b.name);
});
trail.add(trailKey);
trail.set(trailKey, config);
for (const dependency of config.dependencies) {
this.#checkForCyclesAndSortDependencies(dependency, trail);
}
Expand Down
29 changes: 5 additions & 24 deletions src/event.ts
View file
Open in desktop
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
Open in desktop
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':
Copy link
Member

Choose a reason for hiding this comment

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

We could collapse some or all of these validation error interfaces into one, now that the formatter isn't really responsible for formatting them anymore.

Copy link
Member

Choose a reason for hiding this comment

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

(doesn't have to be this PR, just a thought)

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
Copy link
Member

Choose a reason for hiding this comment

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

😢

// .-> 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
Open in desktop
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