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

Add transactional semantics to rustfix #14747

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

saites
Copy link

@saites saites commented Oct 30, 2024

What does this PR try to resolve?

This PR adds transactional semantics to rustfix::Data, enabling rustfix::CodeFix to apply Suggestions as atomic units and rollback partially-applied changes when they conflict with existing ones. The basic approach and goals are discussed in a comment on issue 14699. In that comment, I proposed a solution which extended the existing State enumeration, but described an alternative that simplifies the overall tracking of incoming changes; this PR implements the latter.

The PR is broken up into 5 commits:

  • The first commit is more-or-less the minimal changes necessary for this approach. It adds the commit and restore methods, and it changes how replacements are tracked to make this easier.
  • The second commit adds a little extra information to Error::AlreadyReplaced in order to remove the special-case handling for duplicate replacements.
  • The third commit passes through a lifetime parameter so that the data can be borrowed, since it's not edited directly.
  • The fourth commit generalizes the &[u8] using AsRef to make the interface easier to use (since this code is primarily interested in &str).
  • The fifth commit adds a to_string method and moves the logic from lib.rs for the same reason as above.

How should we test and review this PR?

I've added an additional test and updated the existing ones, but it's a good idea to experiment with cargo clippy --fix on repos that match the cases described in these open issues.

Additional information

This removes special handling for rust-lang/rust#51211.

Fixes #14699
Fixes #13027
Fixes rust-lang/rust-clippy/issues/13549

@rustbot
Copy link
Collaborator

rustbot commented Oct 30, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2024
@saites saites changed the title Rustfix transactional Add transactional semantics to rustfix Oct 30, 2024
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

This is awesome! Thank you for this patch!

Comment on lines +275 to +282
pub fn ignore_identical<T: Default>(err: Error) -> Result<T, Error> {
match err {
Error::AlreadyReplaced {
is_identical: true, ..
} => Ok(Default::default()),
_ => Err(err),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Probably unnecessary pub and generics.

Suggested change
pub fn ignore_identical<T: Default>(err: Error) -> Result<T, Error> {
match err {
Error::AlreadyReplaced {
is_identical: true, ..
} => Ok(Default::default()),
_ => Err(err),
}
}
fn ignore_identical(err: Error) -> Result<(), Error> {
match err {
Error::AlreadyReplaced {
is_identical: true, ..
} => Ok(()),
_ => Err(err),
}
}

Feels like we can just inline this in apply_suggestions, but that doesn't really matter :)

@@ -216,34 +216,34 @@ pub fn collect_suggestions<S: ::std::hash::BuildHasher>(
/// 2. Calls [`CodeFix::apply`] to apply suggestions to the source code.
/// 3. Calls [`CodeFix::finish`] to get the "fixed" code.
#[derive(Clone)]
pub struct CodeFix {
data: replace::Data,
pub struct CodeFix<'orig> {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would probably just call it 'a. Less visual noise and there is no ambiguity.

#[derive(Debug, Clone, Default)]
pub struct Data {
/// Original data.
original: Vec<u8>,
/// [`Span`]s covering the full range of the original data.
/// Important: it's expected that the underlying implementation maintains this in order,
Copy link
Member

@weihanglo weihanglo Oct 31, 2024

Choose a reason for hiding this comment

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

Although the code looks pretty valid, should we add some debug_assertion for this in to_vec()?

///
/// See the module-level documentation for more information on why uncommitted changes are included.
pub fn to_vec(&self) -> Vec<u8> {
let mut prev_end = 0usize;
Copy link
Member

Choose a reason for hiding this comment

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

nit: This should be sufficient.

Suggested change
let mut prev_end = 0usize;
let mut prev_end = 0;

match fixed.apply(suggestion) {
Ok(()) => fixed_file.fixes_applied += 1,
Err(rustfix::Error::AlreadyReplaced {
Copy link
Member

Choose a reason for hiding this comment

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

While I don't think the compiler will get this fixed in any near future, we may want to restore this comment in case there is such a future.

(probably ditto in apply_suggestions()?)

            // If this replacement matches exactly the part that we would
            // otherwise split then we ignore this for now. This means that you
            // can replace the exact same range with the exact same content
            // multiple times and we'll process and allow it.
            //
            // This is currently done to alleviate issues like
            // rust-lang/rust#51211 although this clause likely wants to be
            // removed if that's fixed deeper in the compiler.

Comment on lines 314 to 325
fn insert_same_twice() {
let mut d = Data::new("foo");
d.replace_range(1..1, "b").unwrap();
assert_eq!("fboo", str(&d.to_vec()));
assert!(matches!(
d.replace_range(1..1, "b").unwrap_err(),
Error::AlreadyReplaced {
is_identical: true,
..
},
));
assert_eq!("fboo", str(&d.to_vec()));
Copy link
Member

Choose a reason for hiding this comment

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

This test change should move to the second commit maybe.

Copy link
Member

@weihanglo weihanglo Oct 31, 2024

Choose a reason for hiding this comment

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

While this has already covered the case of #13027, I feel we should generate a snapshot test from the exact macro diagnostics, like https://github.com/rust-lang/cargo/blob/de5832fe8fe1a05e278eee82bbc801d544ed4bd1/crates/rustfix/tests/everything/E0178.rs, if we aim to close it.

I can help with that if it is too much a burden.

// Keep sorted by start position, with pure inserts before replacements.
let ins_point = self.parts.partition_point(|span| {
span.range.start < range.start
|| (span.range.start == range.start && span.range.end < range.end)
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain a bit why we need span.range.end < range.end?

Copy link
Member

Choose a reason for hiding this comment

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

with pure inserts before replacements.

I think got it :)

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the last three commits are mostly refactoring. To make things clearer and more traceable, could we move them to a separate or a follow-up PR?

range.end,
self.parts,
);
return Err(Error::MaybeAlreadyReplaced(range));
Copy link
Member

Choose a reason for hiding this comment

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

If this is no longer needed, we should remove it (and maybe add #[mon_exhaustive] to the error enum as a follow-up).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
3 participants