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

fix(rust): fix no_std compilation due to ockam::node attribute #2355

Conversation

conectado
Copy link
Contributor

@conectado conectado commented Dec 14, 2021

While working in #2213 I found that the compilation of no_std examples were failing due to problems with the ockam::node attribute

Current Behaviour

When doing:

cd implementations/rust/ockam/ockam_examples/example_projects/no_std/
cargo run --example hello --no-default-features --features="alloc, no_std"

I got the error:

error[E0308]: `?` operator has incompatible types
  --> examples/hello.rs:45:1
   |
45 | #[ockam::node]
   | ^^^^^^^^^^^^^^ expected enum `Result`, found `()`
   |
   = note: `?` operator cannot convert from `()` to `Result<(), ockam::Error>`
   = note:   expected enum `Result<(), ockam::Error>`
           found unit type `()`
   = note: this error originates in the attribute macro `ockam::node` (in Nightly builds, run with -Z macro-backtrace for more info)
help: try removing this `?`
   |
45 - #[ockam::node]
45 + #[ockam::node
   | 
help: try using a variant of the expected enum
   |
45 | Ok(#[ockam::node])
   |

For more information about this error, try `rustc --explain E0308`.
error: could not compile `hello_ockam_no_std` due to previous error

The same happened when running the example with any of the feature flags corresponding to no_std

Proposed Changes

The reason for this is that the node attribute returns the result from Executor::execute and it either unwraps the result or attach a ? depending on the return type of main.

This works well for std where the return type of execute is Result<F::Output> where F::Output is the return type of main, so if the return type of main is Result<()> the return type of execute is Result<Result<()>> and applying the ? operator we get a Result<()>.

However, for no_std the return type of execute is Result<()> so applying the ? operator to Result<()> we get a () which doesn't match with the return type of a main Result<()>, so in the no_std case I simply changed it to let the attribute return the result from execute verbatim.

This also means that Ockam::node in no_std will only work with no return type or Result<()> but since there is a TODO for execute to be unified with the std case that'll be solved, there's also no option to make it compatible with any other return type since the execute function as is now consume any type returned from main by unwrapping it and returning Ok(())

Checks

Compilation with no_std was failing when main had any return type because of `Executor::execute`
signature in no_std. This change fixes it by letting the `Executor::execute` return value bubble up.
Copy link
Contributor

@antoinevg antoinevg left a comment

Choose a reason for hiding this comment

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

Great catch, thank you!

Copy link
Contributor

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

LGTM too. We might eventually want to document the restriction of requiring Result returns for no_std main type, or perhaps do the same thing for std-using. Either way, that can probably wait, as realistic uses will use Result anyway.

@mrinalwadhwa
Copy link
Member

Merged in #2364

@conectado Thank you and congratulations on your first Ockam contribution! 🎉

@mrinalwadhwa mrinalwadhwa changed the title chore(rust): fix no_std compilation due to ockam::node attribute fix(rust): fix no_std compilation due to ockam::node attribute Dec 17, 2021
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.

4 participants