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

inline assist not working properly on Hash::hash #16471

Open
antonilol opened this issue Feb 1, 2024 · 12 comments
Open

inline assist not working properly on Hash::hash #16471

antonilol opened this issue Feb 1, 2024 · 12 comments
Labels
C-bug Category: bug

Comments

@antonilol
Copy link

antonilol commented Feb 1, 2024

rust-analyzer version: 0.4.1810-standalone

rustc version: rustc 1.75.0 (82e1608df 2023-12-21)

relevant settings: None

with this code,

pub struct MyStruct {
    value: u64,
}

impl std::hash::Hash for MyStruct {
    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
        self.value.hash(state);
    }
}

impl MyStruct {
    pub fn as_u64(&self) -> u64 {
        self.value
    }
}

place the cursor on hash in self.value.hash(state); and apply the inline function assist

this is the result:

...
        {
            let this = &self.value;
            let state: &mutH = state;
          state.write_u64(*self)
        };
...
  • &mutH should be &mut H
  • *self should be *this
  • the first two lines are indented with 4 spaces (normal), but state.write_u64(*self) only with 2, this is automatically fixed with the formatter but still doesnt look nice

the cause could be that the Hash impl for u64 is macro generated, but i dont know exactly

@antonilol antonilol added the C-bug Category: bug label Feb 1, 2024
@evertedsphere
Copy link
Contributor

evertedsphere commented Feb 2, 2024

I'd like to work on this ticket; it seems simple enough for a starter issue. I have a test in ide-assists that reproduces the bug.

@evertedsphere
Copy link
Contributor

evertedsphere commented Feb 2, 2024

Leaving the formatting issue aside for now, this does not reproduce either of the remaining issues:

fn _write_u64(s: &mut u64, x: u64) {
    *s += x;
}
fn _hash_nonmacro(inner_self_: &u64, state: &mut u64) {
    _write_u64(state, *inner_self_)
}
fn _hash2_nonmacro(self_: &u64, state: &mut u64) {
    // works fine
    _hash_nonmacro(&self_, state);
}

The macro_rules! by itself is sufficient to produce the broken syntax (1), but not to produce a dangling reference (2) because the this parameter is never created:

fn _write_u64(s: &mut u64, x: u64) {
    *s += x;
}
macro_rules! impl_write {
    ($(($ty:ident, $meth:ident),)*) => {$(
        fn _hash(inner_self_: &u64, state: &mut u64) {
            $meth(state, *inner_self_)
        }
    )*}
}
impl_write! { (u64, _write_u64), }
fn _hash2(self_: &u64, state: &mut u64) {
    $0_hash(&self_, state);
}

@evertedsphere
Copy link
Contributor

evertedsphere commented Feb 2, 2024

It looks like hir::semantics::SemanticsImpl::source is returning invalid syntax:

    let fn_source = ctx.sema.source(function)?;
    println!("{}", fn_source.value);
// fn_hash(inner_self_: &u64,state: &mutu64){_write_u64(state, *inner_self_)}

which is missing spaces both after fn and &mut. We then ask fn_source for its .param_list(), which carries the invalid syntax for &mut u64 over.

@Veykril
Copy link
Member

Veykril commented Feb 2, 2024

Macro expanded source lacks almost all whitespace because its not relevant in macro expansions. We need to fix the output up in the assist, there is some helper function for that somewhere but I forgot what its called.

@evertedsphere
Copy link
Contributor

I can take a look in ide-assists.

@evertedsphere
Copy link
Contributor

@rustbot claim

evertedsphere added a commit to evertedsphere/rust-analyzer that referenced this issue Feb 9, 2024
To be added back later once we have a fix.

See rust-lang#16471 and rust-lang#16497 (comment).
bors added a commit that referenced this issue Feb 10, 2024
…ated-method, r=Veykril

Fix incorrect inlining of functions that come from MBE macros

Partial fix for #16471.

As a reminder, there are two issues there:
1. missing whitespace in parameter types (the first test)
2. the `self` parameter not being replaced by `this` in the function body (the second test)

The first part is fixed in this PR. See [this comment](#16497 (comment)) for the second.
@evertedsphere evertedsphere removed their assignment Feb 10, 2024
@evertedsphere
Copy link
Contributor

The missing whitespace has been fixed by #16497; this assist should no longer produce invalid syntax (but the occurrence of self remains.)

@roife
Copy link
Contributor

roife commented Feb 26, 2024

@rustbot claim

@roife
Copy link
Contributor

roife commented Feb 26, 2024

I have fixed both the second problem (*self should be *this) as well as the third one.

For the third question, the problem also lies in the expansion of macro. Should we assume an indent setting of 4 spaces when expanding macros for the third question? (Currently it is set to 2 spaces.) If we change it, it will affect other test cases which involving macro expansions.

@lnicola
Copy link
Member

lnicola commented Feb 26, 2024

Currently it is set to 2 spaces

I hate that 😅.

@Veykril
Copy link
Member

Veykril commented Feb 26, 2024

r-a currently assumes an indentation of 4 spaces for everything so that is fine

bors added a commit that referenced this issue Mar 4, 2024
fix: use 4 spaces for indentation in macro expansion

Partial fix for #16471.

In the previous code, the indentation produced by macro expansion was set to 2 spaces. This PR modifies it to 4 spaces for the sake of consistency.
@roife roife removed their assignment Mar 5, 2024
@antonilol
Copy link
Author

got this bug again (rust-analyzer version: 0.4.1951-standalone)

inlining all in

let _ = [].iter().all(|&a: &i32| a == 2);

gives this:

let _ = {
    let this = &mut [].iter();
    let mutf = |&a: &i32| a == 2;
    while let Some(x) = self.next(){
        if!f(x){
            return false;
        }
    }true
};

let mutf = ... should be let mut f = ..., i thought the missing space was fixed because it is in the hash example (see issue post)

also, the return statement now tries to return from main (see #15471)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

5 participants