nim-lang / fusion

Fusion is for now an idea about how to grow Nim's ecosystem without the pain points of more traditional approaches.
MIT License
128 stars 16 forks source link

`matching` bug with `{orc,arc}` #76

Closed deech closed 3 years ago

deech commented 3 years ago

The following matching example:

import options
import fusion/matching
{.experimental: "caseStmtMacros".}
type
  OKind* = enum O1 O2
  O* = object
    case kind*: OKind
    of O1: o1*: int
    of O2: o2*: Option[string]

case O(kind:O2,o2:some[string]("hello world")):
  of O2(o2:Some(@mystring)): echo mystring

produces the error:

/home/deech/NimScratch/match.nim(11, 1) template/generic instantiation of `case` from here
/home/deech/.nimble/pkgs/fusion-#4f0de4c2c50c8c204fa18d6f46b3bc38f7ff873e/fusion/matching.nim(2249, 16) Error: type mismatch: got <O>
but expected one of: 
proc get[T](self: Option[T]): lent T
  first type mismatch at position: 1
  required type for self: Option[get.T]
  but expression 'expr_369098795' is of type: O
proc get[T](self: Option[T]; otherwise: T): T
  first type mismatch at position: 1
  required type for self: Option[get.T]
  but expression 'expr_369098795' is of type: O
proc get[T](self: var Option[T]): var T
  first type mismatch at position: 1
  required type for self: var Option[get.T]
  but expression 'expr_369098795' is of type: O

expression: get(expr_369098795)

nim-compile exited abnormally with code 1 at Sun Mar  7 11:25:45

but only when compiled with gc:orc or gc:arc.

Nim version:

$ nim --version
Nim Compiler Version 1.5.1 [Linux: amd64]
Compiled at 2021-03-07
Copyright (c) 2006-2021 by Andreas Rumpf

git hash: d1e093207acdd10ad375705bd1732fc6f0db9f55
active boot switches: -d:release
deech commented 3 years ago

The problem appears to be here: https://github.com/nim-lang/fusion/blob/master/src/fusion/matching.nim#L1495

If you print the path argument inside the for-loop you get different output each time.

haxscramper commented 3 years ago

It seems like macro implementation behaves differently based on --gc:arc vs regular GC option. Input ASTs are identical in both cases, so most likely there is some subtle difference in how macro works with different GCs - the only explanation that seems reasonable to me.

deech commented 3 years ago

Yeah it seems the existing tests fail with gc:{orc,arc}:

$ nim cpp --gc:arc tmatching.nim
/tmp/fusion/tests/tmatching.nim(47, 7) template/generic instantiation of `suite` from here
/tmp/fusion/tests/tmatching.nim(345, 13) template/generic instantiation of `multitest` from here
/tmp/fusion/tests/tmatching.nim(373, 5) template/generic instantiation of `case` from here
/tmp/fusion/src/fusion/matching.nim(1980, 19) Error:  no `len` defined for tuple[c: array[0..2, int]] - needed to find number of elements for pattern [_, _, _]

Works fine without.

deech commented 3 years ago

You're right, it looks like a Nim VM bug: https://github.com/nim-lang/Nim/issues/17294

timotheecour commented 3 years ago

https://github.com/nim-lang/Nim/issues/17199

deech commented 3 years ago

https://github.com/nim-lang/Nim/pull/17348 fixes the issue.

timotheecour commented 3 years ago

@deech this needs a test case in fusion even though the underlying bug was fixed in stdlib; can you make a PR?

deech commented 3 years ago

@haxscramper has already written tests for it: https://github.com/nim-lang/fusion/pull/77/files?file-filters%5B%5D=.nim#diff-33abcca7f03798f7fd369f34051711499dee90cea8d05859cd933d31ce115f09R46. It includes the example case I posted, once the changes to matching. nim are backed out since that workaround is no longer necessary all the rest of the work (doc cleanup and tests) should be pulled into fusion.

haxscramper commented 3 years ago

https://github.com/nim-lang/fusion/pull/77 PR has a test case, although it triggers only when orc is enabled, which is enabled by https://github.com/nim-lang/fusion/pull/79. This bug does not have an "edge case", but instead stems from broken behavior on current stable

haxscramper commented 3 years ago

https://github.com/nim-lang/fusion/pull/77 provides a simple fix that can be reverted (or implemented as no-op for version 1.4.6+) when current level becomes the latest stable (which will certainly take some time). Until the new version of nim is release I think #77 provides sufficient fix.