fix: change handling #167

psteinroe · 2025-01-06T19:05:50Z

iteration on the change handling. after struggling with it for a few days, I simplified it now. its now much simpler and stable, but in certain cases will not be optimal. not optimal means that instead of having one ModifiedStatement, we might get up to three StatementDeleted + AddedStatement. This is relevant for treesitter, where a change can be applied faster if we pass in the prev text and the change. but: since we just parse statement by statement, it should still be fast enough.

Also refactored the Document API to make it much much simpler. Thanks for the input here @juleswritescode!

Screen.Recording.2025-01-10.at.12.48.37.mov

todos:

pr description
hands on test

crates/pg_workspace_new/src/workspace/server/change.rs

juleswritescode

I still need to takea look at apply_change in change.rs, but I have to get going now, so here's already a part of the review :)

I love that it's much simpler now! And I love that auto completion seems to friggin' work.

juleswritescode · 2025-01-10T13:58:48Z

crates/pg_query_ext/src/lib.rs

+            .map(|n| n.0.to_enum())
+            .ok_or_else(|| Error::Parse("Unable to find root node".to_string()))


(I am not even sure why this is an issue. my assumption was that if pg_query parses something it always has at least one node, but that does not seem to be the case.)

juleswritescode · 2025-01-10T14:02:13Z

crates/pg_workspace_new/src/matcher/pattern.rs

