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

don't mark declval as {.compileTime.} #190

Closed
wants to merge 1 commit into from

Conversation

metagn
Copy link

@metagn metagn commented Jun 6, 2023

declval is meant to be used to determine types of expressions, and never be executed (#33). However it's marked {.compileTime.}, which would normally make it always execute during constant folding, but due to a bug in Nim (nim-lang/Nim#10753), generic procs like declval do not undergo constant folding. So the {.compileTime.} annotation is removed for this to work when the bug in Nim is fixed.

(Unfortunately this still only errors at execution time, I don't know a mechanism that errors at compile time but only outside typeof/concepts)

`declval` is meant to be used to determine types of expressions, and never be executed (#33). However it's marked `{.compileTime.}`, which would normally make it always execute during constant folding, but due to a bug in Nim (nim-lang/Nim#10753), generic procs like `declval` do not undergo constant folding. So the `{.compileTime.}` annotation is removed for this to work when the bug in Nim is fixed.
@metagn
Copy link
Author

metagn commented Aug 25, 2023

Would be nice if this got reviewed, this is needed for CI to pass with the bugfix in nim-lang/Nim#22022, and this library is probably too active/important to fork for CI.

@arnetheduck
Copy link
Member

this "feels" wrong - ie declval is clearly a compile-time thing so having it marked otherwise seems counter-intuitive - is there a fundamental language reason it should not be marked as such or is it just that fixing one bug exposes another?

@metagn
Copy link
Author

metagn commented Aug 16, 2024

The compileTime pragma normally also implies the proc should be evaluated at compile time as well as blocking them from getting code generated, i.e.:

proc foo(x: int) {.compileTime.} = echo x
foo(123)

is supposed to behave the same as:

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

There is a bug in Nim where specifically generic routines are exempt from this and don't get folded at compile time, declval here is a generic routine, and unfortunately giving it the same behavior as non-generic routines breaks it.

I couldn't come up with a new mechanism to fit declval's needs here that wasn't too niche, we could literally add something like blockCodegen that errors at codegen like compileTime and can even error if the VM tries to generate code for it, but the compiler isn't good at error messages delayed until codegen in general (nim check evades it). In practice the doAssert false makes it act like noreturn but I don't know if this would cause problems with the type system.

@arnetheduck
Copy link
Member

proc should be evaluated at compile time

what prevents declval from being evaluated then? also, declval is typically used in a particular kind of evaluation, namely one where only metadata is used ("what's the type of this complex expression") - ie it seems to me that in the compiler, there should be a lazy evaluation mechanism that defers actual evaluation until it's needed, which should solve the eager evaluation problems that you're probably running into, in this case.

Separately, it's a bit of an open question how useful this "force compile-time evaluation" mechanism is - constant expression folding sounds like something the compiler could be doing "most" of the time irrespective of the pragma, except when side effects like rounding and overflow detection are involved.

@metagn
Copy link
Author

metagn commented Aug 16, 2024

what prevents declval from being evaluated then?

In this case the lack of the compileTime pragma; typeof performs constant folding regardless, which I'm just now realizing is probably bad behavior. I'll try disabling it, sorry for not thinking about it more.

it's a bit of an open question how useful this "force compile-time evaluation" mechanism is

The use case that brought me onto it was similar to how one would want to handle BigInt constant constructors, so maybe one could do something like:

proc bigint(s: string): BigInt = ...
proc bigint(s: static string): BigInt {.compileTime.} = ...

but the static string version here wouldn't work since it's generic, hence the issue. I'm sure there are better use cases.

constant expression folding sounds like something the compiler could be doing "most" of the time irrespective of the pragma, except when side effects like rounding and overflow detection are involved.

The compiler switch --implicitStatic does this (treats every non-side-effect pure proc as soft compileTime) and I think it was on by default at some point, it's possible it has adverse effects for compile times.

@arnetheduck
Copy link
Member

arnetheduck commented Aug 16, 2024

The use case that brought me onto it was similar to how one would want to handle BigInt constant constructors, so maybe one could do something like:

the latter does not need a compileTime pragma though - ie the static string version is called (with preference) when the string is static and otherwise not - this is a trick we already use in strformat for example: nim-lang/Nim#23356

notably, in the strformat case, the primary motivator was actually to avoid unwanted raises effects that propagate to the whole program, even when the format string (and therefore the potential for raising) is known at compile-time and the way of "enforcing" compile-time behavior is to request it at the call site (using static and const)

@arnetheduck
Copy link
Member

The use case that brought me onto it was similar to how one would want to handle BigInt constant constructors, so maybe one could do something like:

btw, this would not work in general since bigint requires runtime memory allocation, since the size of the bigint is arbitrary / unbounded.

@metagn
Copy link
Author

metagn commented Aug 18, 2024

Ended up not being needed, closing

this would not work in general since bigint requires runtime memory allocation

I was talking about the parsing

@metagn metagn closed this Aug 18, 2024
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.

None yet

2 participants