feat: overhaul delete implementation #5894

mitchellwrosen · 2025-09-24T20:39:07Z

Overview

This PR overhauls the delete implementation to kick you into an update mode if you try to delete a set of things that would leave behind something with a nameless dependency (rather than just say "I can't do that" and print out the dependents that block the delete).

The update mode functions exactly like a failed update. You get put on a branch (with same update- prefix convention, not delete-) with the delete targets deleted, and all of the transitive dependents of the subset of delete targets that no longer have any name are put into the scratch file, and don't exist in the underlying namespace. So, if you comment them all out, for example, you'll see the slurp output indicate an update would effectively "delete" them all by merging the branch back into its parent. The top of the scratch file says:

-- The definitions below depend on the deleted definitions.
-- Please fix the errors and run `update`.

You can't run delete while in the middle of resolving an update, upgrade, or delete.

This implementation essentially relies on a couple healthy-namespace checks that are preconditions of merge and update as well: you can't have any conflicted names, and your type declarations must all pass all the checks (one name per constructor, each nested underneath its type in the namespace, etc).

However, certain situations could previously have been solved with a delete, such as deleting a stray constructor. So, I've added back a sort of dumb version, delete.force, which just deletes all of the names you give it, without any restrictions. I tried to update various error messages to mention delete.force as necessary, too. I don't expect delete.force to be particularly common, but it is useful in transcripts as a way of setting up test scenarios, like a type without a name for one of its constructors.

delete and delete.type now delete all of a type's constructors (woohoo). You can't delete a constructor with delete or delete.term anymore.

I also added a suffix-resolving mechanism to delete: delete foo.bar will delete the thing called foo.bar, or if it doesn't exist, everything with the suffix foo.bar. If constructors and terms exist that match the suffix, the constructors are ignored. If only constructors exist, you get the "I can't delete constructors" error message. My thinking here was that getting blocked by an error message is annoying, and there's always undo if you deleted more than intended. However, I think this might have been the wrong call, and a "which did you mean?" error message, with numbered args, would be better overall. WDYT?

I deleted the .verbose variants of delete in favor of just always printing out the list of names that were deleted. I don't think you ever want to just see "Done" without any idea of precisely what was deleted.

Test coverage

I added a couple new cases to the main delete.md transcript.

aryairani · 2025-10-06T14:33:49Z

I couldn't find this case of deleting both terms and types in the transcript coverage, could you add it (or point me to it)?

aryairani · 2025-10-06T16:22:49Z

Here's what I'm thinking:

  I couldn't delete <it/them> because <it's/they're> still being used
  in other definitions.
  
  I've created a temporary branch and added the affected
  definitions to <scratch.u>, where you can fix them up or remove 
  any that are obsolete.
  
  Once you're happy with the results, use `update` to merge them back
  into <start-branch>, or `cancel` if you change your mind.

Happy path, each name listed has one exact match.

quiet-gecko/main> delete a b c

  Ok.

  Tip: You can use `undo` or `reflog` to undo this change.

Any name has multiple matches, or any name has zero matches:

quiet-gecko/main> delete a b c

  I deleted these definitions:

    1. type a
    2. a
    3. b
 
  Tip: You can use `undo` or use a hash from `reflog` to undo this change.

  I couldn't find any definitions called `c`[, comma-separated list].

or if you prefer:

quiet-gecko/main> delete a b c

  I deleted these types:

    1. type a

  and these terms:

    2. a
    3. b
 
  Tip: You can use `undo` or use a hash from `reflog` to undo this change.

  I couldn't find any definitions called `c`[, comma-separated list].

If delete only takes a single argument for now, then these are still okay. ^

Follow-up PR to make everything consistent:

update failure

  I couldn't complete the update, because some existing definitions would no longer typecheck.
  
  I've created a temporary branch and added the affected
  definitions to <scratch.u>, where you can fix them up or remove 
  any that are obsolete.
  
  Once you're happy with the results, use `update` to merge them back
  into <start-branch>, or `cancel` if you change your mind.

merge failure

  I couldn't automatically merge scratch/bob into scratch/alice.
  
  I've created a temporary branch and added the affected
  definitions to <scratch.u>, where you can fix them up or remove 
  any that are obsolete.
  
  Once you're happy with the results, use `update` to merge them back
  into <start-branch>, or `cancel` if you change your mind.

upgrade failure

  I couldn't automatically upgrade <old-lib> to <new-lib>.
  
  I've created a temporary branch and added the affected
  definitions to <scratch.u>, where you can fix them up or remove 
  any that are obsolete.

  Once you get it typechecking, `update` will merge the results
  back into <start-branch>.

aryairani · 2025-09-27T20:53:20Z

unison-src/transcripts/idempotent/help.md

  `delete.branch /bar`     deletes the branch `bar` in the
                           current project

+  delete.force


Do we still need delete.force, with this PR?

Yeah, it's needed if precondition checks prevent a delete, like if you are trying to delete a stray constructor. I tried to make all of the error messages mention it, not delete, where appropriate.

aryairani · 2025-09-27T20:54:14Z

unison-src/transcripts/idempotent/todo.md

    2. bar#cq22mm4sca

-  Tip: Use `move.term` or `delete.term` to resolve the
+  Tip: Use `move.term` or `delete.term.force` to resolve the


Oh hm. What happens if you do a regular delete.term on a conflicted name? (And is that the obvious right thing, and why?)

You would have to use delete.term.force to delete a conflicted name, since delete.term has precondition checks including no conflicted name. I think that's the obvious right thing given how delete works like merge and update that have the same preconditions and share a lot of code that takes advantage of them. Ideally there wouldn't be a restriction but in practice I don't think it will matter much because conflicted names aren't really a thing.

aryairani · 2025-09-27T20:56:53Z

unison-src/transcripts/idempotent/update-on-conflict.md

-  This branch has more than one term with the name `x`. Please
-  delete or rename all but one of them, then try the update
-  again.
+  Sorry, I can't do that right now, because there's more than


This is a kind of weird outcome... I guess only one of the x might be in the scratch file?

aryairani · 2025-09-27T20:57:46Z

unison-src/transcripts/idempotent/update-type-nested-decl-aliases.md

  update when a type exists nested under an alias of itself.
-  Please separate them or delete one copy, and then try updating
-  again.
+  Please separate them or `delete.force` one copy, and then try


What happens if you just use delete?

You can't use delete to delete something when there's this precondition failure, you have to use delete.force.

mitchellwrosen · 2025-10-06T20:30:42Z

I couldn't find this case of deleting both terms and types in the transcript coverage, could you add it (or point me to it)?

That's here: https://github.com/unisonweb/unison/pull/5894/files#diff-57a541f8982f7096fe2317de8c11c32d443bbb68030c37fa940d06e76010f71fR106-R118

aryairani · 2025-10-07T03:01:11Z

I couldn't find this case of deleting both terms and types in the transcript coverage, could you add it (or point me to it)?

That's here: #5894 (files)

Thanks—erm for some reason that link doesn't work for me in Safari or Chrome. What file is it?

mitchellwrosen · 2025-10-07T03:10:55Z

It's in delete.md

mitchellwrosen · 2025-10-07T15:42:51Z

Regarding this suggestion,

  I couldn't delete <it/them> because <it's/they're> still being used
  in other definitions.
  
  I've created a temporary branch and added the affected
  definitions to <scratch.u>, where you can fix them up or remove 
  any that are obsolete.
  
  Once you're happy with the results, use `update` to merge them back
  into <start-branch>, or `cancel` if you change your mind.

Here is the current wording in the PR:

Some definitions depend on the ones you're trying to delete. I've added them to <file>,
where you can fix them or comment them out. Once the file is compiling, run `update`.

I've also switched you to a new branch <branch> for this work. On `update`, it will be
merged back into <branch>.

And I based this on the existing message for an upgrade failure, which is:

Some definitions don't typecheck with your changes. I've updated the file <file> with
the definitions that need fixing. Once the file is compiling, try `update` again.

I've also switched you to a new branch <branch> for this work. On `update`, it will be
merged back into <branch>.

So, I feel if we change the "delete failed" output message I feel we should change the "update failed" message to match. Otherwise, I think the inconsistency is probably unnecessary and maybe jarring.

mitchellwrosen · 2025-10-07T15:45:26Z

Regarding this,

quiet-gecko/main> delete a b c

  Ok.

  Tip: You can use `undo` or `reflog` to undo this change.

I actually don't think it would be better to have a special case that just says "Ok.". For one thing, it's weird to get feedback sometimes but not others. But also, since delete performs suffix matching, it is useful to see the full name of the thing that was deleted, even if only one thing matched. Example:

quiet-gecko/main> delete getRoot

  I deleted these terms:

    1. handler.getRoot
 
  Tip: You can use `undo` or use a hash from `reflog` to undo this change.

mitchellwrosen · 2025-10-07T16:05:58Z

Regarding the suggestion to handle the "no match" case differently, there's on additional source of short-circuiting: constructor names.

In the current PR, every argument is resolved as follows:

If name matches nothing, fail with "not found"
Else name matches only constructors, fail with "I can't delete constructors"
Else ignore constructor matches

So, you can delete x even if there's both a term and a constructor named x – we just delete the term and don't tell you "I can't delete constructors named x". But if you delete Some, and the only things named Some in scope are Optional.Some and Maybe.Some, both constructors, then you'll get the "I can't delete constructors".

Anyway, I can see the appeal of not failing on (1) but instead recording those for later. However, do we want to do something analogous for (2), i.e. if any name resolves only to constructors, don't fail the whole delete, but rather note that in the output message later? (I think so).

And either way: could we push that refactoring to a follow-up PR? (Just in the interest of merging improvements quickly and iterating)

aryairani · 2025-10-08T00:52:25Z

So, I feel if we change the "delete failed" output message I feel we should change the "update failed" message to match. Otherwise, I think the inconsistency is probably unnecessary and maybe jarring.

Did you see the changed "update/upgrade/merge failed" messages in the same comment with the "delete failed" message? I do want to make all four of them consistent.

aryairani · 2025-10-08T01:22:39Z

I actually don't think it would be better to have a special case that just says "Ok.". For one thing, it's weird to get feedback sometimes but not others. But also, since delete performs suffix matching, it is useful to see the full name of the thing that was deleted, even if only one thing matched. Example:
quiet-gecko/main> delete getRoot

  I deleted these terms:

    1. handler.getRoot
 
  Tip: You can use `undo` or use a hash from `reflog` to undo this change.

Right, I see. The idea here was to give feedback if and only if there was meaningful feedback to give; a helpful signal as to when/whether it's worth reading the message at all.

I agree it could be useful to show the expanded name where relevant. If the user fully explicitly gives the name though, and then we do exactly what they, then telling them back exactly what we did is a waste of reading that the user can't even identify until they finish reading it. If deleting by suffix is the primary way we end up using it though, then maybe that explicit case is too rare for it to matter. I had missed that you added capability that in this PR. We can try it!

Anyway, I can see the appeal of not failing on (1) but instead recording those for later. However, do we want to do something analogous for (2), i.e. if any name resolves only to constructors, don't fail the whole delete, but rather note that in the output message later? (I think so).

I think so.

And either way: could we push that refactoring to a follow-up PR? (Just in the interest of merging improvements quickly and iterating)

Yes, will you create a ticket for whatever gets pushed though.

mitchellwrosen · 2025-10-10T15:59:46Z

Planning on doing these in follow-up:

Change wording of delete/update/upgrade/merge as suggested above
Allow bad args (no matches, only constructor matches) in a delete, just mention in output message what happened

I don't think we should pursue this one, though:

If the user fully explicitly gives the name though, and then we do exactly what they, then telling them back exactly what we did is a waste of reading that the user can't even identify until they finish reading it.

That pretty much goes against delete UX best practices as far as I understand. Maybe @hojberg could chime in. Basically, we shouldn't minimize or eliminate feedback about what was deleted in a CLI context where the user isn't, like, clicking on an X to delete. It's very important to assure to the user they didn't delete something other than what they expected and build trust in the tool. (Our current implementation fails here, just says "Done." unless you explicitly ask for feedback with a .verbose variant).

aryairani · 2025-10-13T18:31:03Z

I don't think we should pursue this one, though:

If the user fully explicitly gives the name though, and then we do exactly what they, then telling them back exactly what we did is a waste of reading that the user can't even identify until they finish reading it.

That pretty much goes against delete UX best practices as far as I understand. Maybe @hojberg could chime in. Basically, we shouldn't minimize or eliminate feedback about what was deleted in a CLI context where the user isn't, like, clicking on an X to delete. It's very important to assure to the user they didn't delete something other than what they expected and build trust in the tool. (Our current implementation fails here, just says "Done." unless you explicitly ask for feedback with a .verbose variant).

I think I understand what you're saying (I won't summarize, because I think you stated it clearly).

I would say that verbosity is important exactly when you already don't trust the tool. I'm not sure if we are in that situation currently; though maybe we are. Long term, we're not; knock on wood.

Consider:

# User was explicit - verbose output is redundant
$ delete-thing foo-bar-baz-v1.2.3
Deleted: foo-bar-baz-v1.2.3  # ← no new info, 
# I have to compare characters to determine what happened

# Tool extrapolated - verbose output adds info
$ delete-thing foo
Deleted: foo-bar-baz-v1.2.3
Deleted: foo-test-v0.9.1

$ delete-thing foo
Error: Couldn't find `foo`

The "reading fatigue" issue is underrated in CLI design. Every line of output has a cost, it:

Takes time to read
Contributes to scrolling important info off screen
Trains users to skim/ignore output / makes it harder to spot when something actually unusual happens — e.g. the tool has extrapolated a full name based on the argument.

Even a message like "Done (deleted exact match)." or something, is better than having to scrutinize the standard output template to determine whether the thing you asked for happened, or if the result was different.

As far as: what do we do right now? I'm not sure. If my proposal is hard, let's skip it for now.

mitchellwrosen and others added 7 commits September 17, 2025 09:39

move delete handler into its own module

3d0a337

tweak DeleteTarget

8f6473d

⅄ trunk → delete-branch

b9f2d64

⅄ trunk → delete-branch

01b85b4

overhaul delete implementation

5f59dbd

automatically run ormolu

37e0d04

fix transcripts

f98805d

unisonweb deleted a comment from github-actions bot Sep 25, 2025

mitchellwrosen added 6 commits September 25, 2025 11:54

delete unused symbols

3666966

output message tweaks

809faf2

delete unused import

ca0d624

delete unused imports

6e973d8

deleted numbered args

157ccb0

⅄ trunk → delete-branch

6c451f9

unisonweb deleted a comment from github-actions bot Sep 25, 2025

mitchellwrosen marked this pull request as ready for review September 25, 2025 18:11

mitchellwrosen requested a review from aryairani September 25, 2025 18:11

aryairani reviewed Oct 6, 2025

View reviewed changes

⅄ trunk → delete-branch

cf99e59

⅄ trunk → delete-branch

b7baa41

⅄ trunk → delete-branch

b3eb38d

mitchellwrosen added 3 commits October 14, 2025 10:54

weeder

a9f78b8

⅄ trunk → delete-branch

d981c90

update transcripts

48f7057

aryairani approved these changes Oct 17, 2025

View reviewed changes

aryairani merged commit d716c51 into trunk Oct 17, 2025
32 checks passed

aryairani deleted the delete-branch branch October 17, 2025 03:33

mitchellwrosen mentioned this pull request Oct 27, 2025

standardize wording of failed delete/update/upgrade/merge #5953

Merged

feat: overhaul delete implementation #5894

feat: overhaul delete implementation #5894

Uh oh!

Conversation

mitchellwrosen commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Test coverage

Uh oh!

aryairani commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aryairani commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aryairani Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

mitchellwrosen Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

aryairani Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

mitchellwrosen Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

aryairani Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

aryairani Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

mitchellwrosen Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

mitchellwrosen commented Oct 6, 2025

Uh oh!

aryairani commented Oct 7, 2025

Uh oh!

mitchellwrosen commented Oct 7, 2025

Uh oh!

mitchellwrosen commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitchellwrosen commented Oct 7, 2025

Uh oh!

mitchellwrosen commented Oct 7, 2025

Uh oh!

aryairani commented Oct 8, 2025

Uh oh!

aryairani commented Oct 8, 2025

Uh oh!

mitchellwrosen commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aryairani commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mitchellwrosen commented Sep 24, 2025 •

edited

Loading

aryairani commented Oct 6, 2025 •

edited

Loading

aryairani commented Oct 6, 2025 •

edited

Loading

mitchellwrosen commented Oct 7, 2025 •

edited

Loading

mitchellwrosen commented Oct 10, 2025 •

edited

Loading

aryairani commented Oct 13, 2025 •

edited

Loading