Convert other errors to diagnostics when possible

[rictic]

I think this converts the rest of the error events that are caused by file locations to use diagnostics.

The commits are pretty targeted and should be fairly easy to review one at a time.

[aomarks]

Looks like some Windows failures relating to path separators. Probably need to substitute ${pathlib.sep} into some test cases.

[rictic]

(this is ready for review, the errors are github cache errors, will rerun shortly)

[aomarks]

Cool!

[aomarks]

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.

[aomarks]

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

[aomarks]

😢

[aomarks]

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?

[rictic]

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?

[rictic]

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

[aomarks]

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.

[rictic]

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.