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

API, Rustc: Add AwaitExpr and support async blocks and fns #197

Merged
merged 11 commits into from
Aug 3, 2023

Conversation

xFrednet
Copy link
Member

Basically, what the title says :D


Async and await stuff in rustc heavily relies on syntax sugar. A simple .await call is transformed into one loop and two match statements etc. This far Marker has always resugared this syntax, for two reasons:

  1. I believe that the exact desugar of expressions is allowed to change.
  2. The desugar is often harder to understand.

In this case, I've decided to also resugar the expressions. For .await expressions, I'm sure that this is the right call. However, async blocks and functions might be different. For example:

async fn foo() -> u8 {
	16
}

The function actually has the return type impl Future<Output = u8> and not u8 like the AST node currently says. The impl Future<Output = u8> can still be retrieved as the semantic type of the expression.

The basic question I currently have is: Is resugaring the output of the function to u8 okay, or should it still say impl Future<Output = u8> even if the user didn't write it? Personally, I like this resugar, but I haven't worked too much with async.


@Veetaha you said previously that you work with async. Do you maybe have any thoughts on this? (I hope it's okay that I ping you, if not, please tell me, and I will refrain from it in the future)


Closes #174

@xFrednet xFrednet added C-enhancement Category: New feature or request A-api Area: Stable API D-rustc-driver Driver: Rustc Driver S-waiting-on-review Status: Awaiting review labels Jul 25, 2023
@xFrednet xFrednet force-pushed the 174-async-artificial-sweetening branch 2 times, most recently from 30dc508 to 919081a Compare July 26, 2023 12:31
Copy link
Contributor

@Veetaha Veetaha left a comment

Choose a reason for hiding this comment

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

I don't mind if you ping me for guidance/discussions. I don't promise that I respond quickly or respond at all, but will try my best

marker_rustc_driver/src/conversion/marker/expr.rs Outdated Show resolved Hide resolved
Comment on lines 439 to 453
if let hir::ExprKind::Block(block, None) = block_expr.kind {
let api_block_expr;
let capture_kind = self.to_capture_kind(closure.capture_clause);
super::with_body!(self, body_id, {
api_block_expr = self.to_block_expr(
CommonExprData::new(self.to_expr_id(block_expr.hir_id), self.to_span_id(block_expr.span)),
block,
None,
Syncness::Async,
capture_kind,
);
});
return ExprKind::Block(self.alloc(api_block_expr));
}
unreachable!("`async` block desugar always has the same structure")
Copy link
Contributor

Choose a reason for hiding this comment

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

I love early exits because they remove the nesting of code. This could instead be writen like so:

let hir::ExprKind::Block(block, None) = block_expr.kind else {
    unreachable!("`async` block desugar always has the same structure"); 
};

// The rest of the code here one level closer to the left side of the screen

I know rust linting is a pretty deterministic process. If this code was part of a distributed system where flaky random glitches are possible I'd recommend putting as much info in the panic/error message as possible, because reproducing a problem may not be realistically possible

Copy link
Member Author

Choose a reason for hiding this comment

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

I often find let else statements less readable. It would be cool if if-let chains would be stabilized, but they sadly disable rustftm for the function

marker_rustc_driver/src/conversion/marker/item.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member Author

I don't mind if you ping me for guidance/discussions. I don't promise that I respond quickly or respond at all, but will try my best

Thank you very much! That you provided feedback on the code was already way more than I expected. I really appreciate it ❤️

@xFrednet xFrednet added this to the v0.2.0 milestone Jul 31, 2023
@xFrednet xFrednet force-pushed the 174-async-artificial-sweetening branch from 919081a to 0bd55d7 Compare August 1, 2023 12:09
@xFrednet
Copy link
Member Author

xFrednet commented Aug 1, 2023

I rebased and addressed the review comments this should be ready now.

@xFrednet
Copy link
Member Author

xFrednet commented Aug 3, 2023

I believe it's safe to r+ this :)

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 3, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 2bf48e1 into rust-marker:master Aug 3, 2023
5 checks passed
@xFrednet xFrednet deleted the 174-async-artificial-sweetening branch August 3, 2023 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-api Area: Stable API C-enhancement Category: New feature or request D-rustc-driver Driver: Rustc Driver S-waiting-on-review Status: Awaiting review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support async blocks and await expressions
2 participants