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

Expose derive macro (for better Rust-Analyzer compatibility) #320

Closed
wants to merge 6 commits into from

Conversation

nightkr
Copy link

@nightkr nightkr commented Feb 25, 2021

Fixes #319

With this patch Rust-Analyzer will give proper suggestions iff you use #[derive(PinProject)] and you rename the projections that you use (#[pin(project = FooProj, project_ref = FooProjRef)). The existing #[pin_project] macro still exists unchanged.

  • Expose derive macro
  • Make item-level #[pin] attribute optional (not an issue before, since this was always generated when using #[pin_project])
  • Move projection functions for renamed projections out of the expansion-private scope
  • (If the PR looks good otherwise) Shift documentation and tests to use the derive macro.

@nightkr nightkr requested a review from taiki-e as a code owner February 25, 2021 10:54
Comment on lines +986 to +988
let allowed_lints = global_allowed_lints();
quote! {
#allowed_lints
Copy link
Author

Choose a reason for hiding this comment

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

The global lint allowlist is normally only applied to items in the private scope.

@@ -910,6 +912,7 @@ fn make_drop_impl(cx: &Context<'_>) -> TokenStream {
/// On enums, only methods that the returned projected type is named will be generated.
fn make_proj_impl(
cx: &Context<'_>,
expose: bool,
Copy link
Author

Choose a reason for hiding this comment

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

It's a bit hacky to have to run make_proj_twice, might be better to make a single-use struct and just return both cases at once?

quote! {
#allowed_lints
Copy link
Author

Choose a reason for hiding this comment

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

This does not generate a #[allow(dead_code)], which is also applied globally to the private scope. IMO, renaming the projection is a pretty clear signal that you're planning to use it, and silencing the warning is a false negative in this case.

Copy link
Author

Choose a reason for hiding this comment

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

The CI failures are about these lints. Leaving them until a consensus has been reached on where to go about that.

Copy link
Owner

Choose a reason for hiding this comment

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

Basically, the warning is not issued to the method itself even if no #[allow(dead_code)] because fn project() has call-site span. The problem is the macro_rules macro changes the span (rust-lang/rust#77973).

I would prefer to use #[allow(dead_code)] because it's preferable to work the same as when it's not called inside macros.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, tried to get the spans to make sense but couldn't get it to cooperate, so I added the #[allow].

@nightkr nightkr changed the title Improve Rust-Analyzer compatibility Expose derive macro (for better Rust-Analyzer compatibility) Feb 25, 2021
@taiki-e
Copy link
Owner

taiki-e commented Feb 25, 2021

Thanks for the PR!

First, under what situation and what improvements does this bring?
I checked out this branch locally, replaced all #[pin_project] usage with #[derive(PinProject)], ran cargo clean to remove cache, and restarted RA, but nothing useful like annotations or autocomplete.
I have set "rust-analyzer.procMacro.enable": true and the projection types were named, but did I miss anything?

Second, I agree with it's preferable to be compatible with RA, but AFAIK few people actually names the projection type and the API generated by pin-project is very simple, so I'm currently feeling this doesn't bring an advantage beyond the user confusion of having two same API.

@nightkr
Copy link
Author

nightkr commented Feb 25, 2021

I have set "rust-analyzer.procMacro.enable": true

You also need to set "rust-analyzer.cargo.loadOutDirsFromCheck": true for proc macros to work. You also need to restart Rust-Analyzer every time the macro definition changes (but macro call sites are still updated as soon as they're edited, this only affects developing pin-project itself).

After that it should look something like this (with LSP inlays and completions enabled):

image

This PR also allows RA's go-to-definition and (to a degree) refactoring features to cross projection boundaries. For example, go-to-definition at the this.pinned reference jumps to the field in the struct. Renaming this.pinned also renames the struct field (although for some reason the reverse doesn't seem to work).

Second, I agree with it's preferable to be compatible with RA, but AFAIK few people actually names the projection type and the API generated by pin-project is very simple,

The API generated by pin-project itself, sure, the important thing IMO is completion for the stuff contained inside those structs.

As someone who spends a fair amount of time writing custom Future and Stream combinators, for example, it's pretty annoying to give up RA whenever I'm touching them. My current workaround is to use let to type-hint every field that my combinator uses, but that gets pretty unwieldy quickly:

#[pin_project]
struct Map<T, F> {
    #[pin]
    inner: T,
    map: F,
}

impl<T: Future, U, F: FnMut(T::Output) -> U> Future for Map<T, F> {
    type Output = U;

    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<U> {
        let this = self.project();
        let inner: Pin<&mut T> = this.inner;
        let map: &mut F = this.map;
        Poll::Ready(map(ready!(inner.poll(cx))))
    }
}

so I'm currently feeling this doesn't bring an advantage beyond the user confusion of having two same API.

#[derive(PinProject)] should cover all cases that #[pin_project] does, so the latter can just be deprecated and kept around as a wrapper (as it already is).

@Aaron1011
Copy link
Collaborator

I don't feel very comfortable with the idea of modifying the public API solely to support rust-analyzer. #[derive(PinProject)] would typically imply that we're implementing a trait called pin-project.

Additionally, wouldn't every single crate that defines an attribute macro need to do something like this? I'm sympathetic to the issue of missing completions, but entirely avoiding a Rust feature doesn't seem like the best suction. Can you elaborate on what's blocking RA from expanding attribute macros?

@nightkr
Copy link
Author

nightkr commented Feb 25, 2021

I don't feel very comfortable with the idea of modifying the public API solely to support rust-analyzer. #[derive(PinProject)] would typically imply that we're implementing a trait called pin-project.

Not sure here. For example, Snafu was a derive macro since way before Rust-Analyzer supported proc macros.

Additionally, wouldn't every single crate that defines an attribute macro need to do something like this?

The only libraries that I use regularly that use attribute macros are Tokio and pin-project. Tokio's attribute macros don't add any new API surface (items or impls), so they aren't affected by this problem anyway.

I'm sympathetic to the issue of missing completions, but entirely avoiding a Rust feature doesn't seem like the best suction. Can you elaborate on what's blocking RA from expanding attribute macros?

I'm not too familiar with RA's internals, but as far as I understand it, derive macros are easier to implement because they are guaranteed to be purely additive (can't change the annotated item itself, only generate new items), and are independent from each other (they can't see or interact with each other during the expansion process).

IMO it makes sense to commit to (and implicitly document) those constraints, even if RA wasn't a factor (although admittedly I probably wouldn't have bothered to submit a PR in that case.. :P).

Base automatically changed from master to main March 24, 2021 17:02
Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

The rust-analyzer team seems to be starting to work on support for attribute proc-macros (rust-lang/rust-analyzer#8486), so I would prefer to wait for it instead of adding the API on the pin-project side for now.

@taiki-e taiki-e closed this Apr 17, 2021
@taiki-e taiki-e mentioned this pull request Apr 17, 2021
5 tasks
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.

Rust-Analyzer compatibility
3 participants