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

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

Open
mratsim opened this issue Feb 9, 2021 · 7 comments
Open

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

mratsim opened this issue Feb 9, 2021 · 7 comments

Comments

@mratsim
Copy link
Collaborator

mratsim commented Feb 9, 2021

The PR nim-lang/Nim#12128 allowed runtime access to compile-time variables to solve nim-lang/Nim#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
Copy link
Member

timotheecour commented Feb 9, 2021

I agree with this.

Very related (and compatible with this RFC) is this proposal: #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
Copy link
Contributor

krux02 commented Feb 9, 2021

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

krux02/Nim@db7dee6

@juancarlospaco
Copy link
Contributor

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
Copy link
Contributor

krux02 commented Feb 9, 2021

@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
Copy link
Contributor

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
Copy link
Member

Araq commented Feb 9, 2021

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

@timotheecour
Copy link
Member

timotheecour commented Feb 10, 2021

@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: #283 (comment)

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)

@nim-lang nim-lang deleted a comment from krux02 Feb 10, 2021
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

No branches or pull requests

5 participants