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

Prevent ICE when formatting item-only vec!{} #5879

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

tdanniels
Copy link
Contributor

@tdanniels tdanniels commented Aug 5, 2023

Prevent ICE when formatting item-only vec!{}

Fixes #5735

Attempting to format invocations of macros which are considered "forced bracket macros" (currently only vec!), but are invoked with braces instead of brackets, and contain only items in their token trees, currently triggers an ICE in rustfmt. This is because the function that handles formatting macro invocations containing only items, rewrite_macro_with_items, assumes that the forced delimiter style of the macro being formatted is the same as the delimiter style in the macro's source text when attempting to locate the span after the macro's opening delimiter. This leads to the construction of an invalid span, triggering the ICE.

The fix here is to pass the old delimiter style to rewrite_macro_with_items as well, so that it can successfully locate the span.


Hi rustfmt'ers! I'm back again with another ICE fix. Some notes for the reviewer:

  • With how I've implemented this fix, the original reproducing case
fn find_errors(mut self) {
	let errors: Vec<> = vec!{
		#[debug_format = "A({})"]
		struct A {}
	};
}

will be formatted into

fn find_errors(mut self) {
    let errors: Vec = vec![
        #[debug_format = "A({})"]
        struct A {}
    ];
}

I understand that the team's stance is that braced macro invocations should generally be left unformatted. However, the current behaviour of rustfmt on braced vec! invocations is to format them. That is, before this PR, rustfmt will turn vec!{1, struct X {} } into vec![1, struct X {}];.
My PR doesn't change this behaviour. It just extends a similar behaviour to vec! invocations that contain only items. So with my PR, whereas formatting vec!{ struct X {} } would previously have triggered an ICE, it's now formatted into

vec![
    struct X {}
];

I'm not entirely sure that this is the 'right' direction to go in, but the alternative of leaving braced, item-only vec! invocations unformatted seemed about equally surprising to me from a user perspective. Any feedback here is appreciated, and I'm happy to tweak the implementation to whatever behaviour the team thinks is best.

  • I've also included a small refactoring commit of some redundancy I noticed while working in macros.rs. I'm happy to split that off into a separate PR if that's what people prefer.

  • Clippy isn't happy that I've added another parameter to rewrite_macro_with_items, pushing it over the threshold of 7 parameters. I didn't see a way to cleanly keep it below the threshold while still passing in the original delimiter style, but as usual any suggestions are appreciated.

@tdanniels tdanniels changed the title Prevent ICE when formatting braced vec! when... Prevent ICE when formatting braced vec! when it contains only items Aug 5, 2023
@tdanniels tdanniels changed the title Prevent ICE when formatting braced vec! when it contains only items Prevent ICE when formatting item-only vec!{} Aug 5, 2023
@calebcartwright
Copy link
Member

Thanks for another PR!

r? @fee1-dead

Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

LGTM

@tdanniels
Copy link
Contributor Author

Rebased on to the latest master.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

@tdanniels Thanks for the help on this one. I think the approach of passing the original style to locate the correct opener makes a lot of sense.

Happy to merge this after rebasing on the latest master.

Fixes 5735

Attempting to format invocations of macros which are considered "forced
bracket macros" (currently only `vec!`), but are invoked with braces
instead of brackets, and contain only items in their token trees,
currently triggers an ICE in rustfmt. This is because the function that
handles formatting macro invocations containing only items,
`rewrite_macro_with_items`, assumes that the forced delimiter style of
the macro being formatted is the same as the delimiter style in the
macro's source text when attempting to locate the span after the macro's
opening delimiter. This leads to the construction of an invalid span,
triggering the ICE.

The fix here is to pass the old delimiter style to
`rewrite_macro_with_items` as well, so that it can successfully locate
the span.
@tdanniels
Copy link
Contributor Author

Thanks for the reviews everyone! @ytmimi it should be all up to date now.

@ytmimi ytmimi merged commit e480739 into rust-lang:master Aug 17, 2023
27 checks passed
@ytmimi
Copy link
Contributor

ytmimi commented Aug 17, 2023

You're very welcome! Again, really appreciate the help!

@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-not-reviewed labels Aug 17, 2023
@ytmimi ytmimi removed the release-notes Needs an associated changelog entry label Oct 23, 2023
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.

ICE Request to format inverted span
5 participants