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

Fix to some rustfmt::skip issues #4803

Open
wants to merge 1 commit into
base: rustfmt-2.0.0-rc.2
Choose a base branch
from

Conversation

davidBar-On
Copy link
Contributor

Suggested fix for issue #4706, including enhancements to the handling of #[rustfmt::skip].

#4706 issue was caused because format_impl() did not copy the skipped range from the local context into the visitor.context. The same issue was corrected also in format_trait() and rewrite_macro_with_items().

While testing I found that there are cases where self.line_number in push_skipped_with_span() is incorrect (usually 0 or 1). That caused an incorrect skipped range to be created. Therefore, I had to make changes to this function. The main change is that the beginning of the skipped range mainly depends on the beginning of the attributes block, instead of self.line_number.

Another major change to push_skipped_with_span() is related to the following comment:

// do not take into account the lines with attributes as part of the skipped range

This comment is not correct in general, as in several cases only the first attribute in an attributes block is not skipped (this was also partly discussed in PR #4707). Therefore, my change to push_skipped_with_span() assumes that only the first attribute line should not be skipped. If this is the correct approach, then the above comment should be removed. Otherwise, push_skipped_with_span() should be changed so the beginning of the skipped range will depend on the end of the attributes block, instead of its beginning.

I am aware that the above changes are more general than the specific #4706 issue, but there doesn't seem to be a good way to fix only this issue, because of the required change for push_skipped_with_span() to handle wrong self.line_number that impacts other skip cases.

Because of the general changes, the test cases file include tests for skipping fn/impl/trait/macro. In addition, a test was added in report.rs to make sure that the generated skip_ranges is correct.

@calebcartwright calebcartwright changed the base branch from master to rustfmt-2.0.0-rc.2 April 30, 2021 00:30
@ytmimi ytmimi added pr-targeting-2.0 This PR is targeting the 2.0 branch p-low labels Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-low pr-not-reviewed pr-targeting-2.0 This PR is targeting the 2.0 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants