status-im / nim-stew

stew is collection of utilities, std library extensions and budding libraries that are frequently used at Status, but are too small to deserve their own git repository.
133 stars 18 forks source link

fix `ProveField` warnings in `results` #198

Closed etan-status closed 1 year ago

etan-status commented 1 year ago

Nim emits ProveField warnings with if on case object discriminator. Replace with case instead to avoid those warnings. Note we currently have ProveField disabled but it keeps showing up sometimes when compiling with nim c instead of make.

etan-status commented 1 year ago

Note, this doesn't get rid of all ProveFields warnings, as some need to suppress it at call side (e.g., var).

However, it still, overall, reduces the amount of warnings quite a lot, so is possibly a net gain overall.

etan-status commented 1 year ago

Nim devel 🤔 (seems to be alright on master)

/home/runner/.nimble/pkgs2/unittest2-0.0.7-b6d4a5cbe28b43c166d6442ba6804aafd4abe368/unittest2.nim(900) test_interval_set
/home/runner/work/nim-stew/nim-stew/tests/test_interval_set.nim(129) runTest`gensym123
/home/runner/work/nim-stew/nim-stew/stew/interval_set.nim(1164) verify
/home/runner/work/nim-stew/nim-stew/stew/interval_set.nim(345) new
/home/runner/work/nim-stew/nim-stew/stew/interval_set.nim(295) right
/home/runner/work/nim-stew/nim-stew/stew/interval_set.nim(289) blk
/home/runner/work/nim-stew/nim-stew/nim/lib/system/arc.nim(43) nimIncRef
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
etan-status commented 1 year ago

@araq re Nim devel issue. Something in this PR seems to mess with orc.

Araq commented 1 year ago

Same problem as https://github.com/nim-lang/Nim/issues/22058 ?

etan-status commented 1 year ago

Same problem as nim-lang/Nim#22058 ?

Not sure. The linked problem also occurs on Nim 1.6, this one here is devel only.

There is also no {.noSideEffects.} here

tersec commented 1 year ago

Nim devel issues should not block development progress. I stand by what I wrote in https://github.com/status-im/nim-ssz-serialization/pull/47#issuecomment-1584890664

But it remains that the primary purpose of the nim-ssz-serialization CI is to make nim-ssz-serialization in the context of supported Nim versions (of which constantly-changing devel is not one), so if devel in the future causes too-regular CI regressions, it might prove warranted to reconsider this situation.

s/nim-ssz-serialization/nim-stew/g, but otherwise identical.

etan-status commented 1 year ago

@ringabout Reran on Nim fda8b6f193e2e229488f76f18089f01eb08272fb which includes https://github.com/nim-lang/Nim/pull/22088

It's still broken here :) So, likely a separate bug.

ringabout commented 1 year ago

Seems to be a 2.0 to devel regression.

arnetheduck commented 1 year ago

Nim devel issues should not block development progress.

they shouldn't, but we can also use the opportunity to rid upstream of debilitating bugs that might come to bite us later - ie this is not an urgent PR in the sense that there's no runtime bug being fixed so we can merge it when convenient

tersec commented 1 year ago

Nim devel issues should not block development progress.

they shouldn't, but we can also use the opportunity to rid upstream of debilitating bugs that might come to bite us later - ie this is not an urgent PR in the sense that there's no runtime bug being fixed so we can merge it when convenient

These can happen simultaneously: if/as useful, create a branch, destined never to be merged, which can function as a repro for the bug in question. Then fix master. Then both activities can happen in parallel: upstream Nim can fix as best suits them, and meanwhile, nim-stew development isn't hindered.

ringabout commented 1 year ago

I'm fixing it, which seems to be a piece of cake => https://github.com/nim-lang/Nim/pull/22093

etan-status commented 1 year ago

Looks good now on devel