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

mark dbQuote proc param to dbFormatImpl as {.nimcall.} #23

Merged
merged 1 commit into from
Jan 6, 2024
Merged

mark dbQuote proc param to dbFormatImpl as {.nimcall.} #23

merged 1 commit into from
Jan 6, 2024

Conversation

metagn
Copy link
Contributor

@metagn metagn commented Jan 6, 2024

By default the type proc (s: string): string assumes {.closure.}, but the uses of dbFormatImpl only refer to implementations of dbQuote in each module which are good enough as {.nimcall.}.

Normally this doesn't matter, but if the rules of how template arguments are converted to argument types change to match normal procs in Nim in the future (as experimented in nim-lang/Nim#23176), the passed dbQuote will be treated as an implicit conversion from a {.nimcall.} proc into a {.closure.} proc, which causes the effect tracker to treat it as an arbitrary procvar and assume it causes RootEffect. This won't necessarily be a problem but we can guard against it for the time being.

By default the type `proc (s: string): string` assumes {.closure.}, but the uses of `dbFormatImpl` only refer to implementations of `dbQuote` in each module which are good enough as {.nimcall.}.

Normally this doesn't matter, but if the rules of how template arguments are converted to argument types change to match normal procs in Nim in the future (as experimented in nim-lang/Nim#23176), the passed `dbQuote` will be treated as an implicit conversion from a {.nimcall.} proc into a {.closure.} proc, which causes the effect tracker to treat it as an arbitrary procvar and assume it causes `RootEffect`. This won't necessarily be a problem but we can guard against it for the time being.
@Araq Araq merged commit 07a60d5 into nim-lang:devel Jan 6, 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.

2 participants