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

["Request"] Specify AT_MOST_ONCE and EXACTLY_ONCE in applicable lambdas #2806

Closed
nomisRev opened this issue Aug 23, 2022 · 20 comments
Closed

Comments

@nomisRev
Copy link
Member

In most HOF in Arrow we can specify AT_MOST_ONCE and EXACTLY_ONCE.
Currently, we're not doing so, and this prevents certain code to be written that would otherwise be valid.

@poseidon2060
Copy link
Contributor

Hey, can I work on this issue?
I am relatively new to Android and I hope this can be a good beginner issue for me

@nomisRev
Copy link
Member Author

Hey @poseidon2060,

Yes, this is a great issue to get started with since you can also start small and add AT_MOST_ONCE and EXACTLY_ONCE only in a couple or a single place at the same time. I would suggest getting familiar with Kotlin contracts and callsInPlace, and looking for any file in libs/arrow-core/commonMain and find any lamda were you can add these annotations.

This doesn't have to be done in the entire project in 1 PR or contribution. Feel free to add it only to a single lambda where you think it's applicable and opening a PR 👍

@poseidon2060
Copy link
Contributor

Okay thankyou
Can I ask here for my help and doubts, or do I have to join Slack?

@nomisRev
Copy link
Member Author

Feel free to ask here @poseidon2060, or in a DRAFT PR if you prefer 👍

@poseidon2060
Copy link
Contributor

Hey @nomisRev

I have familiarized myself with Kotlin contracts and callsInPlace but I am unable to figure out where to apply them in the project
I browsed the path that you mentioned but am unable to understand exactly where I can add these annotations
Can you guide me slightly on what to look for, help appreciated

@nomisRev
Copy link
Member Author

Hey @poseidon2060,

If we look for example at Either, there are many places where AT_MOST_ONCE can be added. I.e. for fold you know that either the function for ifLeft or ifRight will be called AT_MOST_ONCE

To add this annotations for Either.kt, you need to inspect the method body and see if the lambda is called AT_LEAST_ONCE or AT_MOST_ONCE and add that into the method body. This should only be done for methods that are not @Deprecated.

If you any more questions be sure to ask them ☺️ I hope this helps.

@poseidon2060
Copy link
Contributor

Hey, @nomisRev

Thank you for being so helpful!
I tried out what you said for fold and this is what I changed

@OptIn(ExperimentalContracts::class)
  public inline fun <C> fold(ifLeft: (left: A) -> C, ifRight: (right: B) -> C): C =
    when (this) {
      is Right -> {
        contract { callsInPlace(ifRight,InvocationKind.AT_MOST_ONCE) }
        ifRight(value)
      }
      is Left -> {
        contract { callsInPlace(ifLeft,InvocationKind.AT_MOST_ONCE) }
        ifLeft(value)
      }
    }

Am I doing it correctly?
I hope I didn't do something terribly wrong😅

@hoc081098
Copy link
Contributor

I think should move all contract blocks to the top body of function.

contract {
  callInPlace(ifRight, 
...)
  callInPlace(ifLeft, ...)
}
when(this) { ... }

@poseidon2060
Copy link
Contributor

Hey @hoc081098

Do you mean like this?

@OptIn(ExperimentalContracts::class)
  public inline fun <C> fold(ifLeft: (left: A) -> C, ifRight: (right: B) -> C): C {
    contract {
      callsInPlace(ifLeft, InvocationKind.AT_MOST_ONCE)
      callsInPlace(ifRight, InvocationKind.AT_MOST_ONCE)
    }
    return when (this) {
      is Right -> ifRight(value)
      is Left -> ifLeft(value)
    }
  }

@nomisRev
Copy link
Member Author

Yes, the second snippet as suggested by @hoc081098 is the correct encoding. contract needs to be the first statement in the function body.

@poseidon2060
Copy link
Contributor

poseidon2060 commented Jan 24, 2023

Yes, the second snippet as suggested by @hoc081098 is the correct encoding. contract needs to be the first statement in the function body.

So can I open a PR?

@nomisRev
Copy link
Member Author

Absolutely ☺️

@poseidon2060
Copy link
Contributor

poseidon2060 commented Jan 24, 2023

Done!
Thank you @nomisRev and @hoc081098 for being so helpful😊

@poseidon2060
Copy link
Contributor

Hey @nomisRev, I have created a Draft PR adding Invocation at 4 more places, can you please let me know if it's correct so that I can mark it Ready for Review🤞

@poseidon2060
Copy link
Contributor

Hey @nomisRev, should I continue working on this issue or get started on a new one?
I don't know if I am supposed to complete this for all the files or not, I'd do it if you want🤔

@nomisRev
Copy link
Member Author

Hey @poseidon2060,
This needs to happen for all files (you can ignore @Deprecated code but also feel free to work on an other issue if you like ☺️

@poseidon2060
Copy link
Contributor

Hey @poseidon2060, This needs to happen for all files (you can ignore @Deprecated code but also feel free to work on an other issue if you like ☺️

Okay, I'll start working on a new issue while also continuing working on this issue in the background. I think this way I'll learn the most.😄
Do you have an issue to recommend for my skill level?

@nomisRev
Copy link
Member Author

@poseidon2060 awesome 🙌 Thank you for taking so much interest to contributing!
These might be some interesting tickets to look into:

Feel free to ask any questions on the respective issues directly, or on any other issues you find interesting and tag me 😉
I'll do my best to add some more beginner friendly tickets in the coming weeks.

@poseidon2060
Copy link
Contributor

@nomisRev Okay thank you, I will look into these issues and try to solve them
Contributing to open source is really fun, I am glad I started with this repository😊

@serras
Copy link
Member

serras commented Feb 6, 2023

Closed by and #2901 and #2915

@serras serras closed this as completed Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants