Non global error handling

[ksjogo]

Move the error handling to a class to have non-global errors.
Use them in tests and runners.

Depends on #60

[johnfn]

The irony is that I refactored error handling to be global not too long ago: 0b71b67

The thing is, passing around error objects simply became too annoying. Yes, it's not too hard to have them in the parseXXX functions, but passing them into helper functions, utility functions, other parts of the code... it just became too much to think about - logging a simple error shouldn't have such a big overhead.

[ksjogo]

Oh, I missed that.
Though adding it to the project seemed to work quite well in not being annoying I would say.
As everything is kind of project contained anyway.

Imo we definitely need everything to be none global though long-term.
I want to add parallel tests in the future and that would def clash with global error handling.

The solution might be in a combination of throwing real errors and adding parse errors. As throws will have the right context.
What do you dislike specifically about the attached changeset?

It is two changes:

• import { ErrorName, addError } from "../errors" -> import { ErrorName } from "../project/errors".
• addError -> props.project.errors.add.

[johnfn]

So, throwing errors seems to be a no-go for me. The thing is, we almost always need to keep going - even if foo.ts has an error, we still want to look through bar.ts - and there's no easy way to do that while throwing errors. This is what got me to thinking about an addError() style approach in the first place - a way to append errors to a list rather than just stopping after we see the first one.

There were a few reasons that I ended up not liking non-global error handling:

1. Utility functions: Even simple utility functions might need to have a project passed in if you ever need to throw an error. Right now, it's not too bad, because a lot of our parseNode functions are massive functions in a single file. However, when we split them into smaller functions, this will become more annoying.
2. Threaded call stacks: I was running into cases where function A calls B calls C calls D. If I need to add an error in D, I need to add props.project to A, B and C. This is a lot of change for a diff which should really just be a single addError call.

The TL;DR is, we want to throw errors everywhere in our project. That means that non-global error throwing will add an extra argument to practically every function argument list we have. That's a high readability price to pay.

I see your point about having parallel test runners. However, I think we can find a compromise! addError takes a location. As long as that location is a node, we can use .getSourceFile() on the node to get the source file, and then use that inside addError() to know which parallel test triggered the error. Then when you do displayErrors() you should just pass in the current TS project, and we can figure out which source files that project contains and then only display errors related to those source files.

Obviously, the above isn't anything that has to go into this diff right now. :)

[ksjogo]

Sure, we could try to link back errors to a project by essentially registering that project on some global error handler and then passing in nodes.

But I don't see right now, where we would need to add that.
The parse_node/ code has access to ParseState which can be used to get the current project/error context.
(If we don't like that, we could probably also extend ParseNodeType with an error field (and combine merging errors)).

So what is left (if I am correct) is error handling for non-parse stuff.
Where indeed some functions might want to add errors.
Maybe the cleanest way here is to use some OOP. The Project class creates the other classes it needs and passes down a reference to itself. Then utility functions within these classes can use the error handler of the passed down project?