nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.56k stars 1.47k forks source link

`map` shouldn't care whether proc arg is `cdecl` #8303

Open timotheecour opened 6 years ago

timotheecour commented 6 years ago

reduced from a mysterious error I was seeing here https://travis-ci.org/nim-lang/Nim/jobs/402492005 for this PR: https://github.com/nim-lang/Nim/pull/8272

The code that caused it:

result = args.map(quoteShell).join(" ")

it worked if I wrote instead:

# with import sugar
result = args.map(a=>quoteShell(a)).join(" ")

After further investigating and reducing the error it boils down to: map somehow can't be used with a proc that's marked as {.rtl.} (or, after reducing further from the definition of rtl in system/inclrtl, if it's marked as {. exportc: "nimrtl_$1", dynlib .})

#[ 
Error: type mismatch: got <openarray[string], proc (s: string): string{.cdecl, noSideEffect, gcsafe, locks: 0.}>
but expected one of:
proc map[T](s: var openArray[T]; op: proc (x: var T))
  for a 'var' type a variable needs to be passed, but 'args' is immutable
proc map[T, S](s: openArray[T]; op: proc (x: T): S): seq[S]
  first type mismatch at position: 2
  required type: proc (x: T): S{.closure.}
  but expression 'fun' is of type: proc (s: string): string{.cdecl, noSideEffect, gcsafe, locks: 0.}
  for a 'var' type a variable needs to be passed, but 'args' is immutable

expression: map(args, fun)
    result = args.map(fun).join(" ")
 ]#

include "system/inclrtl"

import sequtils

# This would also fail: 
# proc fun*(s: string): string {. exportc: "nimrtl_$1", dynlib .} = s
proc fun*(s: string): string {. rtl .} = s

proc funWrapper*(s: string): string = fun(s)

# OK
echo ["1"].map(funWrapper)
# could also be defined inline: echo ["1"].map(proc(s:string):auto=fun(s))

# Error: type mismatch:
echo ["1"].map(fun)

Isn't that a bug?

it makes things work locally but fail on travis/appveyor. (or fail locally when run with nim c -o:app --define:createNimRtl --app:lib test.nim instead of nim c -o:app --app:lib test.nim (when something uses {.rtl.}))

(or fail when using nim c -o:app bugs/t50_app_lib_map.nim when something uses {. exportc: "nimrtl_$1", dynlib .})

[EDIT] workaround in some cases see https://github.com/nim-lang/Nim/issue_comments#issuecomment-411508903

timotheecour commented 6 years ago

so the root problem is that --define:createNimRtl --app:lib implies {.rtl.} implies {. exportc: "nimrtl_$1", dynlib .} implies {.cdecl.} and that's incompatible with map's 2nd argument op in lib/pure/collections/sequtils.nim:

proc map*[T, S](s: openArray[T], op: proc (x: T): S ): seq[S] =
  newSeq(result, s.len)
  for i in 0 ..< s.len:
    result[i] = op(s[i])

However this works if we use instead:

proc map3*[Fun, T](s: openArray[T], op: Fun ): auto =
  result = newSeq[type(op(s[0]))](s.len)
  for i in 0 ..< s.len:
    result[i] = op(s[i])
timotheecour commented 6 years ago

workaround in some cases: use mapIt(fun(it)) instead of map(fun) (now that https://github.com/nim-lang/Nim/pull/8543 was merged)

EDIT => blocked by another issue: https://github.com/nim-lang/Nim/issues/8577

timotheecour commented 3 years ago

@araq can sigmatch ignore {.cdecl.}, assuming it's only used for name mangling of a declaration?

Araq commented 3 years ago

Oh no, .cdecl is really a different calling convention (a slower one on 32 bit Windows) and has little to do with mangling.

timotheecour commented 3 years ago

I was misled by this comment:

    # since we'll be loading the dynlib symbols dynamically, we must use
    # a calling convention that doesn't introduce custom name mangling
    # cdecl is the default - the user can override this explicitly

but the manual indeed says:

decl
The cdecl convention means that a procedure shall use the same convention as the C compiler. Under Windows the generated C procedure is declared with the __cdecl keyword.