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

disallow static in return type #9686

Merged
merged 4 commits into from
Aug 5, 2019

Conversation

krux02
Copy link
Contributor

@krux02 krux02 commented Nov 12, 2018

this code should not compile anymore:

proc foo(x: int): static int =
  x + 123

echo foo(123)

@mratsim
Copy link
Collaborator

mratsim commented Nov 12, 2018

Can we make the compile-time pragma work in such case then? Related: #8051

@mratsim
Copy link
Collaborator

mratsim commented Nov 28, 2018

Currently I'm relying on static return types to workaround #9365 and in general to generate unique ID at compile-time.

# omp_mangling.nim
import random
from strutils import toHex

var mangling_rng {.compileTime.} = initRand(0x1337DEEDBEAF)
var current_suffix {.compileTime.} = ""

proc omp_suffix*(genNew: static bool = false): static string =
  ## genNew:
  ##   if false, return the last suffix
  ##   else return a fresh one

  if genNew:
    current_suffix = mangling_rng.rand(high(uint32)).toHex
  result = current_suffix

Due to #9817, {.compileTime.} pragma will not work for this use case.

In general, the ability to enforce compile-time function evaluation is very useful to avoid adding static(foo(bar)) everywhere in the code and the current static T somehow works without any bugs even though @zah mentioned that he never tested or intended it to work in #8051

@mratsim
Copy link
Collaborator

mratsim commented Dec 7, 2018

After thinking about that a bit, I think we should do the other way around.

Everywhere it makes sense, make the {.compileTime.} pragma implicit. I.e. in the following case:

  • Instantiation from macro (including but not restricted to NimNode)
  • Instantiation from {.compileTime.} variable
  • Input is a type/typedesc

Then {.compileTime.} for functions can be removed and only values matter.

And for devs/users to ensure compile-time function evaluation, keep the static modifier. This is consistent with modifiers like var T and the future lent T.

@krux02 krux02 force-pushed the disallow-static-return-type branch from af16fc0 to ecfb271 Compare May 7, 2019 14:30
@mratsim
Copy link
Collaborator

mratsim commented May 7, 2019

Can we change the error to "Use the {.compileTime.} pragma to enforce compile-time evaluation of this routine"?

@krux02 krux02 changed the title [WIP] disallow static in return type disallow static in return type May 7, 2019
mratsim added a commit to mratsim/laser that referenced this pull request May 8, 2019
@krux02
Copy link
Contributor Author

krux02 commented Jul 30, 2019

@Araq can you please merge this. It is ready for a long time now.

@Araq Araq merged commit ce148e7 into nim-lang:devel Aug 5, 2019
@krux02
Copy link
Contributor Author

krux02 commented Aug 6, 2019

Yay, finally. 😀

@Clyybber
Copy link
Contributor

I think this should be reverted and {.compileTime.} should imply static return type for procs.

Additionally a static return type must imply all parameters are static as well.

@mratsim
Copy link
Collaborator

mratsim commented Nov 22, 2019

@Clyybber while I'm in favor, you should detail what you said on IRC: that it fits better with the type system.

@krux02
Copy link
Contributor Author

krux02 commented Nov 22, 2019

@Clyybber Any thoughts on why this should be reverted? Your suggestion would break some of my code. Here is an incomplete list of the compileTime pragma in my codebase that would probably not work anymore:

https://github.com/krux02/opengl-sandbox

experiment/glslTranslate.nim:proc pseudoBase64*(dst: var string; arg: int): void {.compileTime.} =
experiment/glslTranslate.nim:proc compileProcToGlsl(result: var string; arg: NimNode): void {.compileTime.} =
experiment/glslTranslate.nim:proc compileToGlsl*(result: var string; arg: NimNode): void {.compileTime} =
experiment/renderMacro.nim:proc injectTypes(framebuffer, mesh, arg: NimNode): NimNode {.compileTime.} =
fancygl/boring_stuff.nim:proc isBuiltIn*(sym: NimNode): bool {.compileTime.} =
fancygl/boring_stuff.nim:proc isBuiltIn*(procName: string): bool {.compileTime.} =
fancygl/glwrapper.nim:proc validateShader*(src: string, shaderType: GLenum): bool {.compileTime.} =
fancygl/macroutils.nim:proc newPrefix*(n1,n2: NimNode): NimNode {. compileTime .} =
fancygl/macroutils.nim:proc newInfix*(op, n1, n2: NimNode): NimNode {. compileTime .} =
fancygl/macroutils.nim:proc newRecList*(args: varargs[NimNode]): NimNode {.compileTime.} =
fancygl/macroutils.nim:proc newRangeUntil*(upper: int): NimNode {.compileTime.} =
fancygl/macroutils.nim:proc newTypeDef(name, tpe: NimNode): NimNode {.compileTime.} =
fancygl/macroutils.nim:proc newObjectTy*( name, recList: NimNode ): NimNode {.compileTime.} =
fancygl/macroutils.nim:proc newExpIdentDef*(name: string, tpe: NimNode): NimNode {. compileTime .} =
fancygl/macroutils.nim:proc expectIdent*(n: NimNode, name: string) {.compileTime.} =
fancygl/offsetof.nim:proc calcOffset(data: openarray[tuple[size,align: int]]): int {.compileTime.} =
fancygl/shapes.nim:proc genTetraederVertices(): array[12, Vec4f] {.compileTime.} =
fancygl/shapes.nim:proc genTetraederNormals(): array[12, Vec4f] {.compileTime.} =
fancygl/shapes.nim:proc getTetraederTexCoords(): array[12, Vec2f] {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment(_: typedesc[bool]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment(_: typedesc[int32]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment(_: typedesc[int64]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment(_: typedesc[uint32]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment(_: typedesc[uint64]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment(_: typedesc[float32]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment(_: typedesc[float64]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment[T](_: typedesc[Vec4[T]]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment[T](_: typedesc[Vec3[T]]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment[T](_: typedesc[Vec2[T]]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment[M,N,T](_: typedesc[Mat[M,N,T]]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment[T: tuple | object](_: typedesc[T]): int32 {.compileTime.} =
fancygl/std140AlignedWrite.nim:func getAlignment[N,T](_: typedesc[array[N,T]]): int32 {.compileTime.} =
fancygl/typeinfo.nim:proc glslType*(arg: NimNode): string {.compileTime.} =

github.com/krux02/ast-pattern-matching

src/ast_pattern_matching.nim:proc failWithMatchingError*(arg: MatchingError): void {.compileTime, noReturn.} =
src/ast_pattern_matching.nim:proc expectValue(arg: NimNode; value: SomeInteger): void {.compileTime.} =
src/ast_pattern_matching.nim:proc expectValue(arg: NimNode; value: SomeFloat): void {.compileTime.} =
src/ast_pattern_matching.nim:proc expectValue(arg: NimNode; value: string): void {.compileTime.} =
src/ast_pattern_matching.nim:proc expectValue[T](arg: NimNode; value: pointer): void {.compileTime.} =
src/ast_pattern_matching.nim:proc expectIdent(arg: NimNode; strVal: string): void {.compileTime.} =
src/ast_pattern_matching.nim:proc matchLengthKind*(arg: NimNode; kind: set[NimNodeKind]; length: int): MatchingError {.compileTime.} =
src/ast_pattern_matching.nim:proc matchLengthKind*(arg: NimNode; kind: NimNodeKind; length: int): MatchingError {.compileTime.} =
src/ast_pattern_matching.nim:proc matchValue(arg: NimNode; kind: set[NimNodeKind]; value: SomeInteger): MatchingError {.compileTime.} =
src/ast_pattern_matching.nim:proc matchValue(arg: NimNode; kind: NimNodeKind; value: SomeInteger): MatchingError {.compileTime.} =
src/ast_pattern_matching.nim:proc matchValue(arg: NimNode; kind: set[NimNodeKind]; value: SomeFloat): MatchingError {.compileTime.} =
src/ast_pattern_matching.nim:proc matchValue(arg: NimNode; kind: NimNodeKind; value: SomeFloat): MatchingError {.compileTime.} =
src/ast_pattern_matching.nim:proc matchValue(arg: NimNode; kind: set[NimNodeKind]; value: string): MatchingError {.compileTime.} =
src/ast_pattern_matching.nim:proc matchValue(arg: NimNode; kind: NimNodeKind; value: string): MatchingError {.compileTime.} =
src/ast_pattern_matching.nim:proc matchValue[T](arg: NimNode; value: pointer): MatchingError {.compileTime.} =
tests/test1.nim:proc peelOff*(arg: NimNode, kinds: set[NimNodeKind]): NimNode {.compileTime.} =
tests/test1.nim:proc peelOff*(arg: NimNode, kind: NimNodeKind): NimNode {.compileTime.} =
tests/test1.nim:    proc `procSym`(): void {.compileTime.} =

bitbucket.org/krux02/pubviz

graphgen.nim:  proc getRecList(arg: NimNode): NimNode {.compileTime.} =
graphgen.nim:  proc getTypeDef(arg: NimNode): NimNode {.compileTime.} =

@Clyybber
Copy link
Contributor

@krux02 I don't intend on removing {.compileTime.}, but rather making {.compileTime.} mean exactly the same as returning static.
Maybe introducing {.static.} as an alias for {.compileTime.} will suffice, as this is more of a documentation/consistency thing.

The second part of my proposal concerns the compiler implementation; making {.compileTime.}/{.static.}/static return type imply all parameters being static, the compiler already does this to some extent I think, but I think its a good idea to move that transformation to sem.

@krux02
Copy link
Contributor Author

krux02 commented Nov 23, 2019

@Clyybber A static parameter means that one instance of a function is generate for each value. This means that you can use static arguments in a when statement inside of the function. All parameters being static is almost never a good idea, it just means that you will have an explosion in code generation because the growth of parameter combinations is exponential. On the other hand, {.compileTime.} just means that the function is inteded to be executed at compile time only. This function is generated once, but it won't be generate C or JS code, it will only generate nimVM code. I personally think that "{.compileTime.}/{.static.}/static return type imply all parameters being static" is a horrible horrible idea.

@Clyybber
Copy link
Contributor

Hmm, I see. Discard that idea then.

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 this pull request may close these issues.

4 participants