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

Implicit return working for async proc #20933

Merged
merged 5 commits into from
Dec 9, 2022

Conversation

ire4ever1190
Copy link
Contributor

Closes #11558

Uses an overloaded template to tell if the block has an implicit return or not

@ire4ever1190 ire4ever1190 marked this pull request as draft November 27, 2022 03:54
@ire4ever1190
Copy link
Contributor Author

Docs CI failing because of #20938

@Araq
Copy link
Member

Araq commented Nov 27, 2022

Please rebase.

@ire4ever1190 ire4ever1190 marked this pull request as ready for review November 27, 2022 20:57
tests/async/t11558.nim Outdated Show resolved Hide resolved
@Varriount Varriount added the Requires Araq To Merge PR should only be merged by Araq label Nov 28, 2022
let resultIdent = ident"result"
procBody.add quote do:
template setResult(x: `subRetType`) {.used.} =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've considered a similar thing in chronos so that nim-result can work better - status-im/nim-chronos#297 - perhaps we should coordinate the mechanism here - one thing that often comes up is symbol resolution issues - ie template lookup scope is confusing / broken and has a tendency to ruin unrelated code when used together with generics: status-im/nim-stew#161 - as a consequence, we've started using more "unique" names, also for "private" templates like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading those PR's I think this is a different problem to whats in them. This doesn't effect how its returned but just means that code like this can now compile and work (Doesn't effect how normal return and result = works)

# Not exactly a good async example, but just needed something basic
proc foo(x: int): Future[string] {.async.} =
  if x mod 2 == 0:
    "even"
  else:
    "odd"

But see what you mean about needing unique names. Maybe could make it very explicit like asyncDispatchSetResult?

@Araq Araq merged commit da3274d into nim-lang:devel Dec 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from da3274d

Hint: mm: orc; opt: speed; options: -d:release
165547 lines; 7.989s; 612.16MiB peakmem

@arnetheduck
Copy link
Contributor

@Araq this needed a more unique name for the template

@ire4ever1190 ire4ever1190 deleted the implicit-async-return branch December 9, 2022 21:46
@ire4ever1190
Copy link
Contributor Author

Sorry my fault, I forgot to rename template.
Will make followup PR asap to fix

arnetheduck added a commit to status-im/nim-chronos that referenced this pull request Jan 19, 2023
Based on nim-lang/Nim#20933

* simplify body generation code to use `quote do`
* clean up some variables and assignments
survivorm pushed a commit to survivorm/Nim that referenced this pull request Feb 28, 2023
* Implicit return working for asyncdispatch proc

Closes nim-lang#11558

* Test case

* Test that return value is actually used

* Update tests/async/t11558.nim

Co-authored-by: Andreas Rumpf <[email protected]>
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
* Implicit return working for asyncdispatch proc

Closes nim-lang#11558

* Test case

* Test that return value is actually used

* Update tests/async/t11558.nim

Co-authored-by: Andreas Rumpf <[email protected]>
arnetheduck added a commit to status-im/nim-chronos that referenced this pull request May 11, 2023
Based on nim-lang/Nim#20933

* simplify body generation code to use `quote do`
* clean up some variables and assignments
arnetheduck added a commit to status-im/nim-chronos that referenced this pull request May 23, 2023
Based on nim-lang/Nim#20933

* simplify body generation code to use `quote do`
* clean up some variables and assignments
arnetheduck added a commit to status-im/nim-chronos that referenced this pull request May 23, 2023
arnetheduck added a commit to status-im/nim-chronos that referenced this pull request Jun 4, 2023
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
* Implicit return working for asyncdispatch proc

Closes nim-lang#11558

* Test case

* Test that return value is actually used

* Update tests/async/t11558.nim

Co-authored-by: Andreas Rumpf <[email protected]>
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.

Implicit return in {.async.} proc causes compile error
4 participants