Does this overlap with #4646 or supercede it?

Some cleanup work that I stumbled upon while writing this just got merged into next with 8195d68 and v2.1.x with 4f52441. From CR 3144.

#4646 seems to cover the first checkbox of this.

👍

I agree for all of these (i.e. think that they should be ReqlCompileErrors), with the following exceptions:

• "Maximum expression depth exceeded" / "Nesting depth limit exceeded" which are clearly caused by a driver limitation and not by a ReQL limitations or a compilation problem. We should probably consider making these ReqlResourceLimitErrors instead.
• "Calling '>=' on result of infix bitwise operator:" / "Calling '>=' on result of infix bitwise operator:" I'm unsure about this one. It's more of a language-specific syntax problem, than a ReQL compilation problem I think.

"Maximum expression depth exceeded" / "Nesting depth limit exceeded" which are clearly caused by a driver limitation and not by a ReQL limitations or a compilation problem. We should probably consider making these ReqlResourceLimitErrors instead.

I disagree with this because a ReqlResourceLimitError inherits from ReqlRuntimeError, which indicates that it was returned by the server. I do not think that a driver should be raising a ReqlRuntimeError on its own. As you said, it is a driver/language-specific limitation, so it should probably remain a ReqlDriverError.

I don't think any of these should return CompileErrors because all of them are cases that fail before the server even tries to compile the query. The only one that I think is borderline is the Expected X arguments one in JS, but I think we should solve that by not enforcing arity in the drivers and letting the server produce those errors.

I do agree that we should probably distinguish these errors from other "the driver is buggy" DriverErrors. Maybe we should introduce a new error class like QueryBuildingError to represent errors that occur while building the query to send to the server?

@mlucy Well the whole premise of this proposal is that users don't care whether something has reached the server or not. Arity errors for example are sometimes caught in the driver, and sometimes on the server. For the user, that doesn't make the slightest difference.
I think it makes sense to move away from distinguishing between where errors are handles, and move towards categorizing errors based on what the error is.

To add to that, I think we have a pretty big hierarchy, there doesn't seem to be a reason to double it to distinguish server/driver errors in building the query

Arity errors for example are sometimes caught in the driver

I agree that arity errors shouldn't be ReqlDriverErrors. I think we should just not enforce arity in the clients at all.

I think it makes sense to move away from distinguishing between where errors are handles, and move towards categorizing errors based on what the error is.

Errors like "you exceeding the arbitrary nesting depth for r.expr that we picked in this driver" or "you passed us an object that we weren't able to turn into JSON to send to the server" or "you used an infix operator in a way that is likely incorrect" all seem fundamentally different to me than "you sent a query to the server and the server rejected it as malformed". It's sort of like the difference between a parsing error and a compilation error, and I think it's useful to distinguish them. (They're at least as different from compilation errors as runtime errors are, and we already distinguish those.)

Aside from the intellectual argument that the errors are different, it's really useful to know what component you need to blame for an error. For example, if you're using a third-party driver and you write code that you think is correct but which produces a driver error, you know that you should open a bug with the driver maintainer rather than in the main RethinkDB repo. If it produces a compilation error, though, you know that the server is rejecting it, so you know that you should open a bug in the main RethinkDB repo and/or try upgrading your server.

Actually, probably the way we should handle this is to have two subclasses of compilation error: ServerCompilationError and DriverCompilationError. That will let us distinguish the two but still let people catch and handle both at the same time if they want to. (Arguably we should have DriverRuntimeError too, but I can't think of any errors off the top of my head that fall into that category.)

-1 for new exceptions

Specifically, I don't think solving this issue should involve creating new exception types

I actually think subclassing ServerCompileError and DriverCompileError from a common CompileError class makes sense. I think that's a pretty good compromise.

In practice users will not need to care about this distinction. If some code needs to handle (usually probably log/report and then fail) compilation errors, it can just keep catching ReqlCompileError.

Yeah, thinking about it more I think my favorite solution is breaking CompileError into two subclasses. It's "new exception types", but only in the sense that we offer more granularity if people want it, we don't require people to think about the distinction if they don't want to.

Since exceptions are part of the protocol, we should have a really compelling use case for adding new ones.

The reason to have an exception class is because you need to programmatically distinguish between types of errors and handle some of them automatically. For determining where you need to file a bug, you just look at the stack trace in your logs, so the error message is enough. No one is going to automatically file a bug report from their app.

My recommendation is to add something like "(client detected error)" to the exception messages thrown by the driver, or distinguish them in some way in the message, rather than by adding a new exception to the protocol.

I really don't think the cost of adding a new exception type is that high, and the distinction seems important to me.

I also disagree that exception types only exist to allow for programmatic handling of different errors: types are for humans as well as machines. But even if we accepted that premise, there are probably about as many reasons to distinguish ReQL driver errors from ReQL server errors as there are to distinguish ReQL compilation errors from ReQL runtime errors. For example, you may want to log them at different priority levels, or send an email to your DBAs for one class of error or to whoever maintains your app deployment for the other (since one class of error can be caused by RethinkDB being out of date and the other can be caused by your driver being out of date). For another example, you may be generating queries based on user input, where compilation errors on the server can be caused by the user making a mistake but driver errors (like misuse of infix operators) can only be caused by your program being wrong. (Assuming we change things so that arity errors aren't detected by the client.)

Of your suggested use-cases above, which of those can't be used to justify the creation of any new exception class? For example, splitting ArityError from NullLambdaReturnValue?

I'm not saying no one will ever need these distinctions, but the classes should be along the lines of things people commonly want distinguished programmatically. I think a good way of determining what these cases are is to wait for people to ask to distinguish them, rather than speculate beforehand, adding complexity to the protocol in the process (albeit a small amount).

Given that compilation errors are going to mostly be encountered during initial coding of the app, not as much during running it, I don't think they warrant this level of granularity.

I think we're getting into the weeds a little bit. To sum up what I think briefly:

• The two error classes here occur in completely different parts of the system. They occur in different programs on different servers, possibly maintained by different people on different upgrade schedules. Usually errors that occur in different parts of a system are distinguished (see e.g. parsing errors vs. compilation errors vs. linking errors vs. distcc errors vs. runtime errors). This is what I expect software to do.
• The two error classes are more dissimilar than other error classes (compilation vs. runtime errors) that we already distinguish, so it feels consistent with choices we've already made to distinguish them.
• The cost of distinguishing them is extremely low.

(Also, from the perspective of personal experience, I have never once used a system and thought to myself "I wish there weren't so many exception subclasses". Never once. I just use the superclass if I don't care. On the other hand, not having exception granularity when you need it is extraordinarily frustrating.)

Fair enough, I don't think it's important enough to warrant arguing it to death, so I won't. I suspect we just have different thresholds for when it is best to add an exception class. In my experience, most people don't end up using any of the fine grained exception types and it's wasted effort creating these ontologies.

Marking as settled with the following modification from the original proposal:

• We'll add two subclasses of ReqlCompilationError: ReqlServerCompilationError and ReqlDriverCompilationError. All CompilationErrors that are thrown by the server will be mapped to a ReqlServerCompilationError. The errors listed in the original post will be changed to instances of ReqlDriverCompilationError.

I'm going to start working on this a bit.

The fix is up in CR 3248 with @larkost .

Merged into next with 660ab61