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

Merged
merged 1 commit into from
Nov 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ rand = "0.8.5"
regex = "1.10.5"
rusqlite = { version = "0.32.0", features = ["bundled"] }
rustc-hash = "2.0.0"
rustfix = { version = "0.8.2", path = "crates/rustfix" }
rustfix = { version = "0.9.0", path = "crates/rustfix" }
same-file = "1.0.6"
schemars = "0.8.21"
security-framework = "2.11.1"
Expand Down
2 changes: 1 addition & 1 deletion crates/rustfix/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "rustfix"
version = "0.8.8"
version = "0.9.0"
authors = [
"Pascal Hertleif <[email protected]>",
"Oliver Schneider <[email protected]>",
Expand Down
5 changes: 2 additions & 3 deletions crates/rustfix/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use std::ops::Range;

#[non_exhaustive]
#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error("invalid range {0:?}, start is larger than end")]
Expand All @@ -10,9 +11,7 @@ pub enum Error {
#[error("invalid range {0:?}, original data is only {1} byte long")]
DataLengthExceeded(Range<usize>, usize),

#[error("could not replace range {0:?}, maybe parts of it were already replaced?")]
MaybeAlreadyReplaced(Range<usize>),

#[non_exhaustive] // There are plans to add fields to this variant at a later time.
#[error("cannot replace slice of data that was already replaced")]
AlreadyReplaced,

Expand Down
6 changes: 4 additions & 2 deletions crates/rustfix/src/lib.rs
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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, certainly. I got a bit over-zealous during the original work, and that's actually why I pulled them out into these separate commits. I'll move those commits out of this PR.

the last three commits

Do you mean last three, or last four? That is, should I remove all except the first commit, or do you want to include the changes to Error? If the former, I think this PR should still remove the MaybeAlreadyReplaced variant, as discussed in a different comment.

As for submitting those commits as potential refactors, I was thinking I'd open an issue for them first, since in retrospect, they could use some discussion. But also, I don't want to increase your workload unnecessarily, so no worries if the additional commits aren't really something the project is looking to review/adopt at this time.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. IMO ideally this PR could be splitted into three maybe?

I don't think we need extra issues for any of them.

Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,11 @@ impl CodeFix {
pub fn apply_solution(&mut self, solution: &Solution) -> Result<(), Error> {
for r in &solution.replacements {
self.data
.replace_range(r.snippet.range.clone(), r.replacement.as_bytes())?;
self.modified = true;
.replace_range(r.snippet.range.clone(), r.replacement.as_bytes())
.inspect_err(|_| self.data.restore())?;
}
self.data.commit();
self.modified = true;
Ok(())
}

Expand Down
Loading