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

Where possible replace ::std with ::core in codegen #73

Merged
merged 3 commits into from
Nov 5, 2021
Merged

Where possible replace ::std with ::core in codegen #73

merged 3 commits into from
Nov 5, 2021

Conversation

dimpolo
Copy link
Contributor

@dimpolo dimpolo commented Oct 26, 2020

Partly resolves #72.

::alloc is more problematic since it requires a extern crate alloc;

@KodrAus
Copy link
Member

KodrAus commented Nov 14, 2020

We could add a alloc feature that's enabled by default that brings in the alloc crate so that we can use smart pointers like Arc and Box. If the alloc feature isn't enabled then we can support just & and &mut.

I actually don't think we have any need for full std in this crate!

@dimpolo
Copy link
Contributor Author

dimpolo commented Nov 14, 2020

How would you bring in the alloc crate?
One could split the crate into a proc-macro crate and one that re-exports alloc::boxed::Box, etc.
Then you could use something like ::auto_impl::Box in the generated code.

@KodrAus
Copy link
Member

KodrAus commented Jan 8, 2021

@dimpolo I think we might need to start wrapping the code we generate in a const so that we can extern crate in there, and determine within the macro (rather than the generated code) whether the path we're after comes from std or alloc.

So given an input like:

#[auto_impl(Arc)]
pub trait Trait {}

with the alloc feature instead of std, we might generate code that looks like:

pub trait Trait {}
const __AUTO_IMPL_TRAIT: () = {
    extern crate core;
    extern crate alloc;

    impl<T> Trait for alloc::sync::Arc<T> where T: Trait {}
};

We can look at that independently though unless you're keen to give it a go 🙂

@dimpolo
Copy link
Contributor Author

dimpolo commented Jan 8, 2021

Very keen in fact 😁
Luckily extern crate core; is not required.
Let me know if this is what you had in mind and if I should also fix the UI tests.

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to respond to this.

This seems like a useful change to me! Only thing I'm not yet sure about is whether we can release this as a minor version update. What if users have core or alloc somewhere in their root namespace (i.e. a module or a renamed crate?). Would that interfere?

I would also like to see some tests. Something that runs on CI which makes sure

  • This crate works with a no_std environment: one with alloc and one without.
  • That modules or crate renames don't interfere with our paths.

Could you still add those tests? That would be great! If you need any help or have questions about that, we can just discuss it here.

Oh and, I just fixed the UI tests in #74, so you can just rebase now.

@dimpolo
Copy link
Contributor Author

dimpolo commented Mar 4, 2021

Thanks for looking into this.

I'm not 100% sure but it seems as though things "just work".
Paths are correct and alloc doesn't need any special treatment.
A no_std library can use alloc freely and a binary would need to declare a #[global_allocator] first.

I believe a minor version update would be ok.


use auto_impl::auto_impl;

mod core {}
Copy link
Member

Choose a reason for hiding this comment

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

Is the idea of including these at the root here just to make sure we don't accidentally use module instead of crate paths to the standard library crates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that was the idea

bors bot added a commit to rust-embedded/embedded-hal that referenced this pull request Sep 28, 2021
310: Add blanket impls for references r=therealprof a=GrantM11235

I would prefer to use a proc-macro like [auto-impl](https://crates.io/crates/auto_impl), but that doesn't support no_std yet. (see auto-impl-rs/auto_impl#73)

Co-authored-by: Grant Miller <GrantM11235@gmail.com>
@rakita
Copy link

rakita commented Nov 2, 2021

It would be great if this can be merged, the change worked for my project.

@LukasKalbertodt
Copy link
Member

Sorry for the terribly slow response. This looks good to me! Only thing I noticed: const _ requires Rust 1.37. In our README, we say that auto impl requires Rust 1.30, so this PR will bump the MSRV, so I will release this with a major version bump, i.e. 0.5.0. But I don't think it's a problem. Rust 1.37 is fairly old.

@LukasKalbertodt LukasKalbertodt merged commit 902073b into auto-impl-rs:master Nov 5, 2021
@LukasKalbertodt
Copy link
Member

Released as 0.5.0.

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.

no_std support
4 participants