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

Rustfmt removes argument attribute when line is long! #4579

Closed
Boscop opened this issue Dec 6, 2020 · 11 comments · Fixed by #4980
Closed

Rustfmt removes argument attribute when line is long! #4579

Boscop opened this issue Dec 6, 2020 · 11 comments · Fixed by #4980
Labels
bug Panic, non-idempotency, invalid code, etc.

Comments

@Boscop
Copy link

Boscop commented Dec 6, 2020

Describe the bug

Rustfmt removes argument attribute when line is long!

To Reproduce

  1. Put this in a file temp.rs
#[macro_export]
macro_rules! main {
	() => {
		#[spirv(fragment)]
		pub fn main_fs(
			mut out_color: ::spirv_std::storage_class::Output<Vec4>,
			#[spirv(frag_coord)] frag_coord: ::spirv_std::storage_class::Input<
				::spirv_std::glam::Vec4,
			>,
			#[spirv(descriptor_set = 1)] iChannel0: ::spirv_std::storage_class::UniformConstant<
				::spirv_std::SampledImage<::spirv_std::Image2d>,
			>,
			#[spirv(descriptor_set = 1)] iChannel1: ::spirv_std::storage_class::UniformConstant<
				::spirv_std::SampledImage<::spirv_std::Image2d>,
			>,
			#[spirv(descriptor_set = 1)] iChannel2: ::spirv_std::storage_class::UniformConstant<
				::spirv_std::SampledImage<::spirv_std::Image2d>,
			>,
			#[spirv(descriptor_set = 1)] iChannel3: ::spirv_std::storage_class::UniformConstant<
				::spirv_std::SampledImage<::spirv_std::Image2d>,
			>,
			#[spirv(descriptor_set = 1)] iResolution: ::spirv_std::storage_class::UniformConstant<
				::spirv_std::glam::Vec3A,
			>,
			#[spirv(descriptor_set = 1)] iTime: ::spirv_std::storage_class::UniformConstant<f32>,
			#[spirv(descriptor_set = 1)] iMouse: ::spirv_std::storage_class::UniformConstant<
				::spirv_std::glam::Vec4,
			>,
			#[spirv(descriptor_set = 1)] iDate: ::spirv_std::storage_class::UniformConstant<
				::spirv_std::glam::Vec4,
			>,
			#[spirv(descriptor_set = 1)] iSampleRate: ::spirv_std::storage_class::UniformConstant<
				f32,
			>,
			#[spirv(descriptor_set = 1)] iFrame: ::spirv_std::storage_class::UniformConstant<i32>,
			#[spirv(descriptor_set = 1)] iTimeDelta: ::spirv_std::storage_class::UniformConstant<
				f32,
			>,
			#[spirv(descriptor_set = 1)] iFrameRate: ::spirv_std::storage_class::UniformConstant<
				f32,
			>,
			#[spirv(descriptor_set = 1)] iChannelTime: ::spirv_std::storage_class::UniformConstant<
				[f32; 4],
			>,
			#[spirv(descriptor_set = 1)] iChannelResolution: ::spirv_std::storage_class::UniformConstant<
				[::spirv_std::glam::Vec3A; 4],
			>,
			#[spirv(descriptor_set = 1)] unViewport: ::spirv_std::storage_class::UniformConstant<
				::spirv_std::glam::Vec4,
			>,
			#[spirv(descriptor_set = 1)] unCorners: ::spirv_std::storage_class::UniformConstant<
				[::spirv_std::glam::Vec3A; 5],
			>,
		) {
		}
	};
}
  1. Run rustfmt temp.rs
  2. Notice it removed the #[spirv(descriptor_set = 1)] attribute from the iChannelResolution arg!
    It turns it into this:
// ...
			#[spirv(descriptor_set = 1)] iChannelTime: ::spirv_std::storage_class::UniformConstant<
				[f32; 4],
			>,
			iChannelResolution: ::spirv_std::storage_class::UniformConstant<
						[::spirv_std::glam::Vec3A; 4],
					>,
			#[spirv(descriptor_set = 1)] unViewport: ::spirv_std::storage_class::UniformConstant<
				::spirv_std::glam::Vec4,
			>,
			#[spirv(descriptor_set = 1)] unCorners: ::spirv_std::storage_class::UniformConstant<
				[::spirv_std::glam::Vec3A; 5],
			>,
		) {
		}
	};
}

