dtolnay / async-trait

Type erasure for async trait methods
Apache License 2.0
1.81k stars 84 forks source link

Non-hygienic expansion tied to Box<...> and Box::pin(...) #163

Closed chipsenkbeil closed 3 years ago

chipsenkbeil commented 3 years ago

When attempting to assert that my own macro crates are hygienic, I use a test file along the lines of

#![no_implicit_prelude]

#[::my_crate::my_attr_macro]
struct SomeStruct {
    field1: ::std::primitive::u64,
    // ...
}

It looks like expand.rs at line 288 isn't using the full path to Box:

::core::pin::Pin<Box<
    dyn ::core::future::Future<Output = #ret> + #bounds
>>

Also looks like Box::pin is being used without a full path at expand.rs at line 385:

Box::pin(async move { #let_ret })
dtolnay commented 3 years ago

This is intentional. It'll expand against whatever Box type is in scope. Typically that would be the prelude's in the case of crates that use std, but could also be an import from alloc if not using std.

chipsenkbeil commented 3 years ago

This is intentional. It'll expand against whatever Box type is in scope. Typically that would be the prelude's in the case of crates that use std, but could also be an import from alloc if not using std.

@dtolnay, this prevents folks from having a completely hygienic macro crate as they'll have to import Box themselves. Can this be documented somewhere? I had to dig through async-graphql's codebase to figure out it was async-trait that was non-hygienic.

dtolnay commented 3 years ago

"Completely hygienic" is overrated in my experience. Is this an obstacle to anything concretely useful beyond chasing completely-hygienicness in some other macro?

To answer your question, I would not want to dedicate documentation to this. Documentation is a brutal tradeoff in that the more you write the less anyone reads. It has to focus on globally useful information; meanwhile something like this is adequately discovered via search or looking at the code for the niche set of people it affects, so the bar is not met for the cost of documentation to be worth it.

chipsenkbeil commented 3 years ago

"Completely hygienic" is overrated in my experience. Is this an obstacle to anything concretely useful beyond chasing completely-hygienicness in some other macro?

To answer your question, I would not want to dedicate documentation to this. Documentation is a brutal tradeoff in that the more you write the less anyone reads. It has to focus on globally useful information; meanwhile something like this is adequately discovered via search or looking at the code for the niche set of people it affects, so the bar is not met for the cost of documentation to be worth it.

@dtolnay , makes sense to me. There isn't any major obstacle that warrants the change and the rational behind documentation is okay given there are some issues (like this one) that highlight the gotcha.

Appreciate the transparency 😄