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

Generic instantiation change breaks recCase #51

Closed
Gruruya opened this issue Sep 5, 2023 · 4 comments
Closed

Generic instantiation change breaks recCase #51

Gruruya opened this issue Sep 5, 2023 · 4 comments
Labels
bug Something isn't working/Fixes for a bug (PR)

Comments

@Gruruya
Copy link
Contributor

Gruruya commented Sep 5, 2023

With nim-lang/Nim#22642 this:

ObjectTy
  Empty
  Sym "Union"
  RecList
    RecCase
      Sym "type"
      OfBranch
        IntLit 0
        Sym "field0"
      OfBranch
        IntLit 1
        Sym "field1"

becomes:

NilLit

Tests now error. The error is:

union/union/uniontraits.nim(116, 33) Error: cannot get child of node kind: nnkNilLit

Error points to this:

union/union/uniontraits.nim

Lines 104 to 106 in a5b6d31

proc recCase(u: UnionTy): NimNode =
## Returns the case part of the object declaration.
u[2].last

An example of where the error occurs (first usage of as):

union/tests/treadme.nim

Lines 6 to 15 in a5b6d31

proc search[T, U](x: T, needle: U): union(U | None) =
# Assignment can be done via explicit conversion
result = None() as union(U | None)
let idx = find(x, needle)
if idx >= 0:
# Or the `<-` operator which automatically converts the type
result <- x[idx]
doAssert [1, 2, 42, 20, 1000].search(10) of None

The PR added getObjectSym:

proc getObjectSym(t: PType): PSym =
  if tfFromGeneric in t.flags and t.typeInst.kind == tyGenericInst:
    var dummy: SkippedPtr
    result = t.typeInst[0].skipToObject(dummy).sym
  else:
    result = t.sym

I'm guessing this usage of sameType needs to be updated:

let typ = getTypeSkip(n)
if typ.kind == nnkObjectTy:
# Verify that `n` inherits from Union.
if typ.inherits.sameType(bindSym"Union"):
UnionTy(typ)
else:
UnionTy(nil)

But I don't know how.

@Gruruya Gruruya changed the title recCase erroring, NilLit Generic instantiation change breaks recCase Sep 5, 2023
@metagn
Copy link

metagn commented Sep 5, 2023

Nim change will probably be reverted

Tested treadme with nim-lang/Nim#22652 and it works

metagn added a commit to metagn/Nim that referenced this issue Sep 5, 2023
@alaviss alaviss added the bug Something isn't working/Fixes for a bug (PR) label Sep 5, 2023
@alaviss
Copy link
Owner

alaviss commented Sep 5, 2023

Looks like my little hack to see if T is inherited from an instance of a generic type G finally failed :P

I think I have a solution for this, though.

alaviss added a commit that referenced this issue Sep 5, 2023
Previously we performed tests against Union symbol itself to check
for inheritance.

This relied on an implementation detail where an instance of a generic
has the same type as the generic type itself.

Recent changes in the compiler shows that we cannot rely on this. Thus
the module now opt for inheriting from and testing against a pre-defined
instance of Union.

Ref nim-lang/Nim#22642
Fixes #51
@alaviss
Copy link
Owner

alaviss commented Sep 5, 2023

Apparently I misremembered how this is done.

As described in nim-lang/Nim#22642, old Nim uses the generic type symbol for AST, which is why I had to check directly against the generic symbol.

@alaviss alaviss closed this as completed in cce72a5 Sep 5, 2023
metagn added a commit to nim-lang/Nim that referenced this issue Sep 6, 2023
reverts #22642, reopens #22639, closes #22646, refs #22650, refs
alaviss/union#51, refs #22652

The fallout is too much from #22642, we can come back to it if we can
account for all the affected code.
@metagn
Copy link

metagn commented Sep 6, 2023

This would have also been a problem with nim-lang/Nim#22655, thanks for fixing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working/Fixes for a bug (PR)
Projects
None yet
Development

No branches or pull requests

3 participants