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

Exporting a toSeq overload messes up iterator visibility #512

Open
bluenote10 opened this issue Mar 11, 2018 · 10 comments
Open

Exporting a toSeq overload messes up iterator visibility #512

bluenote10 opened this issue Mar 11, 2018 · 10 comments

Comments

@bluenote10
Copy link

bluenote10 commented Mar 11, 2018

# file: main.nim
import sequtils
import tables

# this import messes up the functionality of table iterators
import b 

let t = initOrderedTable[int, int]()
let s = toSeq(keys(t))
echo s
# file: b.nim
type
  Data*[T] = object
    data*: seq[T]

proc toSeq*[T](c: Data[T]): seq[T] =
  c.data

Compiling main.nim results in: Error: attempting to call undeclared routine: 'keys'. Basically none of the iterators from tables can be used in a toSeq context when another module (accidentally) exports an overload of toSeq.

@Araq
Copy link
Member

Araq commented Mar 12, 2018

That's because overloaded procs need to have the same untyped parameters in the same positions. Documented limitation. sequtils.toSeq always works then. Don't "accidentically export" a toSeq.

@Araq Araq closed this as completed Mar 12, 2018
@bluenote10
Copy link
Author

bluenote10 commented Mar 12, 2018

Having a toSeq function is the natural choice for any data structure which support "to seq conversion", so this is a rather nasty pitfall. In fact I have been using toSeq functions in several projects of mine and I always though toSeq + iterator visibility is just broken in general because I never could do something like toSeq(key(t)) anywhere. It was only by chance to find the cause...

Wouldn't it be possible to solve the issue by avoiding an untyped implementation of toSeq in sequtils and explicitly implement it for iterators and sized collections (maybe by introducing a concept).

Also I struggle to find the documented limitation and I still can't make sense of the error message here. Apparently the overloading resolution seems to pick the right version of toSeq here, otherwise I would expect a different error. But why does the compiler complain with attempting to call an "undeclared" routine, which obviously is declared?

@Araq
Copy link
Member

Araq commented Mar 13, 2018

Wouldn't it be possible to solve the issue by avoiding an untyped implementation of toSeq in sequtils and explicitly implement it for iterators and sized collections (maybe by introducing a concept).

Certainly.

Apparently the overloading resolution seems to pick the right version of toSeq here, otherwise I would expect a different error. But why does the compiler complain with attempting to call an "undeclared" routine, which obviously is declared?

Because it doesn't pick up the right overload.

@bluenote10
Copy link
Author

bluenote10 commented Mar 17, 2018

Certainly.

Then we should probably go for a typed solution in the stdlib to avoid the issue. If you don't mind I'll reopen...

Edit: Oh haha, I cannot re-open :).

@Araq Araq reopened this Mar 18, 2018
@bluenote10
Copy link
Author

bluenote10 commented Mar 18, 2018

In fact even the standard library violates the "don't accidentally export a toSeq" for instance in nre, i.e. this also fails to compile:

import sequtils, tables, nre

let t = initOrderedTable[int, int]()
let s = toSeq(keys(t))

I'm afraid the (current?) limitations of inline iterators make a proper solution very tough :-(.

@Araq
Copy link
Member

Araq commented Mar 18, 2018

In fact even the standard library violates the "don't accidentally export a toSeq" for instance in nre, i.e. this also fails to compile

I know.

@timotheecour
Copy link
Member

just hit this bug again; import nre messes up toSeq.

Wouldn't it be possible to solve the issue by avoiding an untyped implementation of toSeq in sequtils and explicitly implement it for iterators and sized collections (maybe by introducing a concept).

Certainly.

no, unfortunately with the way iterators are implemented in the compiler today, I don't think it's possible to have a toSeq(a: typed).
See also nim-lang/Nim#9219

@andreaferretti
Copy link

I don't think it's possible to have a toSeq(a: typed)

I think the proposal is to have various overloads of toSeq - one for iterators and the other ones for specific collections. As far as I can tell, this should work, at the cost of some repetition

@stale
Copy link

stale bot commented Aug 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. If you think it is still a valid issue, write a comment below; otherwise it will be closed. Thank you for your contributions.

@timotheecour
Copy link
Member

timotheecour commented Dec 21, 2020

I think the proposal is to have various overloads of toSeq - one for iterators and the other ones for specific collections. As far as I can tell, this should work, at the cost of some repetition

i don't see how this can work without a compiler change; the problem is the toSeq overload that takes an untyped param to accomodate for any iterable, eg foo(3) below.

just hit this bug again; import nre messes up toSeq.

for reference, the bug is when toSeq requires an untyped param:

when true:
  import sequtils
  # import nre # uncomment would give: Error: attempting to call routine: 'foo'
  iterator foo(n: int): int =
    for i in 0..<n: yield i
  echo toSeq(1..2)
  echo toSeq([1,2])
  echo toSeq(foo(3)) # this is the case broken by `import nre`

If you love sequtils.toSeq we have bad news for you. This library doesn't work with it due to documented compiler limitations. As a workaround, use this:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants