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

[WIP] fix #20 improve test support for no_std #24

Closed
wants to merge 1 commit into from

Conversation

mattsse
Copy link

@mattsse mattsse commented May 30, 2020

This PR is part of the work to fix broken tests in no_std. (#20)

So far ea5a088 includes changes to make eyre compile successfully when --no-default-feature is used.
Changes made:

  • replaced std::fmt::Display with score::fmt::Display
  • unified the two Chain structs into single Chain both for std and no_std. This was necessary because the crate::chain::Chain was only pub(crate) for no_std.
  • added some docs and removed unused to make clippy happy.

To fix all the tests for no_std is a lot more work, since all the tests using io::Error won't work.
To add test support no_std, I could start with replacing the io::Error dependent variants in the TestError with something that supports no_std? This could minimize test duplication for no_std

Before I proceed I'd like to get some feedback and I'm happy to hear suggestions.

@yaahc
Copy link
Collaborator

yaahc commented May 30, 2020

Oh my god when you said score::fmt::Debug I was terrified, loool, i thought you were pulling in some dependency for a new fmt replacement, but this is much better. I've only looked through half of this so far but initial impressions are good, ty for taking this on! And I know the io::Error stuff is a pain in the ass, I tried this once before and eventually just reverted it rather than pushing it to completion.

iirc in no_std eyre doesn't have any way to be constructed from an existing error, it can only be used to construct adhoc error types either via the macros or via wrap_err, so I'm not sure if the TestError approach will work but if you see something that I missed then feel free to go for it. If that doesn't work out I think its okay to just use cfg to disable all of the non-applicable tests in no_std.

I'll go ahead and finish the review now and leave comments, just had to stop for a second to laugh about score vs core.

@yaahc
Copy link
Collaborator

yaahc commented May 30, 2020

Also someone else opened a PR to do the std -> core change 6 days ago and I missed it / forgot about it, so I'm gonna merge that PR right after it passes tests, so just a heads up you'll need to rebase, I know it's a minor inconvenience but I feel like just closing their PR would be a little rude 😬

@@ -342,8 +342,8 @@ mod alloc {
#[cfg(feature = "std")]
pub(crate) use std::boxed::Box;

#[cfg(not(feature = "std"))]
pub(crate) use alloc::string::String;
// #[cfg(not(feature = "std"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think just remove this rather than commenting it out, and the one below if you can.

@@ -729,6 +730,7 @@ where
unsafe { &mut *(self.vtable.object_mut)(self) }
}

#[cfg(feature = "std")]
Copy link
Collaborator

@yaahc yaahc May 30, 2020

Choose a reason for hiding this comment

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

I don't think we can make this change because this would mean that the Item type of Chain changes when we use no_std mode. This would cause disabling / enabling features to become a breaking change which is a no-go. Did you need the chain interface on no_std? If so we can definitely work something out.

Copy link
Author

@mattsse mattsse May 30, 2020

Choose a reason for hiding this comment

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

I added the cfg here because the only usage of the ErrorImpl::chain function is here: https://github.com/yaahc/eyre/blob/a197feb4e3368f70735a2fe34990fbebb10487cd/src/error.rs#L305-L307 and is marked as std only. In addition ErrorImpl::chain is pub crate making it dead code. It compiles without #[cfg(feature = "std")], but clippy rightfully complaints about dead code without this change:

❯ cargo check --no-default-features                                                                                                                                                                         ⇡!? master 
   Compiling eyre v0.4.2 (/Users/Matthias/git/rust/eyre)
warning: associated function is never used: `chain`
   --> src/error.rs:732:19
    |
732 |     pub(crate) fn chain(&self) -> Chain<'_> {
    |                   ^^^^^
    |

@mattsse
Copy link
Author

mattsse commented May 30, 2020

:) sorry I scared you with that typo. Unfortunately score is already taken, I checked...

I did more research on how to make cargo test --no-default-features succeed for the doctests first.

There seems to be two options:

  1. the experimental doc_cfg, which would require adding !#[feature(doc_cfg)] to lib.rs
  2. wrapping the failing doc tests in #[cfg_attr(feature = "std", doc = r##...##")]

I tried 2., there are atm 11 doc tests that fail. Wrapping them in #[cfg_attr(feature = "std", doc = r##...##")] works fine but is kinda weird. So far I fixed only a couple and would like to know what you think about that before I proceed with this cumbersome task -.-.

For https://github.com/yaahc/eyre/blob/a197feb4e3368f70735a2fe34990fbebb10487cd/src/chain.rs#L21-L40
It would look like:

    /// Construct an iterator over a chain of errors via the `source` method
    ///
    #[cfg_attr(feature = "std", doc = r##"
     # Example

     ```rust
     use std::error::Error;
     use std::fmt::{self, Write};
     use eyre::Chain;
     use indenter::indented;

     fn report(error: &(dyn Error + 'static), f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let mut errors = Chain::new(error).enumerate();
         for (i, error) in errors {
             writeln!(f)?;
             write!(indented(f).ind(i), "{}", error)?;
         }

         Ok(())
     }
     ```
    "##)]

@yaahc
Copy link
Collaborator

yaahc commented May 30, 2020

I'd prefer option 1, there are ways around the experimental aspect I think, I've done this same thing in tracing and already opened an issue yesterday to use doc_cfg in eyre.

@yaahc yaahc self-assigned this Jun 4, 2020
@yaahc
Copy link
Collaborator

yaahc commented Dec 15, 2020

hey @mattsse, sorry for ignoring this PR for so long, and thank you again for the effort you put into it. I just realized that this PR is no longer something we can merge because I removed support for no_std a while ago (and apparently forgot??). I'm really sorry about the confusion.

@yaahc yaahc closed this Dec 15, 2020
@mattsse
Copy link
Author

mattsse commented Dec 15, 2020

no worries @yaahc, I never quite finished it and then also kinda forgot about it  🙈
Thanks for closing!

pksunkara pushed a commit that referenced this pull request Oct 11, 2023
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