Infinite macro body indentation

[davidBar-On]

Proposed fix for issues #4603 and #4609. Both issues are caused because original code snippet is used during macro definition formatting - caused by a missing comment in #4603 and macro code that rustfmt doesn't understand in #4609. The extra indentation is caused since MacroBranch() indents the body of the formatted macro. The problem is that the indentation assumes that the macro body is properly formatted, when the first line starts at column 1. However, this is not the case original code snippet, so each time the code is formatted another extra indentation is added to these lines.

The fix here is by adding a writable bool parameter through all the function calls, either as additional function parameter or by added parameter to FmtVisitor and RewriteContext. When original code snippet is used the event is escalated up by setting this parameter to to true.

This approach seem to be similar to the approach taken with is_macro_def and is_nested_macro, except that escalation is up instead of down. Although the approach is not very elegant, I didn't find a better way to do it, except for using a kind of a global variable, which I am not sure is desired.

[calebcartwright]

Thanks for the PR! I can fully believe that there's an indentation issue in the event of failing to format parts of the macro def, though want to make sure we don't lose sight of the underlying issues that are causing formatting to fail in the first place either (comments between pats in an Or Pattern, etc.).

Probably won't be able to go through this for a few more days, though one thing that jumped out was file permission changes in the index being included in the changeset. Could you please review, and revert those, or explain what those are for if intentional/needed?

[davidBar-On]

file permission changes in the index being included in the changeset

Changed back the files permissions. I have no idea how or why that happened.

though want to make sure we don't lose sight of the underlying issues that are causing formatting to fail in the first place either

In both issues I have added an explanation about the root cause. I did try initially to fix the root cause of both, but realized that event if they will be fixed there may be other cases where original code may be used while formatting macro definition, so a general fix is probably needed in any case.

Regarding the specific root causes of the issues. Fixing #4609 seems to be a major change as it requires proper handling the $foo identities, instead of using the workaround per issue #8. Fixing #4603 is not macros specific issue and requires comments handling for BinOp operations.

[calebcartwright]

Was just starting to take a look at this and was puzzled by what I was seeing in your diff vs. what I had locally, only to remember I have the current/release 1.x version of the rustfmt codebase in my local context right now.

For reported bugs, it's important that we take a look at the version of rustfmt that corresponds with the reported rustfmt version (at least the latest major version in the 1.x cases).

The latest 1.x branch is https://github.com/rust-lang/rustfmt/tree/rustfmt-1.4.35, so that's where we should focus. Also, I know the two issues present with the same general symptom, but even a cursory glance leads me to believe that there are actually some very different underlying causes. As such the same general theme applies of strongly preferring to resolve the cause (or mitigate by containing as close to the root cause as possible), while only falling back to the final symptom mitigation as an absolute last resort.

Accordingly, and in the spirit of trying to minimize the size of PRs for ease of review and testing, I think we'll likely be best served by splitting out the changes into their respective PRs, both targeting the 1.x branch.

Specifically in the case of #4603, and again with the 1.x version of rustfmt that's in use, I see a whole chain of cascading issues:

1. our old friend binop exprs with comments 😄 results in failure to format, and reverting to the "original" snippet (not the literal original, it's a function block-wrapped version with an extra indent)
2. even though there was a formatting because of num. 1 failure, we're not checking for certain types of errors on the results, so it looks like the snippet formatting with the block "succeeded"
3. because the raw snippet formatting failed (expected in this case), and the block "succeeded", we're back in the successful formatting path, but the conditional that checks whether each line was actually formatted speciously gives the all clear because of num. 2, adding the extra indentation

the outcome of num. 3 happens every run, thus causing the idempotency failure.

I don't think it's feasible to address num. 1 at this time, as the same challenges that existed before are still in place. However, i think num. 2 could be addressed in a relatively straightforward manner within format_snippet (and possibly tweaking the session to give insight into formatting errors of the lost commetn variant) by conditionally enhancing the check when we're dealing with macro defs.

[davidBar-On]

... we'll likely be best served by splitting out the changes into their respective PRs, both targeting the 1.x branch.

By now I always made the changes to 2.x and migrated them to 1.x when you asked. Should these changes be done to 1.x first?

issues present with the same general symptom, but even a cursory glance leads me to believe that there are actually some very different underlying causes.

It is true that the issues root causes are different, but in both cases the indentation issue is because original code is used. The workaround implemented by the PR is common for both in several places.

num. 2 could be addressed in a relatively straightforward manner within format_snippet (and possibly tweaking the session to give insight into formatting errors of the lost comment variant) by conditionally enhancing the check when we're dealing with macro defs.

This approach seem to be similar to the approach in the PR (of course it may be that it can be implemented much better). The added macro_original_code_was_used parameter is used to "give insight into formatting errors of the lost comment variant". The main difference seem to be that currently the rewrite function is getting the original code and not format_snippet.

[calebcartwright]

This approach seem to be similar to the approach in the PR (of course it may be that it can be implemented much better)

Not sure I agree with this, particularly in the context of getting the fix resolved for the reported version and the notable divergence of relevant portions of the codebase between 1.x and 2.x. I suppose the abstract goals are the same, but I'm speaking about a very, very different implementation that should be much more succinct/less intrusive.

Specifically, within format_snippet (again, all 1.x branch), adding a single condition to do a more exhaustive querying on the session would resolve #4603

via updating this:

rustfmt/src/lib.rs

Lines 303 to 306 in 55d2620

	session.errors.has_macro_format_failure
	|| session.out.as_ref().unwrap().is_empty() && !snippet.is_empty()
	|| result.is_err(),
	result,

to instead be this (note the additional check):

session.errors.has_macro_format_failure
    || session.out.as_ref().unwrap().is_empty() && !snippet.is_empty()
    || result.is_err()
    || (is_macro_def && session.has_formatting_errors()),
result,

now the has_formatting_errors check is a bit too broad for what we really want, which is why I mentioned some additional minor tweaks to the session to add one or more helpers that let us zoom in on just the error kinds we care about in this scenario (LostComment and probably TrailingWhitespace as well):

rustfmt/src/lib.rs

Lines 97 to 99 in 55d2620

	/// Line ends in whitespace.
	#[error("left behind trailing whitespace")]
	TrailingWhitespace,

rustfmt/src/lib.rs

Lines 125 to 127 in 55d2620

	/// If we had formatted the given node, then we would have lost a comment.
	#[error("not formatted because a comment would be lost")]
	LostComment,

so I'm thinking we could make the check more precise by adding a couple additional flags on the session with an accompanying new helper method(s), and then tweaking the tracking logic to adjust those flags accordingly

rustfmt/src/lib.rs

Lines 208 to 231 in 55d2620

	fn track_errors(&self, new_errors: &[FormattingError]) {
	let errs = &mut self.internal.borrow_mut().1;
	if !new_errors.is_empty() {
	errs.has_formatting_errors = true;
	}
	if errs.has_operational_errors && errs.has_check_errors {
	return;
	}
	for err in new_errors {
	match err.kind {
	ErrorKind::LineOverflow(..) | ErrorKind::TrailingWhitespace => {
	errs.has_operational_errors = true;
	}
	ErrorKind::BadIssue(_)
	| ErrorKind::LicenseCheck
	| ErrorKind::DeprecatedAttr
	| ErrorKind::BadAttr
	| ErrorKind::VersionMismatch => {
	errs.has_check_errors = true;
	}
	_ => {}
	}
	}
	}

[calebcartwright]

By now I always made the changes to 2.x and migrated them to 1.x when you asked. Should these changes be done to 1.x first?

The short answer is that the sequencing always depends on the context.

Remember that the 2.0-rc (i.e. what's on the master branch) is largely not used by anyone, and the 1.x version of the codebase (on the various rustfmt-1.* branches) is what's actually released and distributed to users. When users report bugs, they almost certainly encountered that bug in the official/released 1.x version of rustfmt.

So when triaging such bug reports, you'll typically want to:

1. review the reported version(s) where the bug was encountered, and see if (a) the user is using the latest 1.x version and if not then (b) whether the bug has already been fixed in a newer 1.x version
2. if the bug is reproducible in the latest 1.x version, check to see if it's reproducible on the master branch/2.0-rc version
   • if it's not reproducible on master, then try to identify the commit(s)/PR that fixed the bug on master so that these can be backported/cherry-picked to 1.x. The deferment of releasing 2.0 has resulted in many bug fixes on master that haven't yet been backported

If the bug is reproducible everywhere, then which branch to start with really depends on the nature of the bug and fix. If the fix involves the same code fixes for both versions of the codebase, then it will typically be best to start with fixing on the master branch because those fixes can be directly backported (extra PR not needed).

If the bug will require different fixes between 1.x and 2.x (again for a bug reported against 1.x), then it's almost always best to focus on getting the 1.x fix in first, to actually facilitate getting the fix out to users.

It is true that the issues root causes are different, but in both cases the indentation issue is because original code is used. The workaround implemented by the PR is common for both in several places.

As noted above, there's a cleaner, direct fix for the first issue (#4603) which addresses the issue with determining whether format was successful or not, that doesn't require any workaround. I understand that the PR is adding a fallback/workaround that happens to help with both issues, but that doesn't mean that's the preferable approach for either.

[davidBar-On]

By now I always made the changes to 2.x and migrated them to 1.x when you asked. Should these changes be done to 1.x first?

The short answer is that the sequencing always depends on the context. .....

Thanks for the detailed response. That really helps, although I am not sure if I will be able to decide whether which version to change, as I am not familiar with the differences. By the way, I only now saw the diff with 1.x you mentioned. It may be (but I am not sure) that the switch to 1.x master was done after I pushed the changes, as I already learned the hard way to check the diff to make sure the changes are only what I indented to be changed.

Specifically, within format_snippet (again, all 1.x branch), adding a single condition to do a more exhaustive querying on the session would resolve #4603 ....

O.k. I now understand the intended change. Will try to submit a related 1.x PR for #4603 (I currently have a problems building using cargo make but I hope this is because of a problem with rustc nightly build that will be fixed after the weekend.)

By the way, is there a 2.x mechanism for reporting errors similar to the session.errors in 1.x? If there is, it would allow a similar and easier solution for #4603 in 2.x.

[calebcartwright]

Thanks for the detailed response. That really helps, although I am not sure if I will be able to decide whether which version to change, as I am not familiar with the differences.

Sure! And understood, it's not cleanly deterministic which is why the best answer tends to be "it depends" 😄 TBH it doesn't necessarily matter which branch/version you start with when debugging/troubleshooting a bug. It's just that in those cases where the bug exists everywhere, after you track down the cause to the corresponding function(s) you'd likely want to cross reference the other branch/version once you know the portions of the code that have the bug/would be modified as part of your resolution.

By the way, I only now saw the diff with 1.x you mentioned. It may be (but I am not sure) that the switch to 1.x master was done after I pushed the changes

Sorry, I didn't understand this. Could you elaborate on what you mean?

I currently have a problems building using cargo make but I hope this is because of a problem with rustc nightly build that will be fixed after the weekend

This should be resolved already on master, and on the latest 1.x branch (1.4.36) after I merge #4688. Due to our heavy reliance on the rustc internals via the auto publish crates, we are extremely prone to nightly breakage. You should always be able to use the nightly version specified in the toolchain file for any branch though:

https://github.com/rust-lang/rustfmt/blob/master/rust-toolchain

[davidBar-On]

By the way, I only now saw the diff with 1.x you mentioned. It may be (but I am not sure) that the switch to 1.x master was done after I pushed the changes

Sorry, I didn't understand this. Could you elaborate on what you mean?

Please ignore. Seems I was able to confuse myself. I clicked the force change I did 10 days ago, changed "base" to master and saw the diff with 1.x (I think). However the Fix for issues 4603 and 4609 - unlimitted indentation of macro code above it shows the proper diff.

You should always be able to use the nightly version specified in the toolchain file for any branch though:

O.k. Thanks.

[calebcartwright]

By the way, is there a 2.x mechanism for reporting errors similar to the session.errors in 1.x? If there is, it would allow a similar and easier solution for #4603 in 2.x.

There was a sizeable refactor a while ago as part of the prep for switching to releasing 2.x that dramatically changed some of the public-facing API and internals that caused the divergence we see today, including the removal of some of the things that I think we actually still need.

I think the core infrastructure is still there in 2.x, but at some point I will revisit those changes and see what all it makes sense to restore because it definitely seems like we should maintain some of those error variants as they clearly serve a very useful purpose

[davidBar-On]

This should be resolved already on master, and on the latest 1.x branch (1.4.36) after I merge #4688

I am now able to build, but after changing the code I get strange behavior. cargo make test fails on the new tests I added, as if my changes were not built. However, if I format the new tests cases file manually using the built ./target/debug/rustfmt the output is o.k., i.e. my changes were built. I tried that several times, including cargo clean.

Another issue which I am not sure is related, is that while cargo make test succeeds, cargo make build fails on error "unresolved import crate::arch::x86_64::_mm_movemask_pi8"

[calebcartwright]

This should be resolved already on master, and on the latest 1.x branch (1.4.36) after I merge #4688

I am now able to build, but after changing the code I get strange behavior. cargo make test fails on the new tests I added, as if my changes were not built. However, if I format the new tests cases file manually using the built ./target/debug/rustfmt the output is o.k., i.e. my changes were built. I tried that several times, including cargo clean.

Another issue which I am not sure is related, is that while cargo make test succeeds, cargo make build fails on error "unresolved import crate::arch::x86_64::_mm_movemask_pi8"

I'm not really sure what to tell ya, as these would be issues specific to your environment. I also feel like you're descrbing mutually exclusive behavior (build success but tests with cargo make test failing, and then separately cargo make test passing but the build somehow failing)?

Not much I can do to help with issues in your local environment, but you may want to try nuking your target directory entirely, ensuring that you haven't installed rustfmt from source, and that your local git branches are correct. It may be easier to start with a clean slate and making the changes directly on whatever local/fork branch you have off rustfmt-1.4.36.

I am also going to go ahead and close this, given that there's a definite better approach to address at least one of the issues that will be a very different changeset, current conflicting files, length of this thread + breadth of topic, etc. Looking forward to your PR with the 1.x fix!