Expected behavior
Don't remove any attributes.

Meta

  • rustfmt version: rustfmt 1.4.27-nightly (580d826 2020-11-16)
  • From where did you install rustfmt?: rustup
  • How do you run rustfmt: rustfmt

Settings:

edition = "2018"
version = "Two"
newline_style = "Unix"
use_small_heuristics = "Max"
hard_tabs = true
merge_imports = true
reorder_impl_items = true
use_field_init_shorthand = true
use_try_shorthand = true
spaces_around_ranges = true
overflow_delimited_expr = true
@Boscop Boscop added the bug Panic, non-idempotency, invalid code, etc. label Dec 6, 2020
@calebcartwright
Copy link
Member

Simpler snippet, which can be used for repro even with the defaults in the released 1.x versions

#[macro_export]
macro_rules! main {
	() => {
		#[spirv(fragment)]
		pub fn main_fs(
			mut out_color: ::spirv_std::storage_class::Output<Vec4>,
			#[spirv(descriptor_set = 1)] iChannelResolution: ::spirv_std::storage_class::UniformConstant<
				[::spirv_std::glam::Vec3A; 4],
			>,
		) {
		}
	};
}

For anyone interested in working on this, note that it's not reproducible in source on master so be sure any fixes are based on the 1.x branches (current branch at time of this posting would be 1.4.29)

@chansuke
Copy link
Contributor

Can I working on this? I want to have a try.

@calebcartwright
Copy link
Member

@chansuke

Can I working on this? I want to have a try.

Sure! I want to say that subsequent to the last post I've backported some changes that touched the attribute formatting code, so it might be worthwhile to first check whether this can still be reproduced on the latest branch (rustfmt-1.4.31). Obviously if it's still reproducible then the fix from master is still pending, but even if it has already been resolved it would still be helpful to add tests which cover this case to avoid any potential regressions

@chansuke
Copy link
Contributor

chansuke commented Jan 17, 2021

@calebcartwright Thanks for the comment! I will take a look at the latest branch first.

@chansuke
Copy link
Contributor

I have tested on rustfmt-1.4.31 branch and the error is still reproducible.

@hrydgard
Copy link

Still happening! Really disturbing to have attributes just disappear on save (I use format-on-save). Had to shorten a variable name to be under the limit.

@hrydgard
Copy link

hrydgard commented Jun 29, 2021

Again, still happening.

Realized I can just locally use #[rustfmt::skip] , but it's very disconcerting to have rustfmt just break code!

@elijaharita
Copy link

I'm also affected by this issue. Here's an even simpler repro -

fn test(#[a_very_very_very_very_very_long_attribute] a_very_very_very_very_very_very_long_parameter: u32) {
    // ...
}

after format -

fn test(a_very_very_very_very_very_very_long_parameter: u32) {
    // ...
}

On nightly 2021-08-07.

#[rustfmt::skip] is super helpful in the meantime, I wasn't aware of it!

@hrydgard
Copy link

hrydgard commented Sep 2, 2021

This is so much more broken than most of the other issues here that I'm astonished it hasn't got any attention. Sorry for the bump, just a reminder that this is really really bad.

@calebcartwright
Copy link
Member

All - there were a few pieces that needed to be backported to resolve this but it is now done and the fix will be included in whatever the next rustfmt update is (no ETA, but relatively soon).

@hrydgard - I appreciate your interest in a resolution and frustration with the bug, but please refrain from making these types of comments on any future threads. Issues which are still open and without updates posted inherently means there are no updates, so the repeated pings just add noise and in some cases can discourage folks from wanting to contribute.

Yes, in cases where this bug is hit the severity is pretty high, but it's also not one that seems to occur all that often in practice which is one of the many reasons why this wasn't exactly a number one priority for us.

@hrydgard
Copy link

hrydgard commented Sep 7, 2021

I am usually extremely restrictive with bumping issues, but I got breakage because of this for the third time and it looked like it had to be an absolutely trivial issue. Sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants