codex-storage / questionable

Elegant optional types for Nim
Other
114 stars 5 forks source link

Optional chaining does not work when chaining proc #6

Closed TheSimpleZ closed 3 years ago

TheSimpleZ commented 3 years ago

Hello again!

Really like the library, nice work. However, I think I found another issue.

This code:

import questionable

proc printVal*(someval: int) =
  echo someval

some(0).?printVal()

produces

----\lib\test.nim(6, 8) template/generic instantiation of `.?` from here
----\questionable-0.8.0\questionable\chaining.nim(36, 5) template/generic instantiation of `.?` from here
----\questionable-0.8.0\questionable\chaining.nim(12, 30) Error: expression 'printVal(unsafeGet(some(0)))' has no type (or is ambiguous)

I would expect it to print 0.

Compiler version: Nim Compiler Version 1.4.6 [Windows: amd64]

Questionable version: questionable@0.8.0

markspanbroek commented 3 years ago

Yes, that’s an interesting case. The printVal proc does not return a value. The .? chaining produces either a none() or a some(), but it’s unclear what it should do in the case of a void return type.

In this code I would use an if statement with option binding instead of .? chaining:

if value =? some(0):
  value.printVal()
TheSimpleZ commented 3 years ago

I see! Yeah, I was trying out that syntax because the option binding didn't work in a generic statement (#7). Then the error message confused me. I'm quite new to Nim and I haven't really explored macros yet. Would it be possible to detect the method's return value and if it's void, produce nothing?

The use case for this is the ability to write one-liners in cases like this:

import questionable

type o = object
    a: int

proc produceOption*(): Option[int] =
  result = o(a: 1).some

proc printVal*(someval: int) =
  echo someval

# this
produceOption().?a.?.printVal()

# rather than
if opt =? produceOption():
  opt.a.printVal()

With if statements it can get quite verbose if you have multiple procs producing options in the chain

markspanbroek commented 3 years ago

I think it should be possible to expand the macro to also work in the case of void return types as you suggested. I am hesitant to implement it though. Procs without return types typically have side effects. When you add one of these to the end of a .? chain you’ve created a subtle branching point that either produces the side effect or not. I have a feeling that this could be easily overlooked when it is embedded in a larger piece of code, which is why I prefer the more explicit if-statement in this case.

If you have multiple procs producing options in the chain, then you should still only need a single if-statement. Assuming that procs foo, bar and baz all produce options, you can write:

if value =? a.?foo().?bar().?baz():
  value.printValue()

By the way: the error message that you got was indeed not helpful. I’ll check whether I can improve it.

markspanbroek commented 3 years ago

Improved the error message in d18580bb689f09f3f2d8e33f946d3b508d6a9a6e. Update to version 0.9.1 to see the new error message :)

izackp commented 2 years ago

Procs without return types typically have side effects

I'd argue the opposite. They operate on only 1 piece of data restricting the reach of side effects to globals or internals of the data. Unless you take a fully functional point of view then all procs that aren't funcs are side effects.

myRect.moveToZero()
myIcon.freeSurface() # required sdl api whether or not this has side effects is not affected by your library
myIcon.?freeSurface() # definitely a quality of life improvement

Another point, if you take a look at kotlin and swift they all support functions without return values.

subtle branching point

It's pretty explicit in my opinion. You expect the operator to do 2 things. Execute the function or not. Developer intent is clear too: If this exists do x, but if not then who cares.

I don't think in anyway you're going to reduce code quality by implementing void proc support.

markspanbroek commented 2 years ago

Hi @izackp, thanks for sharing your thoughts. You're correct that the semantics of .? on a function without a return value can be pretty simple. Execute the function or not, as you said. I chose not to allow it in questionable because it can be easily overlooked in a larger piece of code. When I glance over a function, I like to quickly identify the branching points. This is easy when there are properly indented if and for statements, which visually highlight a block of code that is conditionally executed. A .? somewhere inside an expression is much harder to spot, and can therefore lead to surprises.