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

Imports don't resolve correctly when generics are involved. #22984

Closed
distantforest1 opened this issue Nov 25, 2023 · 4 comments · Fixed by #24315
Closed

Imports don't resolve correctly when generics are involved. #22984

distantforest1 opened this issue Nov 25, 2023 · 4 comments · Fixed by #24315

Comments

@distantforest1
Copy link

distantforest1 commented Nov 25, 2023

Description

It appears that nim doesn't resolve the imports correctly when generics are involved.

For example I have two files

  • main.nim
  • grouptest.nim

main.nim

import grouptest
# import sets # <<-- Uncomment this to make the error go away

## The generic implementation
var grp: EntGroup[Fruit] = initEntGroup[Fruit]()
echo get(grp) ## Errors here


## This works though (Non-generic)
var fruitGroup: FruitGroup = initFruitGroup()
echo getNoGeneric(fruitGroup)

grouptest.nim

import sets, hashes

type
  Fruit* = ref object
    id*: int

  # Generic implementation. This doesn't work
  EntGroup*[T] = ref object
    freed*: HashSet[T]

proc hash*(self: Fruit): Hash = hash(self.id)

##
## VVV The Generic implementation. This doesn't work VVV
##

proc initEntGroup*[T: Fruit](): EntGroup[T] =
  result = EntGroup[T]()
  result.freed = initHashSet[Fruit]()
  var apple = Fruit(id: 20)
  result.freed.incl(apple)

proc get*[T: Fruit](fg: EntGroup[T]): T =
  if len(fg.freed) == 0: return
  # vvv It errors here 
  # type mismatch: ([1] fg.freed: HashSet[grouptest.Fruit])
  for it in fg.freed: 
    return it

##
## VVV The Non-Generic implementation works VVV
##
type
  # Non-generic implementation. This works.
  FruitGroup* = ref object
    freed*: HashSet[Fruit]

proc initFruitGroup*(): FruitGroup =
  result = FruitGroup()
  result.freed = initHashSet[Fruit]()
  var apple = Fruit(id: 20)
  result.freed.incl(apple)

proc getNoGeneric*(fg: FruitGroup): Fruit =
  if len(fg.freed) == 0: return
  for it in fg.freed:
    return it

proc `$`*(self: Fruit): string = 
  # For echo
  if self == nil: return "Fruit()"
  return "Fruit(" & $(self.id) & ")"

Adding the import sets within the main.nim makes the error go away. So it seems that it is creating the get() proc within main.nim, rather than in grouptest.nim. If this is true, this might create some unforeseen for things such as libraries or 3rd party code.

I have the files zipped up here to make it easier to test
importbug.zip

Nim Version

Nim Compiler Version 2.0.0 [Windows: amd64]
Compiled at 2023-08-01
Copyright (c) 2006-2023 by Andreas Rumpf

active boot switches: -d:release

Current Output

D:\Code\nim\raylib\samples\bugs\importbug\grouptest.nim(14, 15) Error: type mismatch
Expression: items(fg.freed)
  [1] fg.freed: HashSet[grouptest.Fruit]

Expected one of (first mismatch at [position]):
[1] iterator items(a: cstring): char
[1] iterator items(a: string): char
[1] iterator items[IX, T](a: array[IX, T]): T
[1] iterator items[T: Ordinal](s: Slice[T]): T
[1] iterator items[T: char](a: openArray[T]): T
[1] iterator items[T: enum and Ordinal](E: typedesc[T]): T
[1] iterator items[T: not char](a: openArray[T]): lent2 T
[1] iterator items[T](a: seq[T]): lent2 T
[1] iterator items[T](a: set[T]): T

Expected Output

No error

Possible Solution

Not exactly sure what is causing the issue, but as mentioned earlier it seems that it's creating the get() proc within main.nim, rather than in grouptest.nim. It would be best to create the proc at its original location rather than when it was first called (in this case it was called in main.nim)
.

Additional Information

No response

@beef331
Copy link
Collaborator

beef331 commented Nov 25, 2023

Implicit items is broken inside generics do for a in b.items for a workaround.

@distantforest1
Copy link
Author

ok thanks

@mratsim
Copy link
Collaborator

mratsim commented Dec 22, 2023

@beef331
Copy link
Collaborator

beef331 commented Dec 22, 2023

This is actually not a generic sandwich issue, it's just implicit items being broken with generics. Due to how implicit iterators are resolved it never looks up the bounded symbols. So even a mixin items does not resolve the issue with it.

@Araq Araq closed this as completed in 2864830 Oct 25, 2024
narimiran pushed a commit that referenced this issue Jan 14, 2025
closes nim-lang/RFCs#380, fixes #4773, fixes
#14729, fixes #16755, fixes #18150, fixes #22984, refs #11167 (only some
comments fixed), refs #12620 (needs tiny workaround)

The compiler gains a concept of root "nominal" types (i.e. objects,
enums, distincts, direct `Foo = ref object`s, generic versions of all of
these). Exported top-level routines in the same module as the nominal
types that their parameter types derive from (i.e. with
`var`/`sink`/`typedesc`/generic constraints) are considered attached to
the respective type, as the RFC states. This happens for every argument
regardless of placement.

When a call is overloaded and overload matching starts, for all
arguments in the call that already have a type, we add any operation
with the same name in the scope of the root nominal type of each
argument (if it exists) to the overload match. This also happens as
arguments gradually get typed after every overload match. This restricts
the considered overloads to ones attached to the given arguments, as
well as preventing `untyped` arguments from being forcefully typed due
to unrelated overloads. There are some caveats:

* If no overloads with a name are in scope, type bound ops are not
triggered, i.e. if `foo` is not declared, `foo(x)` will not consider a
type bound op for `x`.
* If overloads in scope do not have enough parameters up to the argument
which needs its type bound op considered, then type bound ops are also
not added. For example, if only `foo()` is in scope, `foo(x)` will not
consider a type bound op for `x`.

In the cases of "generic interfaces" like `hash`, `$`, `items` etc. this
is not really a problem since any code using it will have at least one
typed overload imported. For arbitrary versions of these though, as in
the test case for #12620, a workaround is to declare a temporary
"template" overload that never matches:

```nim
# neither have to be exported, just needed for any use of `foo`:
type Placeholder = object
proc foo(_: Placeholder) = discard
```

I don't know what a "proper" version of this could be, maybe something
to do with the new concepts.

Possible directions:

A limitation with the proposal is that parameters like `a: ref Foo` are
not attached to any type, even if `Foo` is nominal. Fixing this for just
`ptr`/`ref` would be a special case, parameters like `seq[Foo]` would
still not be attached to `Foo`. We could also skip any *structural* type
but this could produce more than one nominal type, i.e. `(Foo, Bar)`
(not that this is hard to implement, it just might be unexpected).

Converters do not use type bound ops, they still need to be in scope to
implicitly convert. But maybe they could also participate in the nominal
type consideration: if `Generic[T] = distinct T` has a converter to `T`,
both `Generic` and `T` can be considered as nominal roots.

The other restriction in the proposal, being in the same scope as the
nominal type, could maybe be worked around by explicitly attaching to
the type, i.e.: `proc foo(x: T) {.attach: T.}`, similar to class
extensions in newer OOP languages. The given type `T` needs to be
obtainable from the type of the given argument `x` however, i.e.
something like `proc foo(x: ref T) {.attach: T.}` doesn't work to fix
the `ref` issue since the compiler never obtains `T` from a given `ref
T` argument. Edit: Since the module is queried now, this is likely not
possible.

---------

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
(cherry picked from commit 2864830)
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

Successfully merging a pull request may close this issue.

3 participants