4a2337d · helix · git.iko.soy

git.iko.soy/helix

on master help sign in

correctly map unsorted positions (#7471)

4a2337dPascal Kuthe3 years ago

helix-core/src/transaction.rs+94−36

compare with master

unifiedsplit

@@ -326,7 +326,7 @@ impl ChangeSet {

326 self.changes.is_empty() || self.changes == [Operation::Retain(self.len)]

327 }

328

329− /// Map a *sorted* list of positions through the changes.

329+ /// Map a (mostly) *sorted* list of positions through the changes.

330 ///

331 /// This is equivalent to updating each position with `map_pos`:

332 ///

@@ -335,27 +335,63 @@ impl ChangeSet {

335 /// *pos = changes.map_pos(*pos, assoc);

336 /// }

337 /// ```

338− /// However this function is significantly faster running in `O(N+M)` instead of `O(NM)`

338+ /// However this function is significantly faster for sorted lists running

339+ /// in `O(N+M)` instead of `O(NM)`. This function also handles unsorted/

340+ /// partially sorted lists. However, in that case worst case complexity is

341+ /// again `O(MN)`. For lists that are often/mostly sorted (like the end of diagnostic ranges)

342+ /// performance is usally close to `O(N + M)`

339 pub fn update_positions<'a>(&self, positions: impl Iterator<Item = (&'a mut usize, Assoc)>) {

343 pub fn update_positions<'a>(&self, positions: impl Iterator<Item = (&'a mut usize, Assoc)>) {

340 use Operation::*;

344 use Operation::*;

341

345

342 let mut positions = positions.peekable();

346 let mut positions = positions.peekable();

343− macro_rules! map {

344− ($map: expr) => {

345− loop {

346− let Some((pos, assoc)) = positions.peek_mut() else { return; };

347− let Some(new_pos) = $map(**pos, *assoc) else { break; };

348− **pos = new_pos;

349− positions.next();

350− }

351− };

352− }

353

347

354 let mut old_pos = 0;

348 let mut old_pos = 0;

355 let mut new_pos = 0;

349 let mut new_pos = 0;

356− let mut iter = self.changes.iter().peekable();

350+ let mut iter = self.changes.iter().enumerate().peekable();

351+

352+ 'outer: loop {

353+ macro_rules! map {

354+ ($map: expr, $i: expr) => {

355+ loop {

356+ let Some((pos, assoc)) = positions.peek_mut() else { return; };

357+ if **pos < old_pos {

358+ // Positions are not sorted, revert to the last Operation that

359+ // contains this position and continue iterating from there.

360+ // We can unwrap here since `pos` can not be negative

361+ // (unsigned integer) and iterating backwards to the start

362+ // should always move us back to the start

363+ for (i, change) in self.changes[..$i].iter().enumerate().rev() {

364+ match change {

365+ Retain(i) => {

366+ old_pos -= i;

367+ new_pos -= i;

368+ }

369+ Delete(i) => {

370+ old_pos -= i;

371+ }

372+ Insert(ins) => {

373+ new_pos -= ins.chars().count();

374+ }

375+ }

376+ if old_pos <= **pos {

377+ iter = self.changes[i..].iter().enumerate().peekable();

378+ }

379+ }

380+ debug_assert!(old_pos <= **pos, "Reverse Iter across changeset works");

381+ continue 'outer;

382+ }

383+ let Some(new_pos) = $map(**pos, *assoc) else { break; };

384+ **pos = new_pos;

385+ positions.next();

386+ }

387+ };

388+ }

389+

390+ let Some((i, change)) = iter.next() else {

391+ map!(|pos, _| (old_pos == pos).then_some(new_pos), self.changes.len());

392+ break;

393+ };

357

394

358− while let Some(change) = iter.next() {

359 let len = match change {

395 let len = match change {

360 Delete(i) | Retain(i) => *i,

396 Delete(i) | Retain(i) => *i,

361 Insert(_) => 0,

397 Insert(_) => 0,

@@ -364,42 +400,51 @@ impl ChangeSet {

364

400

365 match change {

401 match change {

366 Retain(_) => {

402 Retain(_) => {

403+ map!(

367− map!(|pos, _| (old_end > pos).then_some(new_pos + (pos - old_pos)));

404+ |pos, _| (old_end > pos).then_some(new_pos + (pos - old_pos)),

405+ i

406+ );

368 new_pos += len;

407 new_pos += len;

369 }

408 }

370 Delete(_) => {

409 Delete(_) => {

371 // in range

410 // in range

372− map!(|pos, _| (old_end > pos).then_some(new_pos));

411+ map!(|pos, _| (old_end > pos).then_some(new_pos), i);

373 }

412 }

374 Insert(s) => {

413 Insert(s) => {

375 let ins = s.chars().count();

414 let ins = s.chars().count();

376

415

377 // a subsequent delete means a replace, consume it

416 // a subsequent delete means a replace, consume it

378− if let Some(Delete(len)) = iter.peek() {

417+ if let Some((_, Delete(len))) = iter.peek() {

379 iter.next();

418 iter.next();

380

419

381 old_end = old_pos + len;

420 old_end = old_pos + len;

382 // in range of replaced text

421 // in range of replaced text

422+ map!(

383− map!(|pos, assoc| (old_end > pos).then(|| {

423+ |pos, assoc| (old_end > pos).then(|| {

384− // at point or tracking before

424+ // at point or tracking before

385− if pos == old_pos || assoc == Assoc::Before {

425+ if pos == old_pos || assoc == Assoc::Before {

386− new_pos

426+ new_pos

387− } else {

427+ } else {

388− // place to end of insert

428+ // place to end of insert

389− new_pos + ins

429+ new_pos + ins

430+ }

390− }

431+ }),

432+ i

391− }));

433+ );

392 } else {

434 } else {

393 // at insert point

435 // at insert point

436+ map!(

394− map!(|pos, assoc| (old_pos == pos).then(|| {

437+ |pos, assoc| (old_pos == pos).then(|| {

395− // return position before inserted text

438+ // return position before inserted text

396− if assoc == Assoc::Before {

439+ if assoc == Assoc::Before {

397− new_pos

440+ new_pos

398− } else {

441+ } else {

399− // after text

442+ // after text

400− new_pos + ins

443+ new_pos + ins

444+ }

401− }

445+ }),

446+ i

402− }));

447+ );

403 }

448 }

404

449

405 new_pos += ins;

450 new_pos += ins;

@@ -407,7 +452,6 @@ impl ChangeSet {

407 }

452 }

408 old_pos = old_end;

453 old_pos = old_end;

409 }

454 }

410− map!(|pos, _| (old_pos == pos).then_some(new_pos));

411 let out_of_bounds: Vec<_> = positions.collect();

455 let out_of_bounds: Vec<_> = positions.collect();

412

456

413 panic!("Positions {out_of_bounds:?} are out of range for changeset len {old_pos}!",)

457 panic!("Positions {out_of_bounds:?} are out of range for changeset len {old_pos}!",)

@@ -822,6 +866,20 @@ mod test {

822 };

866 };

823 assert_eq!(cs.map_pos(2, Assoc::Before), 2);

867 assert_eq!(cs.map_pos(2, Assoc::Before), 2);

824 assert_eq!(cs.map_pos(2, Assoc::After), 2);

868 assert_eq!(cs.map_pos(2, Assoc::After), 2);

869+ // unsorted selection

870+ let cs = ChangeSet {

871+ changes: vec![

872+ Insert("ab".into()),

873+ Delete(2),

874+ Insert("cd".into()),

875+ Delete(2),

876+ ],

877+ len: 4,

878+ len_after: 4,

879+ };

880+ let mut positions = [4, 2];

881+ cs.update_positions(positions.iter_mut().map(|pos| (pos, Assoc::After)));

882+ assert_eq!(positions, [4, 2]);

825 }

883 }

826

884

827 #[test]

885 #[test]

helix-view/src/document.rs+1−1

@@ -1197,7 +1197,7 @@ impl Document {

1197 if let Some(data) = Rc::get_mut(annotations) {

1198 changes.update_positions(

1199 data.iter_mut()

1200− .map(|diagnostic| (&mut diagnostic.char_idx, Assoc::After)),

1200+ .map(|annotation| (&mut annotation.char_idx, Assoc::After)),

1201 );

1202 }

1203 };