@@ -368,7 +368,7 @@ impl Pattern {
    /// `Pattern` using the default match options (i.e. `MatchOptions::new()`).
    pub fn matches_path(&self, path: &Path) -> bool {
        // FIXME (#9639): This needs to handle non-utf8 paths
-        path.to_str().map_or(false, |s| self.matches(s))
+        path.to_str().is_some_and(|s| self.matches(s))


these changes come from the lint in the justfile, right? awesome!

Yes! I don't know why they weren't already reported in a prev pr though.

juleswritescode · 2025-01-10T14:05:59Z

crates/pg_workspace_new/src/workspace/server/document.rs

-            } else {
-                None
-            }
+    pub fn iter_statements(&self) -> impl Iterator<Item = Statement> + '_ {


nit: I believe it's idiomatic that an iter iterates over &T (except if T is copy?)... so idiomatically, this would be an Iterator<Item = &Statement> :)

no need to change it though!

yeah, but we create a new Statement so we cannot return a ref to it. we might be able to optimise this is caching the Statements but its not worth it I think :D

juleswritescode · 2025-01-10T14:17:23Z

crates/pg_workspace_new/src/workspace/server/change.rs

-        }
-    }
+#[derive(Debug, PartialEq, Eq)]
+pub struct ModifiedStatement {


aaah where you inspired by the slatedb approach? Nice!!

not explicitly, this was just the result of simplifying the Document api. the enum was there before already. but maybe it did subconsciously. :D

crates/pg_workspace_new/src/workspace/server/change.rs

juleswritescode · 2025-01-10T14:58:56Z

crates/pg_workspace_new/src/workspace/server/change.rs

+        let mut prev_index = None;
+        let mut next_index = None;
+
+        for (index, (_, pos_range)) in self.positions.iter().enumerate() {


Food for thought: Would it be safer to gather the affected IDs and then maybe compare with a previous set?
This obviously works, but after the change is applied, the indices of the statements will be different – which could lead to bugs further down the road.
Just a feeling!

do you mean bugs within change.rs, or in other parts of the workspace?

within change.rs and anything that manages changes.
It feels to me like mutating an array while iterating over it – but again, just a feeling 👍

mhh yes I think this entire method can be improved. I didn't find a good way to do "atomic" mutations yet though.

juleswritescode · 2025-01-10T15:02:49Z

crates/pg_workspace_new/src/workspace/server/change.rs

+                next_index = Some(index);
+                break;


nit: we're implicitly assuming that the StatementPositions are sorted ascending by range.

If we change the order that the StatementSplitter returns statements, this will break.

Should we make the ordering more explicit?

good point. I added a test to the statement splitter to ensure the output is always sorted. is that sufficient? I think it's easier to make sure its sorted there than to handle it everywhere.

yeah, great solution 👍

crates/pg_workspace_new/src/workspace/server/change.rs

juleswritescode

Great work! Looks much simpler now!

juleswritescode · 2025-01-11T09:28:17Z

crates/pg_workspace_new/src/workspace/server/change.rs


-        self.debug_statements();
+                let new_id = self.id_generator.next();
+                self.positions[affected_idx] = (new_id, new_range);


juleswritescode · 2025-01-11T09:28:37Z

crates/pg_workspace_new/src/workspace/server/change.rs

-}
+                changed.push(StatementChange::Modified(ModifiedStatement {
+                    old_stmt: Statement {
+                        id: old_id,


at this point, that id is only stored here, because statement matching that id in the document is overwritten, right?
Do we at some point after this try to find the old statement by id?

lets get into the mess...

001318d

psteinroe marked this pull request as draft January 6, 2025 19:06

psteinroe commented Jan 6, 2025

View reviewed changes

crates/pg_workspace_new/src/workspace/server/change.rs Outdated Show resolved Hide resolved

psteinroe commented Jan 6, 2025

View reviewed changes

crates/pg_workspace_new/src/workspace/server/change.rs Outdated Show resolved Hide resolved

psteinroe added 3 commits January 9, 2025 10:40

woooorks now and its much simpler

41f3eb8

fix: minor fixes

b7d3cd5

fix: lint --fix

591a81b

psteinroe assigned psteinroe and unassigned psteinroe Jan 10, 2025

psteinroe added 2 commits January 10, 2025 12:44

format

bcba5b3

format

43fd7ca

psteinroe marked this pull request as ready for review January 10, 2025 13:51

psteinroe changed the title ~~lets get into the mess...~~ fix: change handling Jan 10, 2025

pssssssht

f9f50cc

juleswritescode reviewed Jan 10, 2025

View reviewed changes

psteinroe added 2 commits January 10, 2025 15:30

fixes

24493cf

merge main

38f60b9

psteinroe requested a review from juleswritescode January 10, 2025 19:08

juleswritescode approved these changes Jan 11, 2025

View reviewed changes

psteinroe merged commit 20ee92b into main Jan 11, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change handling #167

fix: change handling #167

psteinroe commented Jan 6, 2025 •

edited

Loading

juleswritescode left a comment

juleswritescode Jan 10, 2025

psteinroe Jan 10, 2025

juleswritescode Jan 10, 2025

psteinroe Jan 10, 2025

juleswritescode Jan 10, 2025 •

edited

Loading

psteinroe Jan 10, 2025

juleswritescode Jan 10, 2025

psteinroe Jan 10, 2025

juleswritescode Jan 10, 2025 •

edited

Loading

psteinroe Jan 10, 2025

juleswritescode Jan 11, 2025

psteinroe Jan 11, 2025

juleswritescode Jan 10, 2025

psteinroe Jan 10, 2025 •

edited

Loading

juleswritescode Jan 11, 2025

juleswritescode left a comment

juleswritescode Jan 11, 2025

juleswritescode Jan 11, 2025

		.map(\|n\| n.0.to_enum())
		.ok_or_else(\|\| Error::Parse("Unable to find root node".to_string()))

fix: change handling #167

fix: change handling #167

Conversation

psteinroe commented Jan 6, 2025 • edited Loading

juleswritescode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juleswritescode Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juleswritescode Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psteinroe Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juleswritescode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psteinroe commented Jan 6, 2025 •

edited

Loading

juleswritescode Jan 10, 2025 •

edited

Loading

juleswritescode Jan 10, 2025 •

edited

Loading

psteinroe Jan 10, 2025 •

edited

Loading