taiki-e / pin-project-lite

A lightweight version of pin-project written with declarative macros.
https://docs.rs/pin-project-lite
Apache License 2.0
226 stars 16 forks source link

missing_debug_implementations does not lint on pin projected type #52

Closed Kestrer closed 3 years ago

Kestrer commented 3 years ago

The following code compiles without warning or error:

#![forbid(missing_debug_implementations)]

pin_project_lite::pin_project! {
    pub struct X {
        inner: ()
    }
}

Would there be any way to prevent it from compiling? I have many futures and I would like to implement Debug on them, but currently I have to go through and check them all manually.

taiki-e commented 3 years ago

Thanks for the report.

I think this is due to the pin_project macro doesn't preserve the original visibility's span. In the following line, pub and struct's spans are replaced with call site span.

https://github.com/taiki-e/pin-project-lite/blob/77b255270933ab5ef5483719e72576b062eb49ac/src/lib.rs#L1439

This should be able to fix by adding a step to separate visibility for parsing and visibility for code-generation.

FYI: I recently found the opposite behavior on pin-project and reported it to rustc as a bug. This comment on that issue explains why these problems occurs.

taiki-e commented 3 years ago

I tried to fix this (https://github.com/taiki-e/pin-project-lite/commit/4102899f93be3f4a12b842166d1a3c77e25f8471), but missing_debug_implementations uses the span of the whole struct (to be precise: the span of start and end), so to fix this problem we need to preserve the span of visibility and brace. This seems to be very difficult because there is no token type that can match a brace in declarative macros.

error: type does not implement `Debug`; consider adding `#[derive(Debug)]` or a manual implementation
   --> tests/test.rs:LL
    |
LL  | /     pub struct S {
LL  | |         f: (),
LL  | |     }
    | |_____^
    |

If you want to fix this problem, I think you will need to change the span used by missing_debug_implementations on the rustc side.

Kestrer commented 3 years ago

Well that's a shame :(. I don't see an easy way around it.

We could investigate the possibility of adding a small proc-macro dependency that doesn't use proc-macro2, quote or syn. It wouldn't affect build times too much; on my machine an empty proc macro crate increases it from 0.4s to 0.9s, where pin-project takes 15s. Another possible reason to do this would be that it could allow for the nicer syntax of an attribute. Although maybe this idea is best kept for a different crate.

taiki-e commented 3 years ago

We could investigate the possibility of adding a small proc-macro dependency that doesn't use proc-macro2, quote or syn.

Well, I have one such crate, and am working on such a thing for another crate as well, so it is probably possible to look into it. (Aside from whether I have the time to look it)

Another possible reason to do this would be that it could allow for the nicer syntax of an attribute.

Do you mean current #[project = ...] syntax?

Although maybe this idea is best kept for a different crate.

Yes.

on my machine an empty proc macro crate increases it from 0.4s to 0.9s, where pin-project takes 15s.

Btw, this is a clean build, right? Since the dependencies are only compiled the first time, I personally don't really understand why people who dislike proc-macro deps are always so concerned about the initial cost. (Also, even if we don't depend on proc-macro in the main build, we should often depend on it during development for testing and so on.)

taiki-e commented 3 years ago

FWIW, if you just want to check if debug is implemented, it might be easier to create a codegen that generates something like assert_debug based on the codegen used in my project. (If you don't dislike writing proc-macro.)

taiki-e commented 3 years ago

Closing (blocked-closed) -- It's impossible to fix this on the pin-project-lite side at this time. We can reopen the issue again once someone starts a discussion in rust-lang/rust about the span of missing_debug_implementations and the changes have been applied