Replace CopyResult with exceptions?

[tkoolen]

I find CopyResult a little cumbersome to work with.

If you call a bunch of functions that could fail in some circumstances in your copy! method, they all have to return a CopyResult in addition to their normal output arguments, and then in copy! itself you have to continually check whether those subfunctions returned a non-Success CopyResult.

Less importantly, it's also annoying that you need to pass in a dud index map when you're about to return a bad CopyResult, even though the index map is useless since it maps to nothing.

Why not just have some documented Exception subtypes in MOI and require that those be thrown if something goes wrong?

[mlubin]

What if we move the indexmap outside of CopyResult and then return a Union{IndexMapType,CopyResult}?

Given that Julia doesn't provide a way to catch only certain types of exceptions (afaik), if we switched from result objects to exceptions, the proper way to call MOI.copy! would be:

try
    indexmap = MOI.copy!(a, b)
catch e
    if e isa MOI.CopyError
        ...
    else
        rethrow()
    end
end

That's pretty painful and is bound to not be implemented correctly. OTOH, with result objects it's clear what the caller has to do.

[tkoolen]

The lazy implementer will just write

indexmap = MOI.copy!(a, b)

without the try-catch, in which case the program stops and you get a nice stacktrace showing exactly where things went wrong (another advantage over returning a CopyResult). I think this is the most common use case: an error during copy! will usually be fatal: the user just did something wrong. The try-catch scenario is much rarer (with #188 (comment) being an example of where it would be needed).

I would actually argue that the current implementation is much more error prone, as the caller is required to handle the CopyResult manually. I've personally already been bitten by the current lack of handling in JuMP.

[mlubin]

I'll let others weigh in here.

[odow]

I prefer throwing an (informative) exception if anything goes wrong. It seems simpler and Miles is right, the alternative is not going to be implemented correctly.

[blegat]

I am in favor the using Exceptions and have copy! just return an index map.
We decided the result of copy! to be human friendly with a message. If you want to check whether you can do something automatically in case a copy fail, you can use supportsconstraint and supports.
This means that most of the time, if copy! fails, we should just let the human know so most of the scripts will just do copy! without checking for exception.
When implementing copy!, the exceptions make it less error prone as it is easy to forget to check the result status, e.g. we currently do:
https://github.com/JuliaOpt/MathOptInterface.jl/blob/adb5ce4fb8ac51e17e9dce981a14983c296a1f8b/src/Utilities/copy.jl#L107-L108

[mlubin]

The error-prone argument for the caller isn't especially tied to exceptions, we could also return Union{IndexMap, CopyErrorCode}. I'm less worried about errors in the implementation of copy! because this should all be tested... However, there seems to be a consensus for exceptions, so I'll consider PRs.