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

toSet(iter, typedesc) overload, allows converting between set types #18397

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jun 30, 2021

follows #18388

example

var s1: set['a'..'z'] = {'a', 'c'}
var s2: set[char] = {'a', 'b'}
# let s = s1 + s2 # now a CT error (as it should) now that https://github.com/nim-lang/Nim/issues/16270 was fixed
let s = s1 + s2.toSet(typeof(s1)) # ok

@timotheecour timotheecour marked this pull request as ready for review June 30, 2021 02:56
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jun 30, 2021
changelog.md Outdated Show resolved Hide resolved
@@ -21,20 +21,32 @@ import typetraits, macros
## The allowed types of a built-in set.
]#

template toSet*(iter: untyped): untyped =
template toSet*[T: set](iter: untyped, _: typedesc[T]): T =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add a generic parameter and a typedesc parameter? Can't you write s.toSet[:T]? And what about situations where you don't need to explicitly say the type, since it can be inferred (or is Nim's type inference not good enough for that)?

Copy link
Member Author

@timotheecour timotheecour Jul 1, 2021

Choose a reason for hiding this comment

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

Why add a generic parameter and a typedesc parameter?

this was covered in nim-lang/RFCs#40; as @zah summarized in nim-lang/RFCs#40 (comment) which explained the rationale:

  1. Use generic types for generic value params, where the types will be inferred from the call-site.
  2. Use typedesc for all params that must be explicitly specified by the user.

There are plenty of arguments for this; besides the ones in nim-lang/RFCs#40 (comment) here are some more:

  • overloading requires typedesc
  • macro fn(T: typedesc): untyped works whereas macro fn[T]() can't do anything useful
  • explicit generic instantiation requires the uglier s.toSet[:T] syntax for MCS and fully qualified calls

most of stdlib now uses this rule (apart from old APIs predate typedesc and some rare exceptions that slipped by code review), eg:

  • this module setutils (see fullSet), typetraits, jsonutils, json, options, jsbigints, enumutils, times, monotimes, complex, fenv, importutils, assertions, fenv, random ...; and most of system (eg system.default,sizeof,low,new ...)

And what about situations where you don't need to explicitly say the type

it also works, assert "abc".toSet == {'a', 'b', 'c'}

@Varriount
Copy link
Contributor

Is there a reason a type conversion can't be used (aside from fixable implementation issues)?

For some reason the compiler doesn't produce the correct conversion code, but I don't understand why

var s1: set['a'..'z'] = {'a', 'c'}
var s2: set[char] = {'a', 'b'}
let s = s1 + set['a'..'z'](s2)
echo s

isn't suitable in theory, if not practice.

@timotheecour timotheecour force-pushed the pr_setutils_toSet_typedesc_refs_16270 branch from 64b3bd6 to b831458 Compare July 1, 2021 01:06
@timotheecour
Copy link
Member Author

Is there a reason a type conversion can't be used (aside from fixable implementation issues)?

regardless of whether this is implemented in the future (which would require patching vm, js, c backends), this feature is useful in itself beyond type conversion, eg: "abc".toSet(set['a'..'z']) which avoids having to do "abc".toSet.set['a'..'z'] which would be less efficient. I've added that in runnableExamples.

@Varriount
Copy link
Contributor

Is there a reason a type conversion can't be used (aside from fixable implementation issues)?

regardless of whether this is implemented in the future (which would require patching vm, js, c backends), this feature is useful in itself beyond type conversion, eg: "abc".toSet(set['a'..'z']) which avoids having to do "abc".toSet.set['a'..'z'] which would be less efficient. I've added that in runnableExamples.

How is it less efficient?

@timotheecour
Copy link
Member Author

How is it less efficient?

"abc".toSet and "abc".toSet.set['a'..'z'] have different binary representations, it's not a simple cast in the type system

@Araq
Copy link
Member

Araq commented Jul 1, 2021

Is there a reason a type conversion can't be used (aside from fixable implementation issues)?

It's an unsound typing rule. If T converts to T2 that doesn't mean that Container[T] converts to Container[T2].

We currently only allow if for backwards compatibility and probably I'll fix it properly soon.

@Varriount
Copy link
Contributor

Does the implementation need to handle/currently handle situations similar to what toSeq has to handle?

@timotheecour
Copy link
Member Author

it handles iterables, yes

@Araq
Copy link
Member

Araq commented Jul 3, 2021

It's quite an alien API with the typedesc. typedesc is used in other places, yes, but not when it produces unintuitive results.

@timotheecour
Copy link
Member Author

timotheecour commented Jul 3, 2021

It's quite an alien API with the typedesc. typedesc is used in other places, yes, but not when it produces unintuitive results.

I don't follow. It's the exact same with other APIs, there's nothing un-intuitive about it:

import std/[json, jsonutils, setutils]
let j1 = @[1,2].toJson
let j2 = {'a', 'b'}.toJson
let j3 = {'a', 'b'}
when defined(js):
  import std/jsffi
  let j4 = {'a', 'b'}.toJs

# and now use those with typedesc:
echo j1.to(seq[int])
echo j1.jsonTo(seq[int])
echo j2.jsonTo(set['a'..'z'])

when defined(js):
  echo j4.to(set['a'..'z']) # jsffi.to

# this PR:
echo j3.toSet(set['a'..'z'])

plus other instances where the type is not inferred from other parameters.

The rules from nim-lang/RFCs#40 (comment) are simple and intuitive, most of stdlib (after typedesc was introduced) follow those, or should:

  • Use generic types for generic value params, where the types will be inferred from the call-site.
  • Use typedesc for all params that must be explicitly specified by the user.

@timotheecour
Copy link
Member Author

ping @Araq

@stale
Copy link

stale bot commented Jul 30, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Jul 30, 2022
@stale stale bot closed this Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants