SysX Activity.span fails with value is null exception

[bmazzarol]

Any usage of Activity.span will throw a value is null exception.

See attached branch and tests for example. main...bmazzarol:language-ext:bugfix/activity_issues

Tests showing the issues

https://github.com/bmazzarol/language-ext/blob/bugfix/activity_issues/LanguageExt.Tests/ActivityTests.cs
https://github.com/bmazzarol/language-ext/blob/bugfix/activity_issues/LanguageExt.Tests/FinNullabilityIssueTests.cs

Working Activity (My copy)

https://github.com/bmazzarol/language-ext/blob/bugfix/activity_issues/LanguageExt.SysX/SysX/Diag/Activity2.cs

There are a few issues,

• Fin is not nullable aware: If you create an Aff or Eff with a type signature containing a nullable T, if the value is actually null, it will fail. This is the case for activity, the spec is Eff<RT,Activity?>. See FinNullabilityIssueTests.cs
• default access to runtime: Its only safe to access the runtime via default, if the value/operation you need does not require access to an inner environment class (user provided state that can only exist when the runtime is provided to the effect). In the case of ActivitySourceIO it requires a provided ActivitySource, which can never exist in a call to default<RT>. IMHO its not a good idea to access the runtime in this way, and it should be recommended to just create a new Eff or Aff or just use runtime<RT>. This way you are always guaranteed to be accessing the correct instance of the runtime. Moreover if a developer refactors the runtime code, they dont need to worry about all call sites and how the runtime is accessed.

Starting this issue to talk through ways to resolve the follow,

• Advice around nullable types and there use in monadic types: I know that nullable reference types are nothing but compiler magic, but in net6 + repos that is the default and I have seen developers use it in code with similar effect to Fin, it was not an ideal experience. At the very least Fin needs a not null type constraint on its T
• Advice around runtime usage: the docs show accessing the runtime using default. I think that should be reconsidered,
  • Its not safe in all cases
  • Its an additional concept that developers need to learn, (adhoc polymorphism is not required when I teach FP to developers)
  • The alternative runtime<RT> is cleaner IMHO
• Fix SysX OTEL features: would prefer to not have duplicated versions of LanguageExt code in my libraries.

[bmazzarol]

One idea to consider would be an Analyser project.

There are a number of rules that are not hard to implement and would provide fixes and advise to developers, some that come to mind,

• Don't use nullable references types in monadic value types
• Replace TryAsync with Aff
• Replace Either<Exception, T> with Eff
• Replace Try with Eff
• Don't new up immutable collection types
• Don't use Result directly, replace with Fin
• ...heaps more

I know it's not a new idea, but it holds a lot of purchase with me as I have a lot of code and developers who are new to fp, analysers are great learning tools.

[louthy]

Fin is not nullable aware: If you create an Aff or Eff with a type signature containing a nullable T, if the value is actually null, it will fail. This is the case for activity, the spec is Eff<RT,Activity?>. See FinNullabilityIssueTests.cs

It's a part of the library that hasn't yet been converted to understand nullable references. But, to be clear, Fin and other monadic types like Option and Either have been designed to disallow null references. This is an explicit motivation of the library, laid out in the README. So, a null in a Fin is Errors.Bottom. This behaviour is by design.

Admitedly these motivations were born in the pre-nullable reference days, and it may be worth removing the built-in nullable protections (and the *Unsafe variants), but that's not a small job - it won't be happening anytime soon I'm afraid. The implications for existing code-bases could be quite significant and so shouldn't be taken lightly.

My advice would be not use null at all, and if it can't be avoided (because of an existing API) then convert to a Succ | Fail | Some | None | Left | Right | SuccessEff | FailEff | ... asap. Even with the new nullable references feature, it's still not perfectly type-safe.

default access to runtime: Its only safe to access the runtime via default, if the value/operation you need does not require access to an inner environment class (user provided state that can only exist when the runtime is provided to the effect). In the case of ActivitySourceIO it requires a provided ActivitySource, which can never exist in a call to default.

Which is why in the example runtimes [it's wrapped in an Eff](default access to runtime: Its only safe to access the runtime via default, if the value/operation you need does not require access to an inner environment class (user provided state that can only exist when the runtime is provided to the effect). In the case of ActivitySourceIO it requires a provided ActivitySource, which can never exist in a call to default.)

IMHO its not a good idea to access the runtime in this way, and it should be recommended to just create a new Eff or Aff or just use runtime.

I disagree, that adds unnecessary performance overhead. The idea with the use of default for ad-hoc polymorphic types is to avoid the need for any allocations; it's also something that the compiler optimises away. To be honest this is a temporary solution really, because currently I need to be able to support older .NET runtimes.

If you're building a runtime from scratch then I'd advise using static interface methods for stateless ad-hoc behaviours. I'll be looking at ways I can provide better built-in runtimes with v5.

Of course you're completely free to implement your runtimes however you like, nothing is imposed on you, (other than the HasCancel constraint for Aff) - so feel free to copy/paste any of the existing runtime, or to wrap it, in any way you see fit.

For work I am currently on a Haskell project, and we've come up with some interesting approaches to doing effectful traits that I'd like to bring inti language-ext when I get time. Also when I update to the CodeGen system to use Source-Generators, I'd like to see if there's a novel approach to generating runtimes from traits+implementations. That might be a bit pie in the sky, but the boilerplate of building the runtime should be automatable to a certain extent.

[bmazzarol]

@louthy thanks for the detailed response.

Any usage of nullable reference T within a monadic type is a bug, at the very least I think the cases of this within the library should be fixed and documentation amended to outline why it should be completely avoided. This would fix SysX and provide clear guidance for how the code is intended to work. I'm happy to do this.

I agree that using default static is cleaner and when I can use that feature I will.

I still think more specific guidance around using default runtime to access an effect which uses runtime state would benefit users. The Eff must be lazy, that destiction is not clear, the docs use SuccessEff to wrap another default implementation. I would be happy to update the docs to clarify the use cases.