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

RFC: deprecate implicit conversions from procvar => pointer and ptr T => pointer; enable function merging #229

Closed
timotheecour opened this issue May 21, 2020 · 4 comments · Fixed by nim-lang/Nim#21953

Comments

@timotheecour
Copy link
Member

timotheecour commented May 21, 2020

in C++

C++ (and other languages) rightfully disallow implicit conversion from function pointer to void*, giving errors like:
no known conversion from 'void (int, double)' to 'void *' for 1st argument

Likewise, == between function pointers of different types is also disallowed. giving errors like:
error: comparison of distinct pointer types ('void (*)(int, double)' and 'void (*)(int)')

However, == between function pointers of same type is allowed and works.

in nim

nim defines:

proc `==`*(x, y: pointer): bool {.magic: "EqRef", noSideEffect.}
proc `==`*[T: proc](x, y: T): bool {.magic: "EqProc", noSideEffect.}
  ## Checks that two `proc` variables refer to the same procedure.

combine this with the fact that proc (actually, procvar, which is now the default) implicitly converts to pointer, and you get both of C++ guarantees thrown out, so that == works between unrelated types, including between a ptr Foo and a ptr Bar or between a ptr Foo and a proc fun(a: int) or a proc fun(a: int) and a proc bar(b: string): bool, which makes no sense from type safety point of view.

  proc fn(a: pointer) = echo cast[int](a)
  proc bar1(a: int, b: float) = echo $(a, b)
  proc bar2(a: int) = echo $(a, )

  proc bar3(a: int, b: float) = discard
  proc bar4[T](a: T, b: float): char = discard

  type
    Foo1 = object
    TFoo1 = ptr Foo1
    PFoo1 = ref Foo1
    Foo2 = object
    TFoo2 = ptr Foo2
  var x1: TFoo1
  var x2: TFoo2
  var x3: PFoo1

  # BUG: none of these should compile; should need explicit conversion (eg: bar1.pointer)
  fn(bar1)
  fn(bar2)
  echo x1 == x2
  echo x1 == bar1
  echo bar1 == bar2

  # echo x1 == x3 # ok: CT error

  # OK: these are ok
  var x1b: TFoo1
  echo x1b == TFoo1.default # == for `ptr T` of same type
  var x3b: PFoo1
  echo x3b == x3 ##  # == for `ref T` of same type
  var bar1b: type(bar1)
  echo bar1b == bar1  # == for `proc` of same type
  echo bar1b == bar4[int] # generic instantiation or proc is irrelevant so long it resolves to same type; return type irrelevant
  # echo bar1b == bar4[float] # resolves to different type => invalid

proposal

  • deprecate (error unless some compatibility flag --legacy:ptrliketopointer, in which case just warning) implicit conversions from procvar to pointer
  • ditto with ptr T to pointer
  • allow explicit conversion to pointer; I suggest doing it via foo.pointer or pointer(foo) (and it'd apply to all ptr-like types, including proc, ref T, ptr T, cstring and pointer)
  • == stays valid for procs or ptr T so long the type of operands is the same; for procvar, what matters is the type, not whether it's a generic instantiation or not
  • == stays valid for ref T (unchanged)

related

ptr char` implicitly converts to cstring, resulting in undefined behavior

This makes ptr T and procvar consistent with ref T for both == and for how conversion to pointer works (explicit, not implicit)

enables function merging

  • this enables function merging which was a key motivation from Deprecate == for refs and procs #224 (I will rebut the other aspects from that RFC later); if 2 procs fn1, fn2 generate identical code, the optimizer can merge them so that they'll have the same address; if they're the same type than there's no contradiction with fn1 == fn2; and if they're different types than fn1 == fn2 does not compile so there's also no contradiction

note

https://stackoverflow.com/questions/26533740/do-distinct-functions-have-distinct-addresses
function merging is controversial and some compilers do it (MSVC aggressively COMDAT folds functions, so two functions with the same implementation can be turned into one function), whereas most others (eg clang) don't or at least only do it under the "as-if" rule whereby merge only happens if no visible behavior changes, including by taking function address; that last fact makes it tricky to tell whether function merging happens so you have to disassemble to check. Nim perhaps could be smart here and do some function merging before handing it to cgen to avoid relying in C compiler doing it. At least nim could be smarter about caching generic instantiations in light of this.

@Araq
Copy link
Member

Araq commented May 21, 2020

Just to be clear, I really want "MSVC aggressively COMDAT folds functions, so two functions with the same implementation can be turned into one function)," for Nim too.

@Araq
Copy link
Member

Araq commented May 21, 2020

this enables function merging which was a key motivation from #224 (I will rebut the other aspects from that RFC later); if 2 procs fn1, fn2 generate identical code, the optimizer can merge them so that they'll have the same address; if they're the same type than there's no contradiction with fn1 == fn2; and if they're different types than fn1 == fn2 does not compile so there's also no contradiction

If procs can be compared then f must be unequal to g for proc f; proc g (note the identical types!) and so this RFC doesn't do what it claims to do because implicit conversions are irrelevant for my example. The spec must either explicitly allow for f == g regardless or it must disallow == for procs at compile-time.

Having said that, fewer implicit conversions are a good thing and the RFC is good, but it doesn't really address the "function merging" problem.

@github-actions
Copy link

This RFC is stale because it has been open for 1095 days with no activity. Contribute a fix or comment on the issue, or it will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 25, 2023
@metagn
Copy link
Contributor

metagn commented May 25, 2023

This wasn't done already?

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

Successfully merging a pull request may close this issue.

3 participants