Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trying to use BiTraversable #252

Closed
erlandsona opened this issue Mar 18, 2020 · 18 comments
Closed

Trying to use BiTraversable #252

erlandsona opened this issue Mar 18, 2020 · 18 comments
Assignees

Comments

@erlandsona
Copy link

I've got this code...

module Result = Relude.Result;
module ResultS = Result.WithError({type t = string});
module ResultA = ResultS.WithApplicative(Relude.Option.Applicative);
...
    ~toast=
      ResultA.bitraverse(
        const(Some("Couldn't fetch coding queries!")),
        const(None),
      ),

And I'm getting this error...

  /Users/aerlandson/code/client/src/apps/coderDashboard/data/CodingQuery.re 68:9-53

  66 ┆ ~toast=
  67 ┆   ResultA.bitraverse(
  68 ┆     const(Some("Couldn't fetch coding queries!")),
  69 ┆     const(None),
  70 ┆   ),

  This has type:
    'a => option(string)
  But somewhere wanted:
    'a => ResultA.Bitraversable.applicative_t('b)

  The incompatible parts:
    option(string)
    vs
    ResultA.Bitraversable.applicative_t('b) (defined as
      ResultS.WithApplicative(Relude_Option_Instances.Applicative).Bitraversable.applicative_t('b))

@mlms13 said it should be unifying and might be bug? So opening this to track here...

@mlms13
Copy link
Member

mlms13 commented Mar 18, 2020

I haven't had a chance to try any ideas out yet, but when I do, I'm going to start looking at the Result module in bs-abstract.

Specifically, Result's Traversable module has an explicit signature that helps unify the t type (but that signature seems to be missing the applicative_t type). For Bitraversable, the signature seems complete, but the actual module seems to be missing the signature, which is probably just an oversight that might be responsible for the fact that the compiler can't tell that applicative_t('b) and option(string) are compatible.

@mlms13 mlms13 self-assigned this Mar 18, 2020
@andywhite37
Copy link
Member

Yeah, I don't know for sure, but there have been some sneaky issues in some of the things in bs-abstract where some functions that aren't using universal quantification are accidentally unifying some type variables. I think there was an issue like this for Foldable, and I vaguely recall something with Apply/Traversable too.

@mlms13
Copy link
Member

mlms13 commented Mar 24, 2020

I made a PR to Bastet (the new bs-abstract), which has been merged. I expect that change to fix this issue. I think we need a new npm release of bs-bastet (1.2.4), and we'll need to actually depend on Bastet (#255) but once that happens, I think this should be fixed.

@erlandsona
Copy link
Author

Read through #255, but does this at least work now with the PR to bs-bastet having been merged?

@mlms13
Copy link
Member

mlms13 commented Mar 30, 2020

It will if you use bs-bastet directly, but at the moment, it won't work with Relude. I'll try to finish up #255 this afternoon and make a release, which should include this fix.

@mlms13
Copy link
Member

mlms13 commented Mar 31, 2020

Relude 0.59 is out now, with a peer-dependency on Bastet 1.2.5, which should have a fix for this issue. I think we should be good, but please reopen this if anything funny is still happening.

@mlms13 mlms13 closed this as completed Mar 31, 2020
@mlms13
Copy link
Member

mlms13 commented Apr 9, 2020

Sounds like there might still be something funny going on here. I'm not 100% sure what that may be, but I'll look into it.

@mlms13 mlms13 reopened this Apr 9, 2020
@erlandsona
Copy link
Author

erlandsona commented Apr 9, 2020

Minimal use case for tests...

        Result.fold(
          _ => Some(Error("Error loading values for field.")),
          _ => None,
        )
==
ResultA.bitraverse(const(Some("Error loading values for field")), const(None))

For a given input result('data, 'err)

@erlandsona
Copy link
Author

Little note, not sure if it helps but here's Bastet's impl.
Result.Bitraversable(SomeApplicative)

@erlandsona
Copy link
Author

Any updates on this one?

@andywhite37
Copy link
Member

@erlandsona - is this an issue in bastet or relude?

@erlandsona
Copy link
Author

@andywhite37 good question I'm instantiating my instances through Relude and judging by the error message I posted, I'm not seeing any Bastet references so I'll assume it's a Relude issue?

@mlms13
Copy link
Member

mlms13 commented Apr 21, 2020

I'm tying up some loose ends on an unrelated branch, then I'll try to reproduce this. I really thought the Bastet update should fix the option(string) vs ResultA.Bitraversable.applicative_t('b) issue, but maybe there's something else going on.

Edit: I wonder if inside Result.WithApplicative, where we define the implementation of BITRAVERSABLE, we might just need the with t('a) ... and applicative_t('b) stuff in the module signature. I'll try that out.

@andywhite37
Copy link
Member

Ok, I think maybe I have it fixed in #264. I'll make a release and let us know if this works @erlandsona

@erlandsona
Copy link
Author

Great!

@andywhite37
Copy link
Member

Ok, a hopeful fix is released in v0.62.0. I'll close this, but feel free to re-open if the problem still exists.

@andywhite37
Copy link
Member

Sorry @mlms13 - I didn't see your comment right before mine

@mlms13
Copy link
Member

mlms13 commented Apr 21, 2020

No worries, looks like we had the same idea, so hopefully that will take care of this one once and for all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants