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

iterable[T] #17196

Merged
merged 22 commits into from
Apr 11, 2021
Merged

iterable[T] #17196

merged 22 commits into from
Apr 11, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Feb 27, 2021

  • someIter(args) is now well typed: iterable[T] instead of untyped
  • solves many long standing issues related to iterators
  • sigmatch error messages show iterable[T] for iterables, which is more informative
  • procs like toSeq, elementType etc can now be defined without untyped
  • all (or almost all?) procs in sequtils can now be made more general, accepting also iterable inputs instead of openArray
  • iterable arguments can now be used in UFCS/MCS, eg iota(3).toSeq now works

see tests

will fix these

nim-lang/RFCs#512
#9219
#13595
#14148
#16545
and sidestep #14778
and reduce cases where we hit #14827

note

this will make it much easier to implement online algorithms, eg summation functions should be usable in online fashion, more general than openArray · Issue nim-lang/Nim#288 · nim-lang/RFCs, avoiding issues related to untyped, and enabling UFCS/MCS

future work

@timotheecour timotheecour changed the title WIP iterable[T] Feb 27, 2021
@timotheecour timotheecour marked this pull request as ready for review February 27, 2021 08:55
@konsumlamm
Copy link
Contributor

What exactly is iterable[T]? Is it a builtin type class, like Ordinal[T]? If so, why use a lowercase name and not Iterable[T]?

And what exactly does iterable[T] encompass? All iterators with a "yield type" of T? Something else (seq[T], openArray[T], everything with an iter proc)?

Also, is there any previous discussion of this?

@timotheecour
Copy link
Member Author

timotheecour commented Feb 27, 2021

What exactly is iterable[T]? Is it a builtin type class, like Ordinal[T]? If so, why use a lowercase name and not Iterable[T]?

It's a type class; most other builtin type classes are lowercase.
This helps distinguish user or library defined types from builtin types that require compiler magic. Note that SomeFloat can be consider non-builtin even though it's in system, as it doesnt' require magic.

And what exactly does iterable[T] encompass? All iterators with a "yield type" of T? Something else (seq[T], openArray[T], everything with an iter proc)?

all calls to an iterator (inline or closure):

when true:
  template fn(a: iterable) = discard
  iterator iota(n: int): auto = (for i in 0..<n: yield i)
  fn(iota(3)) # ok
  fn(items(@[1,2])) # ok
  # fn(@[1,2]) # error

by design fn(@[1,2]) would give CT error:
here's the error msg:

t11921.nim(314, 5) Error: type mismatch: got <seq[int]>
but expected one of:
template fn(a: iterable) [template declared in t11921.nim(310, 12)]
  first type mismatch at position: 1
  required type for a: iterable [iterable declared in /Users/timothee/git_clone/nim/Nim_prs/lib/system.nim(128, 5)]
  but expression '@[1, 2]' is of type: seq[int] [sequence]

Also, is there any previous discussion of this?

I had an RFC in my nim fork (timotheecour#322)

@juancarlospaco
Copy link
Collaborator

juancarlospaco commented Feb 28, 2021

changelog ?.

EDIT(timotheecour): done

@saem
Copy link
Contributor

saem commented Mar 1, 2021

Apologies, if my questions are rather obvious, I'm trying to understand this:

  • there are a number of skipTypes in various parts of sem that are required to get at the underlying type, my mind could be playing tricks on me but is the testing sufficient because it feels like there should be more updates for those?
  • for iterable itself, shouldn't there be an underlying concept of iterable such that everything unifies, maybe I missed that?

@timotheecour
Copy link
Member Author

there are a number of skipTypes in various parts of sem that are required to get at the underlying type, my mind could be playing tricks on me but is the testing sufficient because it feels like there should be more updates for those?

my understanding is these only concerned typed values, which iterable isn't part of; this doesn't constitute a bug though; this PR comes with tests, if we find something that doesn't work, we can always fix it in subsequent PR.

for iterable itself, shouldn't there be an underlying concept of iterable such that everything unifies, maybe I missed that?

this could be done in future work, but is not strictly necessary, and the issues mentioned in PR body can be fixed without such unification, which has its own can of worms. iterable[T] is in principle all you need, you can always pass a called iterator instead of passing a value that has an items defined for it, eg:

# instead of:
foo(@[1,2])
# use:
foo(items(@[1,2])

but as I showed in this PR, you can have an overload that take iterable and an overload that takes a typed value (possible taking a Itemable[T] concept which would check presence of a suitable items iterator defined on it).

@timotheecour timotheecour requested a review from ringabout March 10, 2021 07:32
@Araq
Copy link
Member

Araq commented Mar 10, 2021

How does this solution compare to the (to me) much more obvious tyIter (modelled like tyProc) solution?

@timotheecour
Copy link
Member Author

tyProc is for a proc type, not for a called proc; if you model tyIter like tyProc, this would match an iterator type, not a called iterator, which is what we want here. eg:

iterator iota(n: int): int = (for i in 0..<n: yield i)
assert toSeq(iota(3)) == @[0,1,2]

after this PR, this can work with template toSeq[T](a: iterable[T]): seq[T] instead of requiring untyped param. Furthmore, MCS works. I'm not sure how tyIter would help, or at least this would need more details.

compiler/typeallowed.nim Outdated Show resolved Hide resolved
@konsumlamm
Copy link
Contributor

Any update on this? I think this would be a very useful addition.

If I understand correctly, this should allow us to massively simplify enumerate by turning it into an iterator:

iterator enumerate[T](iter: iterable[T]): (int, T) =
  var i = 0
  for x in iter:
    yield (i, x)
    inc i

If this would not be possible, could this be achieved in a similar way?

@timotheecour timotheecour force-pushed the pr_iterable branch 2 times, most recently from 54a950d to a7efee2 Compare April 8, 2021 22:34
@timotheecour
Copy link
Member Author

@Araq PTAL; I will update spec in followup PR

@konsumlamm

Any update on this? I think this would be a very useful addition.

I've rebased to fix bitrot conflicts and fixed the case of proc fn(a: iterable[int]) = discard so that it errors in typeAllowedAux instead of later. This PR is ready to go again.

If I understand correctly, this should allow us to massively simplify enumerate by turning it into an iterator:

this requires #11992 (see tests/magics/tlambda_iterators.nim which has such examples) but would become more typesafe and easier to use with this PR (because of iterable instead of untyped). This currently works with #11992:

when true:
  import std/lambdas
  {.push experimental:"alias".}
  iterator enumerate(iter: aliassym): (int, char) =
    # `auto` or (int, typeof(iter)) will also be possible in future PRs
    var i = 0
    for x in iter:
      yield (i, x)
      inc i
  for i, ai in enumerate(lambdaIter {'a', 'b', 'c'}):
    echo (i, ai)

@Araq
Copy link
Member

Araq commented Apr 9, 2021

I will update spec in followup PR

Please do it in this PR, I now understand why and how you do it so I could write the spec myself. However, I want to see if my ideas agree with yours.

@konsumlamm
Copy link
Contributor

konsumlamm commented Apr 9, 2021

@timotheecour

What about something like this?

template enumerate[T](iter: iterable[T]): iterable[(int, T)] =
  (iterator (): (int, T) =
    var i = 0
    for x in iter:
      yield (i, x)
      inc i)()

I experimented a bit, but couldn't get it to work. Also, when using untyped as return type in the above, i get an internal error on instantiation:

Error: internal error: expr(nkVarTuple); unknown node kind

EDIT: It seems like the error is unrelated to this PR, also the iterator seems to be transformed to a proc...

@timotheecour timotheecour force-pushed the pr_iterable branch 2 times, most recently from cb0880f to 09f3fbe Compare April 9, 2021 20:24
@timotheecour
Copy link
Member Author

@Araq

Please do it in this PR, I now understand why and how you do it so I could write the spec myself. However, I want to see if my ideas agree with yours.

done, PTAL

@konsumlamm this should be defered to future work, again, see tests/magics/tlambda_iterators.nim from #11992

doc/manual.rst Show resolved Hide resolved
if efWantIterable in flags:
let typ = newTypeS(tyIterable, c)
rawAddSon(typ, result.typ)
result.typ = typ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problematic. See my final comment in the review.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replied in #17196 (comment)

@Araq
Copy link
Member

Araq commented Apr 10, 2021

The solution is quite good, however: Imagine a typed macro that takes someIter(args) and then does type introspection of the typed expression. Does that now produce the new tyIterator type? If so, that is a minor breaking change. If not, well maybe it should... :-)

@timotheecour
Copy link
Member Author

timotheecour commented Apr 10, 2021

Imagine a typed macro that takes someIter(args) and then does type introspection of the typed expression. Does that now produce the new tyIterator type?

macro fn(a: typed) = discard
fn(items(3..6))

produces, both before and after this PR:

Error: attempting to call routine: 'items'
found ...

Future work can decide whether typed should accept an iterable (it can make sense, given that typed also accepts statements, eg x.inc), or if we instead want to support typed or iterable.

If so, that is a minor breaking change

what do you mean? if/when we make typed accept iterable, it would turn a CT error into working code

@Araq Araq merged commit ceadf54 into nim-lang:devel Apr 11, 2021
@Araq
Copy link
Member

Araq commented Apr 11, 2021

produces, both before and after this PR

Thanks, was not aware. That answers my question.

@timotheecour timotheecour deleted the pr_iterable branch April 11, 2021 15:47
@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Apr 11, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* fix failing test toSeq in manual which now works
* changelog
* reject proc fn(a: iterable)
* add iterable to spec
* remove MCS/UFCS limitation that now works
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants