Return Failure early in <* and *> too

[josephcsible]

Checklist:

HLint

• I've changed the exposed interface (add new reexports, remove reexports, rename reexported things, etc.).
  • I've updated hlint.dhall accordingly to my changes (add new rules for the new imports, remove old ones, when they are outdated, etc.).
  • I've generated the new .hlint.yaml file (see this instructions).

General

• I've updated the CHANGELOG with the short description of my latest changes.
• All new and existing tests pass.
• I keep the code style used in the files I've changed (see style-guide for more details).
• I've used the stylish-haskell file.
• My change requires the documentation updates.
  • I've updated the documentation accordingly.
• I've added the [ci skip] text to the docs-only related commit's name.

[chshersh]

Thanks for your contribution! However, I propose an alternative solution to this problem.

Short-circuiting behaviour of these operators can be tested manually via some unit tests later.

[josephcsible]

I also added a unit test to ensure the short-circuiting works correctly, for all of the Applicative methods.

[chshersh]

Thanks! I feel like documentation for the Validation module can be improved. But I have plans on how to do this and will do it separately 👍

[vrom911]

Sorry, I'm slow, but I don't see the point of these changes 🤔 Could you please explain what benefits this PR gives the library?

[josephcsible]

Consider this code:

case thisWillFail *> thisWillTakeALongTime of
  Failure _ -> print "It didn't work."
  Success _ -> print "It worked!"

Since we know that the whole expression will be a Failure after the LHS fails, we now emit Failure itself sooner, so that thisWillTakeALongTime doesn't need to be evaluated unless/until the argument to Failure is evaluated.

[chshersh]

After thinking more about this PR and discussing it with @vrom911, I now believe that removing code and potentially breaking working code is not a good idea. In general, it's preferable to avoid changing the code in such a considerable way without confidence. And the current example in the documentation is also not helpful. So let's not merge this PR until we have better testing and documentation story for Validation.

[vrom911]

Sure, I do understand that, but I'm talking exactly about this PR. This feels like unchecked breaking change. @chshersh are you sure you would like to take such a risk? I'm personally using Validation in production, so I wouldn't hurry to get to the newer version of relude then.

[josephcsible]

Am I missing something? How is this a breaking change?

[chshersh]

@vrom911 Agree with you here. I'm also using Validation in production and it works. We have a separate issue to implement tests for Validation. So let's not change the code yet if it's untested.

[josephcsible]

Actually, this is worse than just a performance problem; we're not a valid Applicative:

The other methods have the following default definitions, which may be overridden with equivalent specialized implementations:

• u *> v = (id <$ u) <*> v
• u <* v = liftA2 const u v

(emphasis mine) Ours aren't equivalent. If u = Failure ["foo"] and v = undefined, then the LHS of both of those evaluate to undefined, but the RHS of both evaluate to Failure undefined.

[josephcsible]

Now that #176 is fixed, is this ready to merge?

[chshersh]

@josephcsible Well, since we have tests now, we actually can implement our own more optimised versions of these operators (like it how was done at first). Because now we have more guarantees that they will work. And we won't rely on the inliner too much as a pleasant consequence.

[josephcsible]

@chshersh Done.

[chshersh]

@josephcsible Thanks for the refactoring, looks good 👍

[vrom911]

Thanks, @josephcsible !

[vrom911]

Heh, I wish these variable names could make a little sense, so the examples could become useful 😢
@chshersh should I create an issue for refactoring this?

[chshersh]

@vrom911 Yes, please!

[vrom911]

#221