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

Update to 1.2.0-RC Raise DSL #13

Merged
merged 4 commits into from
Apr 4, 2023

Conversation

nomisRev
Copy link
Contributor

This PR updates the rules to also check for Raise, I ran into some issues _detektEffect since it's now a _typealias_ and since it's asuspend lambda` it seems all information about the typealias get completely erased.

At least I could not find any reference to it when debugging the rules.

Copy link
Collaborator

@diastremskii diastremskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing, thank you for your contribution! I think we can either merge it into arrow-next branch and release as a snapshot (adding testImplementation(platform("io.arrow-kt:arrow-stack:2.0.0-SNAPSHOT")) + snapshots repo to build.gradle.kts) or just keep a PR open until there is a new non-snapshot version of Arrow released with Raise support.

Comment on lines 121 to 132
val isPackage = (type.getAbbreviatedType()
?.abbreviation
?.constructor
?.declarationDescriptor
?.containingDeclaration as? PackageFragmentDescriptorImpl
)?.fqName?.asString() == "arrow.core.raise"

val isRaiseEffect =
(type.getAbbreviatedType()?.abbreviation?.constructor?.declarationDescriptor?.name?.asString() == "EagerEffect") ||
(type.getAbbreviatedType()?.abbreviation?.constructor?.declarationDescriptor?.name?.asString() == "Effect")

return isBindableFqNames || (isPackage && isRaiseEffect)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
val isPackage = (type.getAbbreviatedType()
?.abbreviation
?.constructor
?.declarationDescriptor
?.containingDeclaration as? PackageFragmentDescriptorImpl
)?.fqName?.asString() == "arrow.core.raise"
val isRaiseEffect =
(type.getAbbreviatedType()?.abbreviation?.constructor?.declarationDescriptor?.name?.asString() == "EagerEffect") ||
(type.getAbbreviatedType()?.abbreviation?.constructor?.declarationDescriptor?.name?.asString() == "Effect")
return isBindableFqNames || (isPackage && isRaiseEffect)
val isRaiseExtensionFunction = type.annotations.hasAnnotation(FqName("kotlin.ExtensionFunctionType")) &&
type
.arguments
.firstOrNull()
?.type
?.constructor
?.declarationDescriptor
?.fqNameSafe == FqName("arrow.core.raise.Raise")
return isBindableFqNames || isRaiseExtensionFunction

Would it be safe to assume that any extension function on Raise is bindable? That should fix the problem with typealiases as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, actually the contrary. An extension function on Raise will typically not return a wrapper.

I.e.

fun Raise<String>.example(): Int = raise("failure")

val either: Either<String, Int> = either { example() }

If it does return a wrapper, it should be caught by one of the other predicate in isBindable. I was trying to detect Effect & EagerEffect which now are respectively typealias Effect<E, A> = suspend Raise<E>.() -> A and typealias EagerEffect<E, A> = Raise<E>.() -> A.

Since they're not types anymore, they can not as easily be detekted. So what I was doing in the predicate was trying to look for builders of Effect (and EagerEffect).

import arrow.core.raise.Effect
import arrow.core.raise.effect
                 
fun test(): Effect<Throwable, Int> = effect {
  effect <Throwable, Int> { 1 }
  1
}

The problem I faced was that while EagerEffect shows up as an abbreviation which I assumed was compiler language for typealias, that is not the case for Effect which simply turns up as SuspendFunction 😟

Not sure if there is other ways of detecting this, for example:

  • Checking if arrow.core.raise.effect was invoked inside a Raise<E>.() -> A builder and not bound.

Copy link
Collaborator

@diastremskii diastremskii Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, actually the contrary. An extension function on Raise will typically not return a wrapper.

Oh yeah, I think I worded it wrong, would it be safe to assuming that any expression with type Raise<Error>.() -> A (regardless of suspend) is bindable (inside suitable scope)?

But that's an excellent example with fun Raise<String>.example()! Seems like we'd need to support it as well because body of such extension function is a "bindable scope" as well. What do you think about something like this, does it look like it would cover all use cases? 🤔
All of the tests from NoRaiseBindableValueAsStatementTest pass in this branch except for Validated test due to using 2.0.0-SNAPSHOT (now all of them pass after changing the version to 1.1.6-alpha.91 :D)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about something like this, does it look like it would cover all use cases? 🤔

That looks awesome!! 😍 Your changes looks much cleaner as well 🥳
How do you want to proceed? You should be able to push your changes directly to this PR, and then we can merge it ☺️ These changes will be released before KotlinConf 😉

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you! Let's merge it then :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Released version 0.2.0 with these changes

Comment on lines +34 to +48
@Test
fun `reports unbound Validated`() {
val code = """
import arrow.core.Validated
import arrow.core.raise.Effect
import arrow.core.raise.effect

fun test(): Effect<Throwable, Int> = effect {
Validated.Valid(1)
1
}
"""
val findings = NoEffectScopeBindableValueAsStatement(Config.empty).lintWithContext(env, code)
findings shouldHaveSize 1
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't Validated removed in Arrow 2? At least with 2.0.0-SNAPSHOT this test case fails due to not having arrow.core.Validated 🤔

@nomisRev
Copy link
Contributor Author

nomisRev commented Apr 3, 2023

Wasn't Validated removed in Arrow 2? At least with 2.0.0-SNAPSHOT this test case fails due to not having arrow.core.Validated

Yes, but I am here updating to 1.2.0-RC which is a midden version between 1.x.x and 2.x.x. Consider 2.0.0 == 1.2.0 - deprecated code. So I wanted to still support this, and then remove it when 2.x.x lands.

In practice, we might want to keep it around longer. God forbid someone is stuck on 1.x.x. I truly hope this is not the case for anyone, which is why I am working hard on collaborating with OpenRewrite to provide large-scale automatic refactoring for 1.x.x -> 2.x.x.

@diastremskii diastremskii merged commit c8aa77f into woltapp:main Apr 4, 2023
@nomisRev
Copy link
Contributor Author

nomisRev commented Apr 4, 2023

Awesome, thank you for the help @diastremskii ❤️ And supporting Arrow with this great project.

@nomisRev nomisRev deleted the update-to-1.2.0-raise branch April 4, 2023 09:40
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

Successfully merging this pull request may close these issues.

2 participants