fix: Remove unnecessary associated type Error from SolverModel

[jacobsvante]

Was set to ResolutionError everywhere.

[lovasoa]

That would be a breaking change. Can you elaborate on why you want this ?

The associated Error type is set to ResolutionError in all of its implementations inside this crate. But the SolverModel trait is public, and people can implement it with any error they want.

[jacobsvante]

Indeed it would be a breaking change.

My reason for wanting it removed is because I receive this error:

  --> src/main.rs:15:18
   |
14 | fn solve(model: impl SolverModel) -> Result<(), MyError> {
   |                                      ------------------- expected `MyError` because of this
15 |     model.solve()?;
   |           -------^ the trait `From<<impl SolverModel as SolverModel>::Error>` is not implemented for `MyError`, which is required by `Result<(), MyError>: FromResidual<Result<Infallible, <impl SolverModel as SolverModel>::Error>>`
   |           |
   |           this can't be annotated with `?` because it has type `Result<_, <impl SolverModel as SolverModel>::Error>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = note: required for `Result<(), MyError>` to implement `FromResidual<Result<Infallible, <impl SolverModel as SolverModel>::Error>>`

For more information about this error, try `rustc --explain E0277`

for the following code:

use good_lp::SolverModel;

#[derive(Debug, thiserror::Error)]
enum MyError {
    #[error("Validation failure for {0}")]
    ValidationFailure(String),
    #[error("Resolution error")]
    Resolution(#[from] good_lp::ResolutionError),
    // TODO: How to support SolverModel:Error errors?
    // #[error(transparent)]
    // Solver(#[from] SolverModel::Error),
}

fn solve(model: impl SolverModel) -> Result<(), MyError> {
    model.solve()?;
    Ok(())
}

fn main() {
    println!("Hello, world!");
}

... And I haven't managed to work around that.

[lovasoa]

Oh, I see ! I think what you need is just a where condition in your solver function:

fn solve<T: SolverModel>(model: T) -> Result<(), MyError> 
  where MyError: From<<T as SolverModel>::Error>
{
    model.solve()?;
    Ok(())
}

Or if you really need the error to be an actual ResolutionError (to match on it for instance):

fn solve(model: impl SolverModel<Error = ResolutionError>) -> Result<(), MyError>  {
    model.solve()?;
    Ok(())
}

We should add this to the documentation, maybe as an example in the SolverModel trait documentation. Do you want to contribute that instead ? A nice explanation would probably go a long way for people who encounter the same issue as you.

The documentation for the trait is here, and it is really not very helpful at the moment:

good_lp/src/solvers/mod.rs

Line 176 in d4a4942

/// A solver's own representation of a model, to which constraints can be added.

[jacobsvante]

Thanks @lovasoa for the suggestion, it did fix the issue. I had no idea you could do that. Thank you for educating me!

However... It does make the code much less readable IMO. Considering that all internal SolverModel implementations use ResolutionError, is the flexibility really worth it when considering the complexity it gives? Maybe something to consider for a 2.0 release..?

[lovasoa]

I think the current trade-off between

• having users who need a generic solve function add <Error = ResolutionError> when they need it and
• forbidding solvers with custom errors completely

is good. I don't think <Error = ResolutionError> really degrades readability that strongly.

But it is not well documented and not present enough in examples today. A contribution to the documentation would be very welcome.

[jacobsvante]

Sorry, didn't see your updated reply. That's much more readable!

I'll go ahead and close this. I'll try to find some time later to create documentation PR.