Skip to content

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

Merged
ytmimi merged 2 commits intorust-lang:masterfrom
tdanniels:issue_5735
Aug 17, 2023
Merged

Prevent ICE when formatting item-only vec!{}#5879
ytmimi merged 2 commits intorust-lang:masterfrom
tdanniels:issue_5735

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
@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