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

enableif: simpler and more powerful alternative to concepts #12048

Closed
wants to merge 6 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Aug 26, 2019

this PR implements enableif for routine (generic proc/template/etc) specialization, known in other languages as:

int isdigit(int c) __attribute__((enable_if(c <= -1 || c > 255, "chosen when 'c' is out of range"))) __attribute__((unavailable("'c' must have the value of an unsigned char or EOF")));

eg usage:

proc fun[T1, T2](a1: T1, a2: T2): auto {.enableif: T1.sizeof == T2.sizeof .} = discard
template isFoo(a): untyped = type(s.a) is int and type(s.b) is float
proc fun[T](a: T, b: static string): auto {.enableif: isFoo(a) and b.len <2 .} = discard

Benefits compared to concepts / alternatives:

proc addInt(a, b: int): int {.since: (1, 1).} =
=>
proc addInt(a, b: int): int {.enableif: since(1, 1).} =
  • [EDIT] can be used to reduce indentation eg:
when not declared(addInt) and defined(builtinOverflow):
  proc addInt(a, b: int): int {.compilerproc, inline.} =
=>
proc addInt(a, b: int): int {.compilerproc, inline, enableif: not declared(addInt) and defined(builtinOverflow).} =
  • [EDIT] likewise, can be used do comment out a proc without modifying indentation, which shows large diffs in github UI and in cmdline git diff:
when false:
  proc addInt(a, b: int): int = ...
=>
proc addInt(a, b: int): int {.enableif: false.} = ...
  • allows writing arbitrary constraint on the whole proc signature. By contrast, concepts can only model single argument constraints; EDIT: eg where this matters: see std/lists: Various changes to lists (RFC #303) #16536 (comment)
  • allows accessing compile time value of static params
  • simpler to read/write for one-off constraints: instead of having to write a concept just for a constraint that appears once, you just write the expression in enableif; for constraints that are reused, you can just use a template (eg {.enableif: isFoo(T).})
  • nothing special (eg {.explain.}) is needed to debug a enableif expression: you can just insert echo statements inside the enableif expression, it'll execute as regular compile time code during sigmatch
  • does not mess argument types, eg:
  type Foo = concept a
    a.x
  proc fun(a: Foo) = echo type(a) # `Foo` instead of `B`
  type B = object
    x: int
  fun(B())

(and it gets worse when Foo[T] is needed, giving Foo[inferred[T], Foo])
wheres enableif: compiles(a.x) will just show B

concept issues avoided by enableif

There are many issues with concepts, see Concepts
These issues have prevented using concepts in the compiler source code. Eg see comments like this #9733 (comment):

My personal opinion is, don't use concepts, they currently cause more problems than they solve. But yes, this is bad.

I believe using enableif will solve most of those issues, while also enable use cases that aren't possible with concepts.

I tried porting a few examples from concept to enableif and the problem disappeared with enableif in every case.
Would be interesting to try other issues and see which are fixed and which are not. /cc @dawkot

  func foo(x: auto) {.enableif: type(x[0]) is type and type(x[1]) is type .} = discard
  foo((0, 'a'))
  template isFoo(c): untyped =
    for v in default(type(c)): (doAssert v is char)
    true
  func foo(c: auto) {.enableif: isFoo(c) .} =
    (for v in c: discard)
  foo @['a', 'b' ,'c']
  • etc, probably a lot more

notes

  • I say would close for those issues because, while merging this PR wouldn't fix the problem with concepts, i would provide a suitable alternative via enableif, and there's no point in keeping unfixable issues open forever if suitable workaround is made available.

  • bikeshedding: I could rename enableif to where: enableif is used in C++ but where is what's used in C#, swift, rust, haskell for the closest semantic thing; and a bit easier to type (and no guesswork enableif vs enableIf); on the other hand enableif is more unique/easier to search

  • add tests done

  • [EDIT]: see also Having problems with concepts that won't finish compiling - Nim forum : Having problems with concepts that won't finish compiling - Nim forum

@Clyybber
Copy link
Contributor

Why not just use when instead?

@krux02
Copy link
Contributor

krux02 commented Aug 26, 2019

Why not just use when instead?

Because it doesn't work.

@dom96
Copy link
Contributor

dom96 commented Aug 26, 2019

You should probably create an RFC before implementing something like this. Personally I am still in favour of changing concepts to use a more standard interface-like syntax (and extend them to work at runtime):

type
  Addition = concept
    proc `+`(x, y: Addition): Addition

I don't think these sort of ad-hoc solutions is what we need to make the language coherent.

@Araq
Copy link
Member

Araq commented Aug 26, 2019

I have an RFC in the works that is mostly like "generics need to be type-checked completely" (note: generics, not only their instantiations) and a mutation of concepts would allow us to do that, enableif would not. That would fix or mitigate the effective guesswork regarding symbol binding rules in generics.

@timotheecour
Copy link
Member Author

timotheecour commented Aug 27, 2019

You should probably create an RFC before implementing something like this. Personally I am still in favour of changing concepts to use a more standard interface-like syntax (and extend them to work at runtime):
type Addition = concept proc +(x, y: Addition): Addition

please write a separate RFC for your suggestion so we can follow-up there and compare apples to apples against this PR; IIUC, this doesn't cover the use cases of {.enableif.}, is much more complex than this PR to implement, and will have a similar list of gotchas/issues as the ones plaguing concepts (see list Concepts ). eg:

proc fun(x: auto){.enableif: compiles(x+x).} = echo x+x # or `overloadExists(x+x)` pending https://github.com/nim-lang/RFCs/issues/164
fun 2.3 # ok
type A = object
template `+`(a, b: A): auto = A()
fun A() # ok
fun 'a' # correctly gives Error: enableIf condition failed: 'compiles(x + x)'

whereas your suggestion would be more verbose (requires a separate type), and fail for fun A() since it requires proc, not template. The condition compiles(x+x) can be refined if needed to achieve exactly what library writer wants.

I don't think these sort of ad-hoc solutions is what we need to make the language coherent.

there's nothing ad-hoc about enableif, unlike concepts which introduces its own DSL (with lots of edge cases), it doesnt' need to introduce anything new inside the enableif clause: it's just a regular expression returning bool.

Furthermore, it's proven to work:

  • it's been there for a while in C++
  • it was the solution deliberately adopted in D over concepts, this solution is working very well in D and is one of its strongest features. In nim, it works even better because of ease of diagnostic of why a constrained returned false (you can insert echo in a enableif constraint, or even automate this using a macro to pinpoint the 1st condition that fails)
  • it's also used in other languages with generics

@Clyybber

Why not just use when instead?

as mentioned, that wouldn't work, however parser could be changed to support this instead of a pragma:

type T1 = int # irrelevant for `where` clause
where T1.sizeof == T2.sizeof: # T1 binds to generic inside `fun`, not to line above
  proc fun[T1, T2](a1: T1, a2: T2): auto = discard
  • pros: less noisy syntax (note: where is the keyword used for generic constraints in C#, swift, rust, haskell albeit with some differences/restrictions on what's in the where clause)
  • cons: as noted in that example, the scoping/binding is a bit suprising (but would be easy to learn), and introduces an extra indentation level

so I went for pragma instead.
(for reference, in D this looks like this: void fun(T1, T2)(T1 a1, T2 a2) if(T1.sizeof == T2.sizeof) {...})

@Varriount
Copy link
Contributor

Hm, but how does this replace the planned (albeit long delayed) feature of having concepts act like golang's interfaces (runtime polymorphism)?

@timotheecour
Copy link
Member Author

timotheecour commented Aug 27, 2019

Hm, but how does this replace the planned (albeit long delayed) feature of having concepts act like golang's interfaces (runtime polymorphism)?

you can build it as a library solution on top of {.enableif.} with macro sugar. Or you can simply do it directly:
here's this golang interface example https://gobyexample.com/interfaces ported to enableif:

type geometry interface {
    area() float64
    perim() float64
}
func measure(g geometry) {
    fmt.Println(g)
    fmt.Println(g.area())
    fmt.Println(g.perim())
}
  template geometry(a): untyped = (type(a.area) is float and type(a.perim) is float)
  proc measure(g: auto) {.enableif: geometry(g).} =
    echo type(g) # tuple[area: float64, perim: float64]
    echo g
    echo g.area
    echo g.perim
  measure((area: 1.1, perim: 1.2))

the point is, it's more flexible as it can consider whole signature, and it's much simpler as it just reuses existing expression evaluator.

here's with concepts:
it affects the type of g as shown in type(g):

  type geometry = concept a
    a.area is float
    a.perim is float
  proc measure(g: geometry) =
    echo type(g) # prints: `geometry` instead of `tuple[area: float64, perim: float64]`
    echo g
    echo g.area
    echo g.perim
  type T = tuple[area: float, perim: float]
  var t: T
  measure(t)

and this doesn't even work with concepts (yet another edge case: hits #9573):

  measure((area: 1.1, perim: 1.2)) # Error: no tuple type for constructor

@Varriount
Copy link
Contributor

That example doesn't appear to tackle runtime polymorphism, only compile time polymorphism.

What I meant was, there were plans to have concepts be used in a way similar to golang's Interfaces, so you could have (to take your example) a sequence of geometry objects that have varying implementations

@dawkot
Copy link

dawkot commented Aug 28, 2019

That example doesn't appear to tackle runtime polymorphism, only compile time polymorphism.

What I meant was, there were plans to have concepts be used in a way similar to golang's Interfaces, so you could have (to take your example) a sequence of geometry objects that have varying implementations

That's surprising. Interfaces can already be implemented using macros, and if you really wanted to integrate them with concepts, you could because macros can inspect contepts.

@timotheecour
Copy link
Member Author

timotheecour commented Aug 29, 2019

@Varriount

That example doesn't appear to tackle runtime polymorphism, only compile time polymorphism.
What I meant was, there were plans to have concepts be used in a way similar to golang's Interfaces, so you could have (to take your example) a sequence of geometry objects that have varying implementations

{.enableif.} is for (static) type constraints; for runtime polymorphism you don't need concepts nor type constraints, you can use the usual suspects:

createInterface(Geom):
  proc area(this: Geom): float
  proc perim(this: Geom): float
proc measure(a: seq[Geom]) = discard
measure(@[toGeom GeomCustom(area: 0.0, perim: 0.1), toGeom (area: 1.0, perim: 2.0)])
# or even use a `converter` to avoid the `toGeom` in some places, although I don't like converters too much

@timotheecour timotheecour force-pushed the pr_enableif branch 2 times, most recently from b55cf33 to 661392c Compare September 3, 2019 18:22
@timotheecour timotheecour marked this pull request as ready for review September 3, 2019 18:23
@timotheecour
Copy link
Member Author

timotheecour commented Sep 3, 2019

added tests, ready for review (test failures unrelated)

compiler/seminst.nim Outdated Show resolved Hide resolved
@andreaferretti
Copy link
Collaborator

I don't have a strong preference about enableif wrt concepts. But I surely think that one should not have both in the language. Nim is already complex as it is, let's not add more redundance. Concepts could be extended to be castable to runtime interfaces, and then one would have a single mechanism for both static and dynamic polymorphism

@timotheecour
Copy link
Member Author

But I surely think that one should not have both in the language

Concepts can be kept for backward compatibility but existing code relying on concepts can be migrated to enableif without loss of functionality, as enableif is more expressive than current concepts.

Concepts could be extended to be castable to runtime interfaces, and then one would have a single mechanism for both static and dynamic polymorphism

I'm not aware of a concrete proposal/RFC/PR to extend concepts to runtime interfaces (which, as mentioned, can already be done with library solutions anyways).

AFAIK concepts are plagued with fundamental issues that are hard/impossible to fix (eg type/value implicit conversion as mentioned in nim-lang/RFCs#13 or the many issues mentioned in Concepts that are so bad they prevent using concepts in compiler sources) . enableif by contrast integrates much better with the language by simply reusing existing expression evaluator.

@bluenote10
Copy link
Contributor

@timotheecour I see a few issues with enable_if that seem to work nicer with concepts:

  1. Naming: With concepts you start with a meaningful name like Iterable[T] that you give a bunch of properties. With enable_if you only have a bunch of properties and you cannot really give this "thing" a name. In the worst case this leads to duplicating the "iterable properties" for each proc. Using a template that contains all the conditions may help, but since it is optional to do so, reusability of enable_if conditions might still be a problem.
  2. Syntax: I prefer the name to appear directly in the signature like proc map(container: Iterable[T], ...) instead of having it somewhere down at the end of the signature in a pragma.
  3. Error messages: If an enable_if contains a large number of conditions that are simply a conjunction (not unusable for what would normally be a concept), is it possible to give meaningful error messages that make it clear why exactly a type T fails the enable_if check? I'd assume that for concepts it should be possible to report exactly that e.g. T is missing a T.len or so.

@dom96
Copy link
Contributor

dom96 commented Sep 4, 2019

Once again, I think this approach is fundamentally wrong. I strongly advocate for the solution described in my post above.

Can we close this and stop spending time on discussing it further?

@timotheecour
Copy link
Member Author

timotheecour commented Sep 4, 2019

I strongly advocate for the solution described in my post above.

Instead of closing this PR, if you strongly advocate for your solution you could open a RFC (or PR) to explain your alternative proposal in sufficient detail so there is something concrete to compare against (and maybe include in the description how it addresses the points in #12048 (comment) or how it'd fix the concept issues mentioned in top post that enableif avoids alltogether)

@bluenote10
Copy link
Contributor

@dom96 I don't see how you would express something like:

type
  Iterable*[T] = concept c
    for x in items(c): x is T
    for x in mitems(c): x is var T

  Indexable*[T] = concept c
    type IndexType = type(c.low)
    var i: IndexType

    c[i] is T
    c[i] is var T

    var value: T
    c[i] = value

    c.len is IndexType

In my opinion, concepts are so powerful mainly because they don't enforce a traditional interface syntax.

As long as the path forward isn't clear, we shouldn't cut the discussion short. The key question might be if it is actually possible to solve the exponential complexity issue of concepts.

@krux02
Copy link
Contributor

krux02 commented Sep 4, 2019

I don't think that this PR fixes any of the mentioned problems of concepts, because this is orthogonal to concepts. It is simply a different feature. When I then ignore the concepts issues that this PR claims to fix, there is no issue left that is actually fixed. It is just jet another complication of the Nim language. If you want this feature to be part of the language, we need a real use case for it. No, there should be multiple use cases. Something that does contain neith foo nor bar, otherwise I am not convinced that this is a useful addition to the Nim language.

And just because language X does have this complication, doesn't mean that we want to introduce this complication in Nim as well. Nim has a very different feature set than any other language out there.

@timotheecour
Copy link
Member Author

timotheecour commented Sep 5, 2019

@bluenote10

I don't see how you would express something like:

I'm assuming your comment was referring to "@dom96 's proposal", not enableif; in any case here's one way to do the same with enableif that works:

  proc Iterable(U: typedesc): bool =
    var c: U
    type T = type(items(c)) # or type T = type(for x in items(c): x)
    type(mitems(c)) is var T # or: (type(for x in mitems(c): x) is var T)

  proc Indexable(U: typedesc): bool =
    var c: U
    type IndexType = type(c.low)
    var i: IndexType
    type T = type(c[i])
    doAssert c[i] is T
    doAssert c[i] is var T
    var value: T
    doAssert compiles((c[i] = value))
    type(c.len) is IndexType

  proc fun[U](c: U){.enableif: Iterable(U).} =
    for x in items(c):
      echo x
  fun @[1,2,3]

  proc fun2[U](c: U){.enableif: Indexable(U).} =
    echo c[0]
  fun2 @[1,2]

note that your'e using is var T in Iterable/Indexable however, is var T is broken (regardless of concepts or enableif) as the var is ignored:

  const z = 1
  doAssert z is var int # passes

@timotheecour I see a few issues with enable_if that seem to work nicer with concepts:
Naming:

this is actually better with enableif: for one-off constraints, just put the constraint inline, which is less boilerplate than introducing a concept just for that. For re-used constraints, just use an auxiliary proc/template. Nothing complicated/unusual here.

Syntax:

pragma is part of the proc signature anyway (eg gcsafe etc), so IMO that's a subjective point; more importantly that's what enables constraints on whole signature

Error messages: If an enable_if contains a large number of conditions that are simply a conjunction (not unusable for what would normally be a concept), is it possible to give meaningful error messages that make it clear why exactly a type T fails the enable_if check? I'd assume that for concepts it should be possible to report exactly that e.g. T is missing a T.len or so.

this is a valid point. It can be done using a library solution via a macro debugIf {.enableif debugIf(Iterable(U)).} or as compiler patch / enhancement to enableif that would report for example the 1st doAssert failure inside the constraint.

@krux02

I don't think that this PR fixes any of the mentioned problems of concepts, because this is orthogonal to concepts. It is simply a different feature. When I then ignore the concepts issues that this PR claims to fix, there is no issue left that is actually fixed.

not sure I follow. Both enableif and concepts enforce type constraints, which allows in particular overloading by static properties (duck typing) or early failing (via proc signature), eg Indexable, Iterable as mentioned by others. As I mentioned in top-post:

I say would close for those issues because, while merging this PR wouldn't fix the problem with concepts, i would provide a suitable alternative via enableif [EDIT: that does not have that issue], and there's no point in keeping unfixable issues open forever if suitable workaround is made available.

@Araq
Copy link
Member

Araq commented Sep 5, 2019

If this works, you effectively reimplemented a variation of concepts with a different syntax:

  proc Indexable(U: typedesc): bool =
    var c: U
    type IndexType = type(c.low)
    var i: IndexType
    type T = type(c[i])
    doAssert c[i] is T
    doAssert c[i] is var T
    var value: T
    doAssert compiles((c[i] = value))
    type(c.len) is IndexType

  proc fun[U](c: U){.enableif: Iterable(U).} =
    for x in items(c):
      echo x
  fun @[1,2,3]

Can we use the enableif implementation ideas to implement concepts? (And then at a later step, fix concept's syntax...)

@zah
Copy link
Member

zah commented Sep 6, 2019

Unfortunately, this "simpler" alternative to concepts misses some of the key capabilities:

  1. A concept is able to infer type parameters (also known as generic concepts).

    Let's say we define a concept such as Container[T] and a function that uses it. If I pass a seq[int] to that function, the T parameter will be inferred to be int.

    This type inference is one of the more complicated features when implementing a concept system. The inferred types may appear as parameter types in the procs required by the concept. Then you can have dependencies between the inferred types and you have a full-blown constraint satisfaction algorithm to implement.

  2. Delivering good error messages from concepts is very tricky. The user needs to understand what are the exact requirements that are not being satisfied in a particular situation. In the long-term, we need some additional features in nimsuggest that will work like an interactive version of the current expain feature.

  3. The compiler needs to understand the concept better in order to be able to lift a matching polymorphic run-time type (see the VTable docs linked below). The same understanding is also necessary to implement a proper behavior for nimsuggest's code auto-completions and to get the eager type checking of concepts that Araq wants.

I'm not aware of a concrete proposal/RFC/PR to extend concepts to runtime interfaces (which, as mentioned, can already be done with library solutions anyways).

https://github.com/nim-lang/Nim/blame/1fb9a6d94631bf7f3570d3382874ba2d59e6ddbb/doc/manual_experimental.rst#L720-L773

Can we use the enableif implementation ideas to implement concepts? (And then at a later step, fix concept's syntax...)

No, because of the limitations mentioned in the first paragraph.

AFAIK concepts are plagued with fundamental issues that are hard/impossible to fix (eg type/value implicit conversion as mentioned in nim-lang/RFCs#13 or the many issues mentioned in Concepts that are so bad they prevent using concepts in compiler sources) . enableif by contrast integrates much better with the language by simply reusing existing expression evaluator.

Concepts are plagued by a lack of development resources - people who have enough time and willingness to dive in the existing code and improve it. There are no fundamental issues that are impossible to fix.

As long as the path forward isn't clear, we shouldn't cut the discussion short. The key question might be if it is actually possible to solve the exponential complexity issue of concepts.

It was always envisioned that all concepts will be backed by an instantiation cache just like generic procs and types. This is not particularly hard to do, but it hasn't been implemented yet and it should be the source of compilation slowdowns that people report.

@juancarlospaco
Copy link
Collaborator

This feels like Python Decorator-ish. 🤔

Like an anonymous JavaScript function on each proc will get long and would be hard to test/debug.
If it can be forced to be proc/func/template would be more readable.

@dom96
Copy link
Contributor

dom96 commented Sep 7, 2019

@bluenote10 easy, here is how:

type
  Iterable*[T] = concept
    iterator items(x: Iterable): T
    iterator mitems(x: Iterable): var T
  
  Indexable*[T, IndexType] = concept
    proc low(x: Indexable): IndexType
    proc `[]`(x: Indexable, i: IndexType): T
    proc `[]`(x: Indexable, i: IndexType): var T

    proc `[]=`(x: Indexable, i: IndexType, val: T)
    proc len(x: Indexable): IndexType

Am I missing something? This syntax is also far easier to understand as it doesn't require you to evaluate Nim code in your head to understand the concept spec. There are undoubtedly crazy things that concepts let you do, but I would argue that they are niche cases which don't deserve to be in a spec like this (as they will be far too complex to understand at a glance). With my syntax it's very trivial to see which procs/iterators/whatever are missing and the compiler will be able to give a very clear error message as well.

@bluenote10
Copy link
Contributor

x.len could either refer to a field, a proc, a template, or a macro. Even if we only allow proc, the actual signature could be different due to optional arguments, varargs, or discarded return values.

Also the point of the example was to show that with the current syntax it's optional to pull out a type into the generic. It can be inferred as well if it is not essential.

I personally prefer to read the concept code to see what I have to provide eventually, but I'm free to choose how exactly I implement it.

@dom96
Copy link
Contributor

dom96 commented Sep 7, 2019

@bluenote10

x.len could either refer to a field, a proc, a template, or a macro. Even if we only allow proc, the actual signature could be different due to optional arguments, varargs, or discarded return values.

Very good point. But I think this is something that can be solved by either softening what proc actually means in a concept or providing a callable keyword (@alehander42's suggestion).

Also the point of the example was to show that with the current syntax it's optional to pull out a type into the generic. It can be inferred as well if it is not essential.

Personally I think this is what makes the concepts syntax so bad. You'll be figuring out what each type evaluates to every time you try to understand a new concept. It's a good thing that this is disallowed.

I'm writing an RFC, will edit this post to link to the PR once I submit it. Let's discuss this further in there.

Edit: RFC -> nim-lang/RFCs#167

@bluenote10
Copy link
Contributor

I was thinking back of the PR that attempted to introduce Indexable in the standard library. Indexable[T] is simply much more convenient to work with than Indexable[T, IndexType] because it was a straightforward replacement of seq[T] and openarray[T].

@dom96
Copy link
Contributor

dom96 commented Sep 7, 2019

@bluenote10 for seq[T] and openarray[T] the IndexType would just be int, so it would be trivial.

@zah
Copy link
Member

zah commented Sep 7, 2019

The same arguments are repeated in every discussion about the syntax of concepts.

The proc declaration approach is a bit misleading, because you can implement a "proc" requirement with a template or a method.

As a minimum, a concept should also be able to check for the presence of associated types and constants, so you'll need to add additional specialized syntax for this as well. How would that look? Perhaps, something like this:

type C = concept
  const C.signatureSize: int
  type C.ElemType

Also, please write a concept requiring a type for which all of the fields are serializable. Or another one for a type that can be initialized from a value that's convertible to string? These seem like a reasonable requirements that may arise in the context of a generic library.

@timotheecour timotheecour force-pushed the pr_enableif branch 2 times, most recently from fdcd4f4 to be51e27 Compare September 8, 2019 16:35
@Araq
Copy link
Member

Araq commented Sep 9, 2019

And to complete the mess, here is my RFC nim-lang/RFCs#168

@Araq
Copy link
Member

Araq commented Apr 22, 2020

Sorry, rejected. Concepts are our way to type-check generics and enableif cannot do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment