On the way to questionable v1.0

michaelsbradleyjr commented 2 years ago

During the Codex team call yesterday (04 Aug 2022), I mentioned still experiencing some problems using this lib's without/=? facility.

I ran into the problems when working on nim-datastore, so I went through that codebase today and did/attempted some cleanup using without on branch without_demo. I added some hopefully helpful comments re: the problems, and I'll include snippets below.

I made sure I'm using the latest questionable (0.10.4), and I'm running Nim v1.6.6 installed via choosenim on macOS.

(1)

L59-L71 in datastore/tiered_datastore.nim

var
  bytesOpt: ?seq[byte]

for store in self.stores:
  # the following commented line does not produce the expected result at runtime:
  # without bytesOpt =? await store.get(key), error:

  # instead we use intermediate identifier `bOpt`
  without bOpt =? await store.get(key), error:
    return failure error

  # and now assign `bopt` value to `bytesOpt`
  bytesOpt = bOpt

(2)

L161-L184 in datastore/key.nim

for s in namespaces:
  # if `, error` is used instead of `, err` then compilation fails with:
  # Error: cannot use symbol of kind 'func' as a 'template'
  # maybe related to `from pkg/stew/results as stewResults` at top of module,
  # which could be removed if other problems with questionable are resolved

  # but when using `, err` then compilation fails with:
  # SIGSEGV: Illegal storage access

  # but strangely the last call in the stack trace pertains to L211 below

  # without ns =? Namespace.init(s), err:
  #   return failure "namespaces contains an invalid Namespace: " & err.msg
  #
  # nss.add ns

  let
    nsRes = Namespace.init(s)

  if nsRes.isErr:
    return failure "namespaces contains an invalid Namespace: " &
      nsRes.error.msg

  nss.add nsRes.get

L177-L184 should be commented out when uncommenting L172-L175.

(3)

L205-L211 in datastore/key.nim

# if `, error` is used instead of `, err` then compilation fails with:
# Error: cannot use symbol of kind 'func' as a 'template'
# maybe related to `from pkg/stew/results as stewResults` at top of module,
# which could be removed if other problems with questionable are resolved
without key =? Key.init(nsStrs), err:
  return failure "id string contains an invalid Namespace:" &
    err.msg.split(":")[1..^1].join("").replace("\"\"", "\":\"")

The SIGSEGV runtime error mentioned in the comments for L161-L184 suggests that somehow the without expansions in two different proc (though having the same name init) are interfering with one another, but I have not yet dug deeper into it.

michaelsbradleyjr commented 2 years ago

I've encountered another problem using without/=?. Actually, it's one I had in mind during the call, but I forgot to include it in my write-up above.

(4)

See https://github.com/status-im/nim-codex/blob/138_Add_SQLite_BlockStore_backend/codex/stores/sqlitestore.nim#L46-L50.

Locally I bumped vendor/questionable to 0.10.4, made sure to clean out nimcache just in case, and changed the code linked above from

let
  datastoreRes = SQLiteDatastore.new(repoDir)

if datastoreRes.isErr:
  raise (ref Defect)(msg: datastoreRes.error.msg)

to

without datastore =? SQLiteDatastore.new(repoDir), error:
  raise (ref Defect)(msg: error.msg)

After the change, compilation fails with

Error: undeclared field: 'msg'

markspanbroek commented 2 years ago

Thanks for making a detailed error report @michaelsbradleyjr! I took the liberty of adding numbers to the various errors that you saw, so that I can refer to them more easily in this comment:

(1) seems to be caused by shadowing of an existing variable. You declare bytesOpts as a variable, but then also use the name bytesOpts in a =? expression. This declares a new bytesOpts variable (as a let this time), which shadows the variable in the outer scope.

(2) seems to fail because (3) fails?

(3) fails because nested without statements were not always handled well. A fix is underway in #21.

I haven't looked into (4) yet.

To be continued..

markspanbroek commented 2 years ago

Issues (2), (3) and (4) have been solved in #21 and #22. The fixes are available in release 0.10.5.

codex-storage / questionable

On the way to questionable v1.0 #19

(1)

(2)

(4)