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

Make IntSet a generic ordinal set PackedSet[A] + new module packedsets #15564

Merged
merged 6 commits into from
Nov 13, 2020

Conversation

landerlo
Copy link
Contributor

This makes intsets generic for ordinal types like enums or distinct of ints. For cases when multiple intsets with complex logic making them generic boosts their usability and type safety.
E.g. iqqn my use case I need to distinguish different ids making them distinct and get compilation errors if
I do any set operations with incompatible ids.

The PR achieves this maintaining full backwards compatibility and same performance.
As a drop in replacement, the current api is unchanged and IntSet is aliased to OrdSet[int].
OrdSet is for ordinal set. I realise there might be a confusion with ordered sets, so naming suggestions welcome.

Better error messages on compilation errors will be achieved when the concepts are ready, but I didn't want to tie it to a future roadmap, and the current solution achieves the stated goals satisfactorily IMHO.

lib/pure/collections/intsets.nim Outdated Show resolved Hide resolved
lib/pure/collections/intsets.nim Outdated Show resolved Hide resolved
@landerlo landerlo force-pushed the generic_intsets branch 4 times, most recently from 6d619a7 to a012d35 Compare October 14, 2020 03:36
@disruptek
Copy link
Contributor

I liked it better before it touched the compiler, but whatever. I think we have to wait until Ordinal is a concept before it will work for distinct types.

@timotheecour
Copy link
Member

timotheecour commented Oct 14, 2020

I liked it better before it touched the compiler, but whatever.

/cc @landerl
I agree with @disruptek , I don't think there's a need to change client code, see my answer here: #15564 (comment)

I think we have to wait until Ordinal is a concept before it will work for distinct types.

what do you mean? this passes:

type int2 = distinct int
doAssert int2 is Ordinal

@landerlo
Copy link
Contributor Author

landerlo commented Oct 14, 2020

I liked it better before it touched the compiler, but whatever.

/cc @landerl
I agree with @disruptek , I don't think there's a need to change client code, see my answer here: #15564 (comment)

I think we have to wait until Ordinal is a concept before it will work for distinct types.

what do you mean? this passes:

type int2 = distinct int
doAssert int2 is Ordinal

See my comment above, making all the type params A: Ordinal didn't work for me, but just making OrdSet[A: Ordinal] and the api procs [A] seems to work and has now better compilation errors, so yes, concepts are not necessary for this.

@landerlo landerlo force-pushed the generic_intsets branch 2 times, most recently from 78e65ee to 2987344 Compare October 14, 2020 13:33
@disruptek
Copy link
Contributor

I mean that Ordinal doesn’t capture a type than can nominally behave as an ordinal because it’s magic and not a concept.

@landerlo
Copy link
Contributor Author

landerlo commented Oct 14, 2020

I mean that Ordinal doesn’t capture a type than can nominally behave as an ordinal because it’s magic and not a concept.

At the moment is magic but there could be a concept modeling Ordinal with ord(a: A): A, fromOrd(o: int): A, and an implicit assumption that the constraint: forall x: int ord(fromOrd(int)) == x && forall a: A fromOrd(ord(a)) == a, that's left to an axiom for ints, enums and distincts of an ordinal. But maybe user could define other ordinal types to benefit from things like ordsets.

That Ordinal concept could be further generalised to Isomorphic. so concept Ordinal[A] = Isomorphic[A, int] with the above signature.

@timotheecour
Copy link
Member

LGTM; good to merge?

@timotheecour
Copy link
Member

timotheecour commented Nov 13, 2020

landerlo requested a review from timotheecour 16 minutes ago

I've already did a full review and gave my LGTM ...

@Araq or other nim member can i merge this?

@Araq
Copy link
Member

Araq commented Nov 13, 2020

Sorry, I thought it already was merged.

@Araq Araq merged commit c39fa0d into nim-lang:devel Nov 13, 2020
@landerlo
Copy link
Contributor Author

I do realise now that sed doesn't go through the commit messages, so the commit log still talks about OrdSet and not the renamed PackedSet. Nevermind.

@disruptek
Copy link
Contributor

Thanks for all your work on this, @landerlo -- I know it was a slog but this is a nice contribution. ❤️

@disruptek
Copy link
Contributor

disruptek commented Nov 18, 2020

I hate to say it, but this is causing the items iterator against an IntSet to fail:

https://github.com/disruptek/gram/runs/1419861309?check_suite_focus=true

/home/adavidoff/nims/lib/std/packedsets.nim(370, 14) Error: type mismatch: got <PackedSet[system.int]>
but expected one of: 
iterator items(a: cstring): char
  first type mismatch at position: 1
  required type for a: cstring
  but expression 's' is of type: PackedSet[system.int]
iterator items(a: string): char
  first type mismatch at position: 1
  required type for a: string
  but expression 's' is of type: PackedSet[system.int]
iterator items(n: NimNode): NimNode
  first type mismatch at position: 1
  required type for n: NimNode
  but expression 's' is of type: PackedSet[system.int]
iterator items[IX, T](a: array[IX, T]): T
  first type mismatch at position: 1
  required type for a: array[IX, T]
  but expression 's' is of type: PackedSet[system.int]
iterator items[N, E; F: static[GraphFlags]](graph: Graph[N, E, F]): N
  first type mismatch at position: 1
  required type for graph: Graph[items.N, items.E, items.F]
  but expression 's' is of type: PackedSet[system.int]
iterator items[T: char](a: openArray[T]): T
  first type mismatch at position: 1
  required type for a: openArray[T: char]
  but expression 's' is of type: PackedSet[system.int]
iterator items[T: enum](E: typedesc[T]): T
  first type mismatch at position: 1
  required type for E: type T: enum
  but expression 's' is of type: PackedSet[system.int]
iterator items[T: not char](a: openArray[T]): lent2 T
  first type mismatch at position: 1
  required type for a: openArray[T: not char]
  but expression 's' is of type: PackedSet[system.int]
iterator items[T](a: seq[T]): lent2 T
  first type mismatch at position: 1
  required type for a: seq[T]
  but expression 's' is of type: PackedSet[system.int]
iterator items[T](a: set[T]): T
  first type mismatch at position: 1
  required type for a: set[T]
  but expression 's' is of type: PackedSet[system.int]
iterator items[T](s: HSlice[T, T]): T
  first type mismatch at position: 1
  required type for s: HSlice[items.T, items.T]
  but expression 's' is of type: PackedSet[system.int]

expression: items(s)

@timotheecour
Copy link
Member

timotheecour commented Nov 18, 2020

@disruptek can you provide a minimized reproducible example?

@timotheecour timotheecour changed the title Make IntSet a generic ordinal set OrdSet[A] Make IntSet a generic ordinal set PackedSet[A] Nov 18, 2020
@timotheecour timotheecour changed the title Make IntSet a generic ordinal set PackedSet[A] Make IntSet a generic ordinal set PackedSet[A] + new module packedsets Nov 18, 2020
@disruptek
Copy link
Contributor

Is gram too complex for you?

@timotheecour
Copy link
Member

that's basically what everyone has to go through when submitting bug reports, providing minimized reproducing examples. More work for the reporter, less work for everyone else looking at it.

@disruptek
Copy link
Contributor

Yeah, but only if they didn't manage to get their package into important_packages first. If they were able to do that, then for some reason the developers are willing to do the same work themselves. Weird.

@timotheecour
Copy link
Member

timotheecour commented Nov 18, 2020

  • if its's a compiler bug: minimize with 0 stdlib module dependencies, when possible
  • if it's a stdlib bug: minimize with as few stdlib module dependencies, ideally just the module in question
    it's about minimizing total work = 1 * work(bug reporter) + N * work(people looking at issue)

@disruptek
Copy link
Contributor

Let's agree that you'll be the 1 to do the work and all N of us will reap the benefits.

@Araq
Copy link
Member

Araq commented Nov 19, 2020

As a compromise, both modules could expand a common template implementation -- then there cannot be regressions like this.

@landerlo
Copy link
Contributor Author

@disruptek Sorry for the regression. Tests test int iterators but there must be some issue why the iterator is not properly inferred.
I've created a PR that simply reverts IntSet to previous impl rather than as an alias for PackedSet[int], that should do the trick for now. I want to dig deeper into the issue anyway at some point.
Rollbak IntSet as before:
#16059

@disruptek
Copy link
Contributor

Can't we just fix it instead?

@timotheecour
Copy link
Member

timotheecour commented Nov 20, 2020

As a compromise, both modules could expand a common template implementation -- then there cannot be regressions like this.

=> #16060, much cleaner

Araq pushed a commit that referenced this pull request Nov 20, 2020
* packedsets fix regression introduced in #15564

* add tests
@disruptek
Copy link
Contributor

Nice work! Another good reason to be explicit with iterators, at least until this is fixed.

PMunch pushed a commit to PMunch/Nim that referenced this pull request Jan 6, 2021
* Make IntSet an ordinal set OrdSet[A: Ordinal]

Backward compatibility with IntSet is maintained.
IntSet is an alias for OrdSet[int]

* move ordsets to new file, intsets exports it
* ordset, move to lib/std folder

* Fix `$` for ordsets and test cleanup
* Fix ordsets compilation in doc example
* Rename ordsets to packedsets
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* Make IntSet an ordinal set OrdSet[A: Ordinal]

Backward compatibility with IntSet is maintained.
IntSet is an alias for OrdSet[int]

* move ordsets to new file, intsets exports it
* ordset, move to lib/std folder

* Fix `$` for ordsets and test cleanup
* Fix ordsets compilation in doc example
* Rename ordsets to packedsets
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* packedsets fix regression introduced in nim-lang#15564

* add tests
irdassis pushed a commit to irdassis/Nim that referenced this pull request Mar 16, 2021
* Make IntSet an ordinal set OrdSet[A: Ordinal]

Backward compatibility with IntSet is maintained.
IntSet is an alias for OrdSet[int]

* move ordsets to new file, intsets exports it
* ordset, move to lib/std folder

* Fix `$` for ordsets and test cleanup
* Fix ordsets compilation in doc example
* Rename ordsets to packedsets
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* Make IntSet an ordinal set OrdSet[A: Ordinal]

Backward compatibility with IntSet is maintained.
IntSet is an alias for OrdSet[int]

* move ordsets to new file, intsets exports it
* ordset, move to lib/std folder

* Fix `$` for ordsets and test cleanup
* Fix ordsets compilation in doc example
* Rename ordsets to packedsets
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* packedsets fix regression introduced in nim-lang#15564

* add tests
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 this pull request may close these issues.

4 participants