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

fixes #4115, #4029, #3898 #4136

Merged
merged 2 commits into from
Apr 22, 2020
Merged

fixes #4115, #4029, #3898 #4136

merged 2 commits into from
Apr 22, 2020

Conversation

ahl
Copy link
Contributor

@ahl ahl commented Apr 22, 2020

Fixes #4115. Fixes #4029. Fixes #3898.

The new tests fail on master as follows:

Mismatch at tests/target/issue-4115.rs:1:
-#[derive(
-    A,
-    B,
-    C,
-    D,
-    // E,
-)]
+#[derive(A, B, C, D)]
 fn foo() {}

Mismatch at tests/target/issue-4029.rs:1:
 // issue #4029
-#[derive(Debug, Clone, Default Hash)]
+#[derive()]
 struct S;
 
 // issue #3898

Mismatch at tests/target/issue-4029.rs:6:
-#[derive(Debug, Clone, Default,, Hash)]
+#[derive()]
 struct T;

They (and all other tests) pass with the proposed fix.

At the root of all three issues is the use of ast::Attribute:: meta_item_list() which returns None if the derive is malformed. In addition no affordance was made for comments interspersed with derive components. This fix checks for errors from meta_item_list() and leans on existing mechanisms to deal with comments.


Although I handled this case, I would appreciate review feedback for the case where a derive doesn't fit quite fit on a single line and is formatted like this:

#[derive(this, would, be, too, long, for, a single, line, lets, say)]
// so you get this:
#[derive(
    this, would, be, too, long, for, a single, line, lets, say,
)]

To handle that case I use Always for the trailing separator with ListFormatting and then remove that final character if it all does fit on one line. I couldn't find an existing mechanism in ListFormatting that did what I wanted there.

See lines 143 and 177.

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM, thank you!


Although I handled this case, I would appreciate review feedback for the case where a derive doesn't fit quite fit on a single line and is formatted like this:

I think we could use overflow::rewrite_with_parens for this with some refactoring, though I am not confident.

@topecongiro topecongiro merged commit c019181 into rust-lang:master Apr 22, 2020
@karyon
Copy link
Contributor

karyon commented Oct 26, 2021

Backported in #4564

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants