effect icon indicating copy to clipboard operation
effect copied to clipboard

proposal for Effect.Tag conundrum

Open patroza opened this issue 1 year ago • 13 comments

(I left out the changeset on purpose; what is the policy for changesets for fixes for unreleased changes)

patroza avatar Mar 08 '24 19:03 patroza

🦋 Changeset detected

Latest commit: 9a00d023a01ba61da4cdf8f226e9f9def986d443

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
effect Patch
@effect/cli Patch
@effect/experimental Patch
@effect/opentelemetry Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-node-shared Patch
@effect/platform-node Patch
@effect/platform Patch
@effect/printer-ansi Patch
@effect/printer Patch
@effect/rpc-http Patch
@effect/rpc Patch
@effect/schema Patch
@effect/typeclass Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Mar 08 '24 19:03 changeset-bot[bot]

would be a patch, not sure I understand the fix though

mikearnaldi avatar Mar 09 '24 07:03 mikearnaldi

not sure I understand the fix though

andThen & friends detect a function, while underlying it may be an Effect (aka the conundrum). it's going to call that function, which in turn can really check if the underlying service member is a function to be called, or an Effect to return. solving the disambiguity. once it has been able to check if its a function to call, or an Effect to return, it caches that knowledge for the next calls so that you only pay the price once.

the requirement for this to work consistently, is that at every place we want to deal with both functions and effects, we must first check if it's a function, only then if it's an Effect. as luck so has it (or well, it must be done that way anyway), that appears to be the case already.

would be a patch

isn't that weird, to have a changelog entry for an unreleased feature?

patroza avatar Mar 09 '24 07:03 patroza

not sure I understand the fix though

andThen & friends detect a function, while underlying it may be an Effect (aka the conundrum). it's going to call that function, which in turn can really check if the underlying service member is a function to be called, or an Effect to return. solving the disambiguity. once it has been able to check if its a function to call, or an Effect to return, it caches that knowledge for the next calls so that you only pay the price once.

the requirement for this to work consistently, is that at every place we want to deal with both functions and effects, we must first check if it's a function, only then if it's an Effect. as luck so has it (or well, it must be done that way anyway), that appears to be the case already.

would be a patch

isn't that weird, to have a changelog entry for an unreleased feature?

Not really weird if we link changelogs to PRs. RE the fix, I am kind of unsure, there would be a mismatch between type and what happens behind the scenes, this would work with andThen but it feels like it's made as a special case for that.

cc @tim-smart

mikearnaldi avatar Mar 09 '24 08:03 mikearnaldi

There is one other case in cli that needs addressing:

image

Also, instead of using a map you could put the cached props back on the class itself:

image

tim-smart avatar Mar 12 '24 00:03 tim-smart

Hmm I think the only way to ensure functions are functions, and effects are effects is to separate the access like the previous revision.

tim-smart avatar Mar 12 '24 00:03 tim-smart

Hmm I think the only way to ensure functions are functions, and effects are effects is to separate the access like the previous revision.

What’s wrong with my solution? esp. if we apply your suggestion to put it back on the class.

patroza avatar Mar 12 '24 06:03 patroza

Ah yeah thinking about it some more your solution will work :)

Could also re-use the cn variable for the constant case.

tim-smart avatar Mar 12 '24 06:03 tim-smart

Ah yeah thinking about it some more your solution will work :)

Could also re-use the cn variable for the constant case.

Awesome. Yea sure, who doesn’t like free stuff :)

patroza avatar Mar 12 '24 06:03 patroza

The one case where it will still fail is if something expects a union of a function that doesn't return an Effect, or a plain Effect. I.e LazyArg<A> | Effect<A>

tim-smart avatar Mar 12 '24 06:03 tim-smart

The one case where it will still fail is if something expects a union of a function that doesn't return an Effect, or a plain Effect. I.e LazyArg<A> | Effect<A>

Got a failing test?

patroza avatar Mar 12 '24 07:03 patroza

Hmm I think the only way to ensure functions are functions, and effects are effects is to separate the access like the previous revision.

The issue is that it's valid to have a function be an effect, it's a dsl that we used in the past too if I recall in platform, so this issue is kind of orthogonal to tag

mikearnaldi avatar Mar 12 '24 07:03 mikearnaldi

Hmm I think the only way to ensure functions are functions, and effects are effects is to separate the access like the previous revision.

The issue is that it's valid to have a function be an effect, it's a dsl that we used in the past too if I recall in platform, so this issue is kind of orthogonal to tag

Only in implementation or also on the type level? If also on type level it should be fine as long as andThen and friends have the same bias type wise as implementation wise

patroza avatar Mar 12 '24 07:03 patroza

Hmm I think the only way to ensure functions are functions, and effects are effects is to separate the access like the previous revision.

The issue is that it's valid to have a function be an effect, it's a dsl that we used in the past too if I recall in platform, so this issue is kind of orthogonal to tag

Only in implementation or also on the type level? If also on type level it should be fine as long as andThen and friends have the same bias type wise as implementation wise

No not necessarily, the function will return something different and expect potentially different arguments. It's not safe to assume andThen behaviour

mikearnaldi avatar Mar 12 '24 07:03 mikearnaldi

No not necessarily, the function will return something different and expect potentially different arguments. It's not safe to assume andThen behaviour

How does such a case look like? can we add some failing tests?

patroza avatar Mar 12 '24 08:03 patroza

No not necessarily, the function will return something different and expect potentially different arguments. It's not safe to assume andThen behaviour

How does such a case look like? can we add some failing tests?

Something like:

interface Wrapper<A> extends Effect.Effect<A> {
  (arg: number): string
}

const wrapper = <A>(f: () => A): Wrapper<A> => {
  function wrapped(n: number): string {
    return `Hello: ${n}`
  }
  const base = Effect.sync(f)
  Object.assign(wrapped, base)
  Object.setPrototypeOf(wrapped, Object.getPrototypeOf(base))
  return base as any
}

Effect.andThen(Effect.unit, wrapper(() => "OK"))

is currently prohibited by the NotFunction thing, I guess we have the choice: "it is not safe to discriminate between functions and effects but it can be safe to discriminate between functions and effects that are not functions"

in the case above we will be forced to rewrite to:

Effect.andThen(Effect.unit, () => wrapper(() => "OK"))

which will then work as expected

mikearnaldi avatar Mar 12 '24 08:03 mikearnaldi

After a good amount of thinking it I think it is safe to discriminate between Function and NotFunction so this looks like it is working. The only troubles I can see are in tacit usage cases which we are advising against anyway

mikearnaldi avatar Mar 16 '24 14:03 mikearnaldi