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

Generate default impl when converting #[derive(Debug)] to manual impl #9814

Merged
merged 15 commits into from
Aug 8, 2021

Conversation

yoshuawuyts
Copy link
Member

@yoshuawuyts yoshuawuyts commented Aug 8, 2021

This patch makes it so when you convert #[derive(Debug)] to a manual impl, a default body is provided that's equivalent to the original output of #[derive(Debug)]. This should make it drastically easier to write custom Debug impls, especially when all you want to do is quickly omit a single field which is !Debug.

This is implemented for enums, record structs, tuple structs, empty structs - and it sets us up to implement variations on this in the future for other traits (like PartialEq and Hash).

Thanks!

Codegen diff

This is the difference in codegen for record structs with this patch:

struct Foo {
    bar: String,
}

impl fmt::Debug for Foo {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        todo!();
+        f.debug_struct("Foo").field("bar", &self.bar).finish()
    }
}

@yoshuawuyts
Copy link
Member Author

CI is currently failing on:

thread 'tidy::files_are_tidy' panicked at '
TODO markers or todo! macros should not be committed to the master branch,
use FIXME instead
/home/runner/work/rust-analyzer/rust-analyzer/crates/ide_assists/src/tests/generated.rs
', crates/rust-analyzer/tests/slow-tests/tidy.rs:292:9

But looking at the code diff, the only change we've made to the file is removing a todo! statement. This makes it seem like the error might be originating outside this PR? What would the right fix for this be?

cc/ @Veykril.

@Veykril
Copy link
Member

Veykril commented Aug 8, 2021

Heh, thats a fun one. So the generated tests still contain a todo!() invocation, but your changes removed the only ${0:todo!()} snippet. So now the tidy script is still picking up a todo in the generated.rs but there is no snippet so its not tidy according to it.

I think you can just add the path of the generated.rs file to the tidy exclusions for now.
That is here https://github.com/rust-analyzer/rust-analyzer/blob/f0d32591a68ecacd61a59aaa9df924890d0212d7/crates/rust-analyzer/tests/slow-tests/tidy.rs#L270-L282

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Aug 8, 2021

Updated with feedback from review. Waiting on CI to pass now ✨

edit: yay, it passes! 🎉 - think it's good to merge now!

@bors
Copy link
Contributor

bors bot commented Aug 8, 2021

✌️ yoshuawuyts can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@lnicola
Copy link
Member

lnicola commented Aug 8, 2021

Can we avoid some of the unwraps in there, even if by doing nothing when something is missing? I'm sure that at least some of them should be safe, but that can change, and in the past assists were the part of RA that was most prone to random panics.

@Veykril
Copy link
Member

Veykril commented Aug 8, 2021

I agree, minimizing unwraps where possible is a good choice.

yoshuawuyts and others added 2 commits August 9, 2021 00:00
Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
@yoshuawuyts
Copy link
Member Author

Implemented all remaining feedback!

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 8, 2021

@bors bors bot merged commit 5664a2b into rust-lang:master Aug 8, 2021
@yoshuawuyts yoshuawuyts deleted the gen-debug branch August 8, 2021 22:37
@lnicola
Copy link
Member

lnicola commented Aug 9, 2021

debug-impl

bors bot added a commit that referenced this pull request Aug 9, 2021
9825: Generate default impl when converting #[derive(Default)] to manual impl r=Veykril a=yoshuawuyts

Similar to #9814, but for `#[derive(Default)]`. Thanks!

## Follow-up steps
I've added the tests inside `handlers/replace_derive_with_manual_impl.rs` again, but I'm planning a follow-up PR to extract these to `utils/` so we can share them between assists - and maybe even add another assist just for the purpose of testing these impls (e.g. `generate_default_trait_body`).

The step after _that_ is likely to fill out the remaining traits, so we can make it so whenever RA auto-completes a trait which also can be derived, we provide a default function body.


Co-authored-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
bors bot added a commit that referenced this pull request Aug 10, 2021
9830: Enable more assists to generate default trait body impls r=Veykril a=yoshuawuyts

Enable more assists to benefit from trait body generation. Follow-up to #9825 and #9814.

__edit:__ I'd like to move the existing tests to this new file too, but I'll do that in a follow-up PR.

Co-authored-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
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.

4 participants