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

Unwrap macro #273

Merged
merged 3 commits into from
Nov 26, 2020
Merged

Unwrap macro #273

merged 3 commits into from
Nov 26, 2020

Conversation

Dirbaio
Copy link
Contributor

@Dirbaio Dirbaio commented Nov 24, 2020

This is a PoC of the unwrap! macro discussed in #161, to see how well it works.

Seems to work great, line numbers are printed correctly. The only drawback is the syntax is less nicer than the normal unwrap() since it doesn't read left-to-right.

Ignore the Escape braces in assert expressions. commit, it's included so the unwrap macro can use the escape_expr function.

@Dirbaio Dirbaio marked this pull request as draft November 24, 2020 20:07
@japaric
Copy link
Member

japaric commented Nov 25, 2020

Thanks for building a PoC.
I tested out locally and I liked that unwrap!(something) prints "something" to the console. core::unwrap doesn't do that in comparison.
My biggest concern, though, is that the macro "turns off" Rust-Analyzer so if I write a long method chain:

unwrap!(x.
    map(|z| z + 1).
    then(something)
)

I don't get type annotations next to each method call or in internal closures.

To work around the issue I would split the chain into its own variable:

let res = x.
    map(|z| z + 1).
    then(something);
unwrap!(res)

But then I won't get the chain expression printed in the panic message (if the unwrap fails) and the reported location will be sort of "off by one line" (but still be more precise than with the .dewrap() approach)

@japaric
Copy link
Member

japaric commented Nov 25, 2020

@Dirbaio the team discussed how we'd like to implement this feature and we are OK with a macro implementation so I'll review this PR once you mark it as ready for review.

@japaric japaric self-assigned this Nov 25, 2020
@Dirbaio
Copy link
Contributor Author

Dirbaio commented Nov 25, 2020

DOne :D

@Dirbaio Dirbaio marked this pull request as ready for review November 25, 2020 18:13
@Dirbaio
Copy link
Contributor Author

Dirbaio commented Nov 25, 2020

I don't know what's up with the unused import: crate as defmt from CI. If I remove it it won't build because it's needed.

Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

Looks great!

I don't know what's up with the unused import: crate as defmt from CI.

I think that's because internp! requires it when target = x86 but not when target = ARM. Sticking an allow on it would be an OK fix.

.into()
}

fn ident_expr(name: &str) -> Expr {
Copy link
Member

Choose a reason for hiding this comment

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

nice refactor 👍

@japaric japaric merged commit 9f97c1f into knurling-rs:main Nov 26, 2020
@Lotterleben Lotterleben mentioned this pull request Nov 26, 2020
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