Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

matching bug with {orc,arc} #76

Closed
deech opened this issue Mar 7, 2021 · 10 comments
Closed

matching bug with {orc,arc} #76

deech opened this issue Mar 7, 2021 · 10 comments

Comments

@deech
Copy link

deech commented Mar 7, 2021

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 deech changed the title matching bug with `{orc,arc matching bug with {orc,arc} Mar 7, 2021
@deech
Copy link
Author

deech commented Mar 7, 2021

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
Copy link
Contributor

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
Copy link
Author

deech commented Mar 7, 2021

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
Copy link
Author

deech commented Mar 7, 2021

You're right, it looks like a Nim VM bug: nim-lang/Nim#17294

@timotheecour
Copy link
Member

nim-lang/Nim#17199

@deech
Copy link
Author

deech commented Mar 26, 2021

nim-lang/Nim#17348 fixes the issue.

@timotheecour
Copy link
Member

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

@deech
Copy link
Author

deech commented Mar 26, 2021

@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
Copy link
Contributor

haxscramper commented Mar 26, 2021

#77 PR has a test case, although it triggers only when orc is enabled, which is enabled by #79. This bug does not have an "edge case", but instead stems from broken behavior on current stable

@haxscramper
Copy link
Contributor

haxscramper commented Mar 26, 2021

#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.

Araq pushed a commit that referenced this issue Jun 3, 2021
…efault>` implementation (#77)

* [TEST]

* [TEST] More tests

* [FIX] #76

* [TEST] String length test

* [DOC] Spec for `opt`

* [DOC] `doc` interaction with `any`

* [DOC] Scale back `opt` features

* [CHECKPOINT] Initial `opt` implementation

* [FEATURE] `opt` implementation

- ADDED ::
  - implement `opt` for field access

* [FIX] Additional check for `nil` values

- CHANGED ::
  Before calling check function on nested match element check using
  `isNil` if entry can is correct. This avoids almost unfixable
  nil-based exceptions triggered deeply within pattern.

* [CLEAN] Internal naming cleanup

- CHANGED ::
  - Use more readable, unabreviated field names (cnt -> foundCount,
    fldElems -> fieldElements)

* [FIX] make style check shut the fuck up

* [FIX] Also bitch with error on typos

* [FIX] oh no, stdlib does not pass style check

 Compiling /home/runner/work/fusion/fusion/fusion/tests/tmatching (from package fusion) using c backend
Error: /home/runner/work/fusion/fusion/nim/lib/std/private/underscored_calls.nim(43, 21) Error: 'nnkArgList' should be: 'nnkArglist'
       Tip: 5 messages have been suppressed, use --verbose to show them.

* [DOC]

* [FIX] Support qualified variant selectors with matching? #83

* [CLEAN] debug prints

* [TEST] Decision matrix predicate test

* [DOC] Predicates and infix operators

* [FIX] Use peg instead of regex for example test

* [FIX] Duplicate assign for matches with conditions

* [CHECKPOINT]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants