search

nim-lang / RFCs

A repository for your Nim proposals.
135 stars 26 forks source link

Remove runtime access to {.compileTime.} variable #338

Open mratsim opened 3 years ago

mratsim commented 3 years ago

The PR https://github.com/nim-lang/Nim/pull/12128 allowed runtime access to compile-time variables to solve https://github.com/nim-lang/Nim/issues/10990

This is a controversial change:

that opened a can of worms:

I don't think there is any code that rely on that feature because it does not work.

I suggest removing it and make static(myExpression) the standard way to pass data from {.compileTime.} variable to runtime variable with an appropriate error message when a compileTime variable is used at runtime.

This has the following benefits:

timotheecour commented 3 years ago

I agree with this.

Very related (and compatible with this RFC) is this proposal: https://github.com/nim-lang/RFCs/issues/283 which makes globals variables (eg {.threadvar.} and {.global.}) at module level work at CT, just like {.threadvar.} and {.global.} at proc scope already do, without cross-over between RT and CT. It simplifies many use cases and makes code work at CT just like it does at RT. There is no cross-over between RT and CT (you need const or static for that), which would cause all sorts of issues.

furthermore, code like this:

proc main =
  echo getCurrentCompilerExe()

should generate a warning, and eventually an error when used in a RT context. Instead, what should be used is: echo getCurrentCompilerExe().static or const s = getCurrentCompilerExe() unless of course main is already called from a static context.

krux02 commented 3 years ago

As I said from the very beginning. Here is my patch that I did in the past. Could be outdated:

https://github.com/krux02/Nim/commit/db7dee6a96237f3fb5fe3664f3f9c5e08d951b2e

juancarlospaco commented 3 years ago

I started to use static( ) sometimes with block: because I found it "works on more places" compared to {.compileTime.}.

Is there any use case were static() + block: can not replace {.compileTime.} ?.

if no, then {.compileTime.} should be Deprecated. if yes, then {.compileTime.} should be Fixed.

(I am asking, not confirming, I am neutral, but in favor of whatever is less bug prone)

krux02 commented 3 years ago

@juancarlospaco 

proc foo(arg: Foo): Foo {.compileTime.} = 
  # a function that can only be called at compile time. Trying to generate a call to this procedure at runtime generates an error
  discard

var a: Foo # a global variable that can be modified from procedures tagged with {.compileTime.}. Currently it does something wonkey if you try to access it at runtime. This RFC wants to make it emit an error again to work like the {.compileTime.} pragma on procs, like it used to.
juancarlospaco commented 3 years ago

Makes sense, I am wrong, should be Fixed, if that includes reverting it back so be it. This is a Bug more than rfc.

Araq commented 3 years ago

Did anybody read the spec and the reason behind this feature? Oh well.

timotheecour commented 3 years ago

@Araq

Did anybody read the spec

yes, and the spec is either wrong or badly worded:

compileTime variables are available at runtime too. This simplifies certain idioms where variables are filled at compile-time (for example, lookup tables) but accessed at runtime:

when true:
  var a {.compileTime.}: int
  proc main()=
    echo a
    a.inc
    echo a
  static: main()
  main()

prints:

vm:
0
1
rt:
0 # instead of 1
1

furthermore the example given in spec is also misleading:

This simplifies certain idioms where variables are filled at compile-time (for example, lookup tables) but accessed at runtime: var nameToProc {.compileTime.}: seq[(string, proc (): string {.nimcall.})]

because nameToProc really isn't filled at CT but at RT: static: doAssert nameToProc[2][1]() == "baz" would fail with Error: index out of bounds, the container is empty (it's just a pragma macro, which inserts code that evaluates at RT, not CT (nameToProc.add(("foo", foo))) , and has nothing to do with evaluation at CT in this example;

and indeed, the example given would still pass if using instead: var nameToProc: seq[(string, proc (): string {.nimcall.})].

and the reason behind this feature? Oh well.

{.compileTime.} should require calling it via a static context eg static or const, otherwise you end up with phase ordering problems.

One aspect not mentioned in this RFC is your remark here: https://github.com/nim-lang/RFCs/issues/283#issuecomment-725910690

This is how the spec used to be:

A global var that are accessed via a compile-time mechanism needs to be annotated via .compileTime. To use a .compileTime var at runtime, convert it to a a const or let helper variable.

One big problem was when to turn it into this helper variable. It needs to be done after the full compilation. This is hard to implement properly but we require the same mechanism for IC and "method" code generation so there is no way around it. So since conversion from .compileTime vars to runtime vars is hard to do with const, we can say that a compileTime variable can be used at runtime and it's initial value is that after the full compilation. And maybe in order to use a .compleTime variable at runtime you need a special accessing mode like runtime(ctVar) so that it's clear what happens.

and indeed, that "after whole program compilation" conversion should be opt-in and explicit (via runtime or delayedStatic), not implicit (as done now in a very buggy and error prone way)