Skip to content

Conversation

bung87
Copy link
Collaborator

@bung87 bung87 commented Sep 22, 2022

fix #16736
fix #16737

@bung87 bung87 marked this pull request as draft September 22, 2022 17:24
@bung87 bung87 changed the title fix #16736 assign proc to const fix #16736 assign closure proc to const Sep 23, 2022
@bung87 bung87 changed the title fix #16736 assign closure proc to const fix #16736 assign closure proc to const should be CT error Sep 23, 2022
@bung87 bung87 marked this pull request as ready for review September 23, 2022 04:22
@bung87
Copy link
Collaborator Author

bung87 commented Sep 23, 2022

CI fails cause by #14340 related test case , if this PR accepted , should remove that test case.

proc opl3EnvelopeCalcSin0() = discard
type EnvelopeSinfunc = proc()
# const EnvelopeCalcSin0 = opl3EnvelopeCalcSin0 # ok
const EnvelopeCalcSin0: EnvelopeSinfunc = opl3EnvelopeCalcSin0 # was bug
const envelopeSin = [EnvelopeCalcSin0]
var a = 0
envelopeSin[a]()

@top-master
Copy link

top-master commented Sep 24, 2022

@bung87 You basically cause the error:

tvmmisc.nim(358, 11) Error: const 'EnvelopeCalcSin0' cannot be assigned to proc of calling convention 'closure'

And why do you think setting to a const should be forbidden?

Like:

  • const just means that the internal state is not modifiable by outsiders,
  • and that such attempts should cause compile-error,
  • but as it is a closure, I doubt anything "outside" has even access, hence closure is truly const even without const-keyword use.

Update; Above I am confusing const with let (to be expected, for someone with C/C++ experience).

The only difference with let is that, Nim's const expressions are evaluated at compile-time, just like constexpr in C++.

Nim-lang Example:

var x = 123
let y = x+4 # Works.
const z = x+4 # Throws error.

@beef331
Copy link
Collaborator

beef331 commented Sep 24, 2022

Bung pinged me in realtime chat I assume to respond

const is not a let as such it has different semantics. The way I see it is a const closure does not make much sense as refs are presently not capable of being used for const, This is a problem for a closure as it has a heap allocated environment.

Not only that but presently the following errors:

const a = proc() {.closure.} = echo "Hello"
a()

Error: VM problem: dest register is not set

Since it's attempting to evaluate a at CT and not runtime.

but as it is a closure, I doubt anything "outside" has even access, hence closrure is truly const even without const-keyword use.

This Exists.

Another example to explain how odd it is of a concept to me:

proc makeMyClosure(r: ref int): proc() =
    result = proc() =
      inc r[]
      echo r[]

var myRef {.compileTime.} = new int # This makes a reference in VM since it needs to be accessible at CT, but we can make a closure to capture it.

const a = makeMyClosure(myRef)
let b = a

b()

@bung87
Copy link
Collaborator Author

bung87 commented Sep 24, 2022

Thanks beef's response, am not a native english speaker and not capble to explain this, so I pinged beef, as we discussed yesterday about this. now am pretty sure this should be CT error.

@Varriount Varriount requested a review from Araq September 25, 2022 19:03
@Varriount Varriount added the Requires Araq To Merge PR should only be merged by Araq label Sep 25, 2022
@Araq
Copy link
Member

Araq commented Sep 28, 2022

Patch the existing typeAllowed code for this.

@arnetheduck
Copy link
Contributor

Why shouldn't const be allowed to be a proc? It makes proc's less of first-class citizens in the language, which prevents some code and generic constructs, as well as renamed function pointers and other use cases

@bung87
Copy link
Collaborator Author

bung87 commented Sep 29, 2022

Why shouldn't const be allowed to be a proc? It makes proc's less of first-class citizens in the language, which prevents some code and generic constructs, as well as renamed function pointers and other use cases

today's news is it is planed to support, but for now it may causes users mis-use, which we dont like to see.

@arnetheduck
Copy link
Contributor

Example use case: status-im/nim-chronos#319

@bung87
Copy link
Collaborator Author

bung87 commented Sep 29, 2022

since chronos used as a real use case, that can't be patched in a easy way, I decide partially support it or wait real const closure implementation.

@metagn
Copy link
Collaborator

metagn commented Sep 29, 2022

Since it's attempting to evaluate a at CT and not runtime.

Can't we find a way to only allow calling non-closures converted to a closure type at runtime? i.e. if the closure environment is nil, or whatever that would correspond to for the compile time VM (I'm guessing all procvars at compile time are stored as symbols, so we would check if the symbol type is a closure).

@bung87 bung87 marked this pull request as draft October 1, 2022 03:29
@bung87 bung87 closed this Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Araq To Merge PR should only be merged by Araq
Projects
None yet
Development

Successfully merging this pull request may close these issues.

const procs in const array are nil in half of array const proc in let array causes invalid codegen
7 participants