Skip to content

Fill match arms replaces whole impl block with proc macro attribute #10573

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

Closed
azerupi opened this issue Oct 18, 2021 · 12 comments
Closed

Fill match arms replaces whole impl block with proc macro attribute #10573

azerupi opened this issue Oct 18, 2021 · 12 comments
Labels
A-assists Broken Window Bugs / technical debt to be addressed immediately C-bug Category: bug S-actionable Someone could pick this issue up and work on it right now

Comments

@azerupi
Copy link

azerupi commented Oct 18, 2021

I have some code that I unfortunately can't share where the "fill match arms" replaces a whole impl block.

I think it is related to the fact that I use a proc-macro attribute:

#[payload]
impl Payload {
    #[write(address = 0, length = 1)]
    fn enable_front_led(&mut self, data: U8) -> Result<(), u8> {
        // ...
    }
    
    // ...

   fn test(&mut self) {
        match self.trigger_mode {
            
        }
    }
}

When I use the "fill match arms" action it replaces the whole impl block and I am left with this:

{
    TriggerMode::Off => todo!(),
    TriggerMode::Laser => todo!(),
    TriggerMode::LedPattern(_) => todo!(),
    TriggerMode::Torch => todo!(),
}

This might be a recent regression, I don't remember having any problems before 2 or 3 weeks ago. I have been trying to reproduce this with a small self-contained example but haven't been able to yet. I'll see if I can find some example code that triggers this behavior so that I can share it.

In the meantime, if I can provide any more useful information let me know.

@Veykril Veykril added A-assists Broken Window Bugs / technical debt to be addressed immediately C-bug Category: bug S-actionable Someone could pick this issue up and work on it right now labels Oct 18, 2021
@lnicola
Copy link
Member

lnicola commented Oct 18, 2021

This might be a recent regression, I don't remember having any problems before 2 or 3 weeks ago.

That's when we enabled attribute proc macros. You can still disable them to work around this issue.

@Veykril
Copy link
Member

Veykril commented Oct 18, 2021

Can you run the Expand macro recursively(ctrl + shift + p for the command input in VSCode) command on the impl block and show the output of that? I assume we fail to map out of the macro expansion here falling back to the entire invocation which we then replace.

@azerupi
Copy link
Author

azerupi commented Oct 18, 2021

That's when we enabled attribute proc macros. You can still disable them to work around this issue.

Yes, disabling attribute proc macros produces the expected behavior for this action.

I've trimmed down our code and I can reproduce it with this minimal impl block:

#[payload]
impl Payload {
    fn test(&mut self) {
        match self.trigger_mode {
            
        }
    }
}

This expands to the following:

// Recursive expansion of payload! macro
// ======================================

impl Payload {
  fn test(&mut self){
    match self.trigger_mode{}
    
  }
  fn handle_frame(&mut self,frame: &Frame) -> Result<(),Error>{
    use loki_payload::crsf::types::FramePayload;
    match frame.payload(){
      FramePayload::Ping(c) => self.com.handle_ping_request(*c),
      FramePayload::Type => self.com.send_frame(&Frame::new_type_response(Payload::id(),Payload::version(),Payload::features(),)),
      FramePayload::Authenticate(data) => self.com.handle_authenticate_request(data,Payload::id(),Payload::version(),Payload::features(),),
      _ => {
        Err(loki_payload::error::Error::UnknownFrameType(frame.type_id()))
      }
      
    }
  }
  
}

@Veykril
Copy link
Member

Veykril commented Oct 18, 2021

Ye looks like we fail to map the MatchArmList out of the expansion here, so this problem exists in more assists than just this one I imagine 😬.

@azerupi
Copy link
Author

azerupi commented Oct 18, 2021

Just for my personal understanding. You are saying that rust-analyzer fails to map the match statement before expansion to the one after expansion?

If I wanted to poke around in the rust-analyzer code about this issue, do you know where I should start looking?

@Veykril
Copy link
Member

Veykril commented Oct 18, 2021

The problem is that we find the match expression in the expanded code, we then want to map it back up to the text range of the original one so we edit it, but we fail in doing so for some reason. And currently if we fail this mapping we fall back to the entire macro invocation, which in this case is the impl with the attribute.

The part that tries to get the correct range is this here,
https://github.com/rust-analyzer/rust-analyzer/blob/5704de66c271b35ffa6f22aa0b3f8f3f50f7f561/crates/ide_assists/src/handlers/add_missing_match_arms.rs#L174
for starters this should be a original_range_opt call instead and in case of None we shouldn't emit the assist at all as we don't know what to replace.
Figuring out why we fail to map up here is somewhere in the call hierarchy of those functions, I don't remember the specifics how we do this though.

@azerupi
Copy link
Author

azerupi commented Oct 19, 2021

Thanks for the details 🙂
I've managed to trim down the code to have a small example I can share. The code in this repository produces the bug for me when I try to use the "fill match arms" assist on the empty match arm.

https://github.com/azerupi/ra-fill-match-arms-bug

@azerupi
Copy link
Author

azerupi commented Oct 19, 2021

This problem seems to appear when using the syn and quote crate.

This works

#[proc_macro_attribute]
pub fn payload(_args: TokenStream, input: TokenStream) -> TokenStream {
    input
}

But this does not work

#[proc_macro_attribute]
pub fn payload(_args: TokenStream, input: TokenStream) -> TokenStream {
    let item = parse_macro_input!(input as Item);

    let output = quote! {
        #item
    };

    output.into()
}

@Veykril
Copy link
Member

Veykril commented Oct 25, 2021

This actually already works with rustc 1.56. It will work with other rustc versions in the next release. Problem was us not shipping some span method implementations for other proc-macro abis. cc #10378

@Veykril Veykril closed this as completed Oct 25, 2021
@azerupi
Copy link
Author

azerupi commented Oct 25, 2021

Ah good to hear 👍
So next monday everything should work?

@Veykril
Copy link
Member

Veykril commented Oct 25, 2021

Yep, next monday(tomorrow nightly) or if you run rustc 1.56 now.

@azerupi
Copy link
Author

azerupi commented Oct 25, 2021

Excellent, thanks a lot! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-assists Broken Window Bugs / technical debt to be addressed immediately C-bug Category: bug S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

No branches or pull requests

3 participants