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

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

Open
timotheecour opened this issue Jul 13, 2018 · 5 comments
Open

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

timotheecour opened this issue Jul 13, 2018 · 5 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Jul 13, 2018

reduced from a mysterious error I was seeing here https://travis-ci.org/nim-lang/Nim/jobs/402492005 for this PR: #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
Copy link
Member Author

timotheecour commented Jul 13, 2018

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 timotheecour changed the title Error: type mismatch triggered on travis/appveyor but not locally Error: type mismatch triggered on travis/appveyor but not locally; caused by map overly restrictive Jul 13, 2018
timotheecour added a commit to timotheecour/Nim that referenced this issue Jul 14, 2018
@timotheecour timotheecour changed the title Error: type mismatch triggered on travis/appveyor but not locally; caused by map overly restrictive map shouldn't care whether proc arg is cdecl Aug 8, 2018
@timotheecour
Copy link
Member Author

timotheecour commented Aug 8, 2018

workaround in some cases: use mapIt(fun(it)) instead of map(fun) (now that #8543 was merged)

EDIT => blocked by another issue: #8577

@timotheecour timotheecour changed the title map shouldn't care whether proc arg is cdecl map shouldn't care whether proc arg is cdecl Aug 8, 2018
timotheecour added a commit to timotheecour/Nim that referenced this issue Aug 8, 2018
timotheecour added a commit to timotheecour/Nim that referenced this issue Aug 8, 2018
timotheecour added a commit to timotheecour/Nim that referenced this issue Aug 8, 2018
timotheecour added a commit to timotheecour/Nim that referenced this issue Aug 8, 2018
timotheecour added a commit to timotheecour/Nim that referenced this issue Aug 14, 2018
timotheecour added a commit to timotheecour/Nim that referenced this issue Aug 16, 2018
@timotheecour
Copy link
Member Author

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

@Araq
Copy link
Member

Araq commented Jun 29, 2021

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

@timotheecour
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants