Import errors hiding parse errors

[SHeimlich]

If you have a .arcs file that does not parse properly is imported by another .arcs file, the errors generated make it appear the second .arcs file has the error.

This error has been seen in the creation of the Javascript demo for tutorials. (See https://github.com/SHeimlich/arcs/tree/tutorial-demo

In the tutorial demo, the Board particle is defined in Board.arcs which is imported into Demo.arcs. There was a parse error (incorrect tabs) with one of the Cell particles in the in the Board.arcs file. Because the Board.arcs file did not parse correctly, it was not imported as is reflected in the fourth error message (see below). But, since the actual error is totally hidden from developer, it's very difficult to figure out what is going on.

Error messages that appeared in the log:

[Cypher1]

@SHeimlich Sorry, where is the error being hidden?

I think that the first error in the log is reporting the indentation problem that you have diagnosed, though it is not particularly clear.

Possibly it should be saying something more like 'Expected top level definition or import but 'inout' found'. Ideally it would actually know that this is a particle handle definition, but the parser library we are using does not make this trivial to implement.

[Cypher1]

This seems related to https://github.com/PolymerLabs/arcs/issues/2570. As errors in parsing imports should fail the importee (as it's recipes will almost definitely not resolve properly).

I checked out the branch and can run the demo now. Is there a previous commit to look at?

[SHeimlich]

I didn't commit when the issue was appearing, but I've been able to reproduce it pretty simply on my machine. If you go into Board.arcs and put a tab on line 50 before "particle DisplayAvatar3 ..." you should be able to reproduce the error messages.

I've used the term "hidden" since the error messages all relate to Board, but the actual error is coming from the DisplayAvatar3 particle definition. When debugging this, I spent a significant amount of time looking at the definition of the Board particle within Board.arcs, and never thought to scroll down and look at the other particles in the file.

By comparing this to a situation where the error is in the not-imported .arcs file. In this case I added an extra tab to line 13 of Demo.arcs and got these error messages:

I agree this seems similar to #2570.

[Cypher1]

To clarify, do you mean hidden as in not shown (I haven't been able to repro this), or hidden as in swamped in other errors (I've been able to repro this)?

I think we can improve the latter problem by failing fast (i.e. in the import, without finishing the importing file) but need @shans and @piotrswigon to sign off on changing our approach to failure (as they're the ones who I believe know why it is currently this way).

[shans]

The way the error appears to read to me is something along the lines of:

• didn't expect inout at line 56 of Board.arcs (this would be because of the indenting at line 50)
• .. so therefore couldn't import Board.arcs
• no Board defined but it's referenced in Demo.arcs
• ..so therefore couldn't import Demo.arcs

Point being that now I know exactly what's gone wrong - Tutorial.arcs couldn't import Demo.arcs, because Demo.arcs expected Board from Board.arcs; but Board.arcs had a parse error on line 50.

Failing fast would make the first error more prominent but hide how you got there; I agree what we have is wordy though. Is there any way for us to summarize errors after the first, so we still get the chain of evidence but without drowning people?

[Cypher1]

Yep. I agree about the interpretation.

Failing fast would show errors even if the containing construct didn't get used and (correspondingly) would not explain the dependency chain. We could still show the import chain which lead to the error (simple impl here: https://github.com/PolymerLabs/arcs/pull/3608).

This does remove some information that is currently available to the developer, but it also means that the particles they have written are available when they reach for them and should reduce cognitive load when doing actual debugging (as I don't have to worry about all the particles depending on my broken particle until it works in isolation).

[shans]

Can you explain your second paragraph in more detail? AFAIK "fast fail" just means die as soon as an error is detected; how does this mean that particles are available more frequently or that one doesn't need to worry about particles depending on a broken one?

[shans]

(Also how does #3608 show the import chain? It doesn't appear to)

[shans]

Sorry for the comment spamming..

From memory, there's a good reason that importing a broken manifest didn't cause the current manifest to die too - it makes a user's context extraordinarily brittle as any changed (and now failing) import nukes everything.

We could import differently into the context, and at some point we shouldn't be relying on the current version of things but rather the version at which they were instantiated into arcs; these are both bigger changes that we need to work through a design on though.

[Cypher1]

My assumption is that these parse errors would accumulate in projects (not unlike C++ warnings) slowly making devEx worse and then developers would be burdened with having to search through the (now potentially large) error logs for relevant errors. Fast failing avoids this being possible as the code base is broken until you respond to the broken code.

I'm also concerned about the potential for misinterpreting manifests if we fail part way through. Fast failing avoids this too, as if the parser fails the developer has to step in (rather than having an approach to work with some kind of best guess).

3608 shows fast fail, the imports should be accumulated via the exception stack in the pre-existing code (updated code for reference).

catch(e) {
 manifest.errors.push(new ManifestError(item.location, `Error importing '${target}'`)); // Already existing
 throw e;
}

By re-throwing, rather than pushing the exception ensures we do not continue after an error.

I agree with the need for a design on versioning of imports but hopefully this is out of scope.

[shans]

The notion of the context doesn't match your assumption; a context isn't a project that developers explicitly add to, it's the accumulation of user state. If it is brittle then the user experience is really bad as a changed file renders all the user's arcs inaccessible. I think this was actually happening; that's why we changed to this approach in the first place.

[shans]

BTW I'd be interested to see a manifest that was meaningfully misinterpreted; failing and recovering just means that we skip the contents of a manifest that was imported.

[Cypher1]

Ah, I had only been concerned with imports during development.

I think the versioning system (and capturing manifests at instantiation time) should fix the brittleness (keeping the serialized and current versions in sync is a challenging problem, equivalent to doing a database migration with every particle change), I don't know that we actually want to try to do that anyway (instead allowing users to choose to use old versions).

I don't think the error reporting system should be made to paper over this complexity, its harder to use (this is the third issue opened about import based debugging troubles that I can think of) and doesn't really fix the problem.

About the BTW, I'm pretty sure that with today's manifest language the worst result of the mis-parse would be a subset of suggestions being generated which would reduce options / confuse the user, but it's a long way from a catastrophe.

[shans]

It definitely fixed the problem.. the problem being that modifying a single manifest had the potential to render all of your arcs useless.

I don't think improving developer understanding at the expense of going back to this behavior is an acceptable trade-off.

[piotrswigon]

My .2c would be that we should be able to improve the DevEx if we improve the error reporting.

Some ideas:

• The first warning mentions the Demo.arcs file first, and then the Board.arcs file and the same line number for both (56) - this is confusing and maybe even wrong. It may be better if we only mentioned the actual file in which the error appears - the following warnings show the causality trail.
• If we have given more than one line of context (e.g. +2 surrounding lines, that could make it easier to debug issues), but that seems to be unusual... thoughts? I may be biased just because this error is padding related...

[Cypher1]

Talked to shane this morning to iron out my understanding of the context in its current form.

@piotrswigon I'd prefer to avoid printing the context (in favour of developers using their own editor highlighting, etc. This also better supports usage of the language server where error message length is visually costly).

Having the first report be the core error and the rest be the causality trail sounds very reasonable, though if we can condense this it would be very helpful.

It seems like fast fail is not going to be viable for the foreseeable future and summaries would be a huge improvement.

[piotrswigon]

Would you have bandwidth to investigate why we're having both the path to Demo.arcs and Board.arcs in the first error? I presume the thinking is that it's showing the trail of import, but IMO it would be more readable if we had the path to the file with error first, something like:

Parse error: Expected foo but found bar:
   bar bar barh
   ^

in /a/b/c/d/File.arcs:38
  imported at Demos.arcs:12
  imported at Canonical.arcs:5

Or something along these lines. WDYT?

@SHeimlich maybe could comment.

[Cypher1]

Small update to jog my memory in future: I believe this issue was mostly due to the 'retry' mechanism that allowed schemas to be added to the manifest until there was a topological ordering found that insert them such that they didn't throw any exceptions.

This was removed last quarter in favour of a two pass system: one finding all the schemas and the other populating them. I expect the exceptions to be far more understandable now.

As such, I'm closing this for now and we can re-open if we find that it's still occuring for other reasons.