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

resolveSymbol(foo(args)) and overloadExists(foo(args)) to return symbol after overload resolution #12076

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Aug 28, 2019

  • fixes overloadExists(foo(args)) to tell whether an overload exists without compiling/running body via compiles RFCs#164 (both proposal1 and proposal2)
  • a single magic is added resolveSymbol that performs symbol resolution (in particular overload resolution for call-like expressions) and returns either nil if not such overload was found, or the symbol after overload resolution is done, or the symchoice if a symbol is ambiguous.
  • overloadExists(foo(args)) is a library proc built on top of resolveSymbol to tell whether a proper overload exists
  • extensive tests are provided in tests/magics/tresolve_overloads.nim

As an example use case, see library defined inspect macro that inspects the sym/symchoice returned by resolveSymbol:

inspect resolveSymbol(`$`)
/Users/timothee/git_clone/nim/Nim_prs/tests/magics/tresolve_overloads.nim:134:35: $ = closedSymChoice:
  /Users/timothee/git_clone/nim/Nim_prs/lib/system/dollars.nim:10:1 proc `$`(x: float): string {.magic: "FloatToStr", noSideEffect.}
  /Users/timothee/git_clone/nim/Nim_prs/lib/system/dollars.nim:128:1 proc `$`[T](x: seq[T]): string
  /Users/timothee/git_clone/nim/Nim_prs/lib/system/strmantle.nim:307:1 proc `$`(x: uint64): string {.noSideEffect, raises: [].}
  /Users/timothee/git_clone/nim/Nim_prs/lib/system/dollars.nim:152:1 proc `$`[T](x: openArray[T]): string
  /Users/timothee/git_clone/nim/Nim_prs/lib/system/dollars.nim:34:1 proc `$`[Enum](x: Enum): string {.magic: "EnumToStr", noSideEffect.}
  /Users/timothee/git_clone/nim/Nim_prs/lib/system/widestrs.nim:172:1 proc `$`(w: WideCString; estimate: int; replacement: int = 0x0000FFFD): string
etc...
  • you can also resolve a proc overload via its arguments, this is quite useful when you don't know which overload is used in some piece of code. eg:
inspect resolveSymbol(newLit(true))
/Users/timothee/git_clone/nim/Nim_prs/tests/magics/tresolve_overloads.nim:151:27: newLit = /Users/timothee/git_clone/nim/Nim_prs/lib/core/macros.nim:702:1 proc newLit(b: bool): NimNode {.compileTime.}

let t = resolveSymbol toUpper("asdf")
inspect resolveSymbol(t), resolveLet=true
echo t("foo")

inspect is a bit similar to what nimsuggest binary provides but it's a pure library solution, ie easier to integrate with custom user needs. It can resolve things that aren't possible with nimsuggest, eg:

proc fun[T](a: T) =
  inspect resolveSymbol(fun(a))
  • it works with any symbol (const,let,var,iterator,macro,template,proc,types etc)

note

note2

will allow doings something more targeted than this:

# in typetraits.nim
export system.`$`

which results in re-exporting all the system.$ overloads instead of just $(typedesc)
(currently, it shows in https://nim-lang.github.io/Nim/typetraits.html#elementType.t%2Cuntyped:

Exports
$, $, $, $, $, $, $, $, $, $, $, $, $, $, $, $, $, $
```)

compiler/ast.nim Outdated Show resolved Hide resolved
@alehander92
Copy link
Contributor

good feature: i do this manually in a hacky and probably wrong way

@Araq
Copy link
Member

Araq commented Aug 28, 2019

Btw, a little offtopic but what use cases are left for system.compiles then?

@timotheecour
Copy link
Member Author

timotheecour commented Aug 28, 2019

Btw, a little offtopic but what use cases are left for system.compiles then?

definitely still useful, at least for testing:

proc testFoo() =
  # bar1() slow at **runtime**
  doAssert compiles bar1()

  # bar2() would bring things in scope
  # bar3() would bring things in scope that clash with bar2
  doAssert compiles bar2()
  doAssert compiles bar3()

There are other use cases I have in mind for compiles.

however, probably almost always overloadExists should now be preferred over compiles for branching code, eg:

when overloadExists(foo(arg)): foo(arg)
else: fooAlt(arg)

@krux02
Copy link
Contributor

krux02 commented Aug 29, 2019

I am not the biggest fan of this pattern.

when overloadExists(foo(arg)): foo(arg)
else: fooAlt(arg)

Usually it is much better to replace it with just foo(arg) and add the overloads of foo where missing.

template foo(arg: MyType): untyped =
  fooAlt(arg)
  
foo(arg)

I prefer it for two reasons. foo(arg) is easier to read than when compiles .... Then I prefer it, because I get better feedback about what branch is taken. When for some reason the correct symbols are not in scope, your solution would fall back to fooArg, in my version there would be a compilation errer, so that I can make sure the correct symbols are in scope.

@timotheecour
Copy link
Member Author

timotheecour commented Aug 29, 2019

Overloads have their use obviously but there are many use cases where when pattern is preferable to what you're suggesting, and I use the pattern heavily.

  • with your template overload pattern, you're silently hijacking overloads. In complex cases, you'll have no idea whether foo masks some existing overload, especially with type classes, generics and code refactorings.
  proc foo(a: SomeOrdinal): auto = (a,"ordinal")
  proc fooAlt[T](a: T): auto = (a,"alt")
  template foo(a: uint8): untyped = fooAlt(a) # hijacks `foo(a: SomeOrdinal)`
  echo foo(1) # (1, "ordinal")
  echo foo(1'u8) # (1, "alt")
  • you may want a catch all clause and give a CT error with additional information, eg:
  proc fun[T](a: T, b: static string) =
    when overloadExists(foo(a)): foo(arg)
    else: doAssert false, $("additional context:", $T, b)

That's not possible with the pattern you're suggesting, you'd just get CT error without the user defined custom context

  • more complex branching, eg:
proc fun[T1, T2](a1: T1, a2: T2) =
  when overloadExists(foo(a1)): foo(a1)
  elif overloadExists(foo2(a2)): foo2(a2)
  elif condition(a1,a2): ....

again, I don't see how the template overload pattern would help there

@krux02
Copy link
Contributor

krux02 commented Aug 30, 2019

Overloads have their use obviously but there are many use cases where when pattern is preferable to what you're suggesting, and I use the pattern heavily.

  • with your template overload pattern, you're silently hijacking overloads.

I would rather call it "providing explicit overloads".

In complex cases, you'll have no idea whether foo masks some existing overload, especially with type classes, generics and code refactorings.

If things become too complex, I usually cut complexity by providing explicit overloads only. Then I know exactly what is going on.

  • you may want a catch all clause and give a CT error with additional information, eg: [...]. That's not possible with the pattern you're suggesting, you'd just get CT error without the user defined custom context

Not really necessary. A missung overload is usually enough information for me to remind myself to introduce that overload.

  • more complex branching, eg: [...]

For me this looks like code smell. I can't think of a situation where I would want to use this pattern.

again, I don't see how the template overload pattern would help there
Overloads have their use obviously but there are many use cases where when pattern is preferable to what you're suggesting, and I use the pattern heavily.

Well when it is possible I always prefer static overloads over the when pattern. There are just some cases where a when pattern is necessary.

@timotheecour timotheecour force-pushed the pr_resolveSymbol_D20190825T173945 branch from ab53f55 to 3586daf Compare March 21, 2020 10:29
@timotheecour timotheecour force-pushed the pr_resolveSymbol_D20190825T173945 branch from 6a9f238 to 2e69fdc Compare March 22, 2020 09:20
@timotheecour
Copy link
Member Author

@Araq PTAL

  • I'm now using efOverloadResolve instead of nfOverloadResolve as you recommended
  • resolveSymbol and overloadExists are now also usable in dot-expressions, avoiding the need for when compiles in those cases too, eg: overloadExists(myInstance.len) can be used and will do the intuitive thing, returning true if len is a field of myInstance or a proc used in method call syntax where overloadExists(len(myInstance)) would be true or a FQ symbol in module myInstance
  • added exhaustive tests

There's been talks recently of getting rid of compiles as much as possible, because:

  • it's expensive (try/catch + spawns a new PContext + more importantly, evaluates routine body instead of just sigmatching
  • it hides errors when sigmatch would succeed but routine body would have some error

overloadExists avoids all those caveats and should almost always be what's used instead of compiles. A notable exception is when we want to actually check that the body compiles too (but that's often not what's needed for purposes of branching)

Note that this is also more flexible than alternatives based on tryCompile.

Furthermore, resolveSymbol is also very useful, to resolve which symbol would be selected in an expression (eg for debugging and other applications)

@krux02
Copy link
Contributor

krux02 commented Mar 22, 2020

I think currently more in the direction of a tryExpression.

tryExpr(something.len):
  body:
    # in this body the argument ``something.len`` exists and can be used with just ``expr``. This way the duplication of the argument doesn't need to happen.
    for x in 0 ..< expr: # `expr` here will be substituted with ``something.len``
       echo x
  fallback:
    # in this body neuther ``expr`` nor ``something.len`` can be used.
    echo "<nothing>"

This tryExpr should be implemented via macros and overload resolution. No compiler change necessary.

@timotheecour
Copy link
Member Author

timotheecour commented Mar 22, 2020

feel free to write a separate PR with tryExpr if you want so we can have a separate discussion there instead of here but I really don't see how your approach (eg based on http://ix.io/2eL3 or other macros) can work, let alone be a replacement for compiles or overloadExists or resolveSymbol:

  • the control flow is too restrictive, eg I don't see how you could write something like this:
when overloadExists(sideEffect1()) and overloadExists(sideEffect2(foo)):
  code involving calls to sideEffect1() and sideEffect2(...)
  • likewise, I don't see how it'd be possible to have a "prelude" that must be called before the 1st use:
when overloadExists(callNeedingNetwork(url)):
  setupNetwork()
  callNeedingNetwork(url)

# or this example:
when overloadExists(expansiveCall(urls[0])):
  if runtimeCondition(): expansiveCall(urls[0]) else: expansiveCall(urls[1])
  • doesn't work with generics eg:
proc fun2[T](a: T): int = 100
echo tryCompile(fun2("foo"), 18) # this gives: Error: ambiguous call with code in http://ix.io/2eL3
  • doesn't allow you to get access to the symbol, unlike this PR (via resolveSymbol)

  • doesn't offer a drop-in syntax replacement for compiles, so users would need complex refactorings to use your feature

  • plus a host of other edge cases eg handling multiple parameters, untyped params, void or typedesc return type, iterators ...

all of which work with my PR btw, see the tests in this PR.

@krux02
Copy link
Contributor

krux02 commented Mar 22, 2020

@timotheecour regarding overloadExists(sideEffect1()), sideEffect1() is not an overload of any kind, because it has no argument.

reganding overloadExists(callNeedingNetwork(url), this is just a prime example wher tryExpr works best.

tryExpr(callNeedingNetwork(url))
  body:
    setupNetwork()
    expr
  fallback:
    discard

# this should work as well:

trExpr(expansiveCall(urls[0])):
  body:
    if runtimeCondition():
      expansiveCall(expr)
    else:
      expansiveCall(urls[1]) # this should work. But I think I still have to prove it since I still didn't provide an implementation.

this gives: Error: ambiguous call with code

True that is a real limitation that I din't think about. Could destroy the entire idea. But I don't want to give it up so soon.

doesn't allow you to get access to the symbol, unlike this PR (via resolveSymbol)

I just looked at your resolveSymbol and yes resolveSymbol is something that I wished often would be available in Nim. Because it is lacking I have to create untyped macros that generate calls to types macros so I finally get resolved symbols. But your type signature of resolveSymbol is wrong. It should be NimNode to NimNode. nnkIdent to nnkSym or nnk{Open,Closed}SymChoice transformation. Not untyped.

doesn't offer a drop-in syntax replacement for compiles, so users would need complex refactorings to use your feature.

True could be worked on. But for the concept I currently don't really care that much.

@timotheecour
Copy link
Member Author

timotheecour commented Mar 22, 2020

sideEffect1() is not an overload of any kind, because it has no argument.

no argument doesn't mean it can't indicate an overload:

  • proc foo() overloads proc foo(a: int),
  • proc foo(a = 0) overloads proc foo(a: string)

this is just a prime example wher tryExpr works best.

I don't see how that could be implemented, I suggest to follow-up here: timotheecour#71 to keep this discussion focused on this PR.

@Araq
Copy link
Member

Araq commented Mar 24, 2020

Regarding nim-lang/RFCs#164 I really like overloadExists much better though.

@stale
Copy link

stale bot commented Mar 25, 2021

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 Mar 25, 2021
@timotheecour timotheecour removed the stale Staled PR/issues; remove the label after fixing them label Mar 26, 2021
@stale
Copy link

stale bot commented Apr 18, 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 Apr 18, 2022
@stale stale bot closed this May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

overloadExists(foo(args)) to tell whether an overload exists without compiling/running body via compiles
4 participants