unisonweb / unison

A friendly programming language from the future
https://unison-lang.org
Other
5.75k stars 269 forks source link

delete.term doesn't always work for dependents results #5083

Open ceedubs opened 3 months ago

ceedubs commented 3 months ago

Describe and demonstrate the bug

If you run dependents and then try to delete.term with a numbered result it doesn't work (or at least not always).

Below you can see that it works if I manually select the fully-qualified name.

Screenshots

@cloud/nimbus/cleanup-2> dependents up.codec.Decode.variableSizeBytes

  Dependents of: codec.Decode.variableSizeBytes

    Terms:

    1. variableSizeBytes.tests.success

  Tip: Try `view 1` to see the source of any numbered item in the above list.

InputPattern: 937 ms (cpu), 937 ms (system)
@cloud/nimbus/cleanup-2> delete.term 1
@cloud/nimbus/cleanup-2> delete.term variableSizeBytes.tests.success

  ⚠️

  The following names were not found in the codebase. Check your spelling.
    variableSizeBytes.tests.success

InputPattern: 572 µs (cpu), 2.07 ms (system)
@cloud/nimbus/cleanup-2> delete.term    
Select a definition to delete:
@cloud/nimbus/cleanup-2> delete.term up.codec.encode.variableSizeBytes.tests.success

updateRoot: 48.9 ms (cpu), 37.7 ms (system)

  Done.

Environment (please complete the following information):

Additional context

It's possible that this is a regression that was introduced at the same time that #5055 was, but I haven't verified that. cc @sellout

sellout commented 3 months ago

dependents returns numbered arguments as Name, and delete.term (and all the delete.* commands) expects HQSplit'. The conversion should be (and appears to be) lossless.

Unfortunately, dependents foo; delete 1 always fails prior to the structured arguments work because of #4898, so it’s hard to tell if this particular problem existed before.

transcript ```ucm:hide .> project.create test-5083 test-5083/main> builtins.merge ``` ```unison foo = 1 + 1 meh.bar = foo + foo baz = foo + bar ``` ```ucm test-5083/main> add test-5083/main> find bar test-5083/main> dependents foo test-5083/main> debug.numberedArgs ``` The results of `dependents` are `Name`s. We can easily delete `baz` based on the `dependents` results. ```ucm test-5083/main> delete.term 2 ``` We can `view` `bar` from the `dependents` results (which expects a `HashQualified Name`). ```ucm test-5083/main> view 1 ``` but deleting (which expects a `HQSplit'`) `bar` (which is in a deeper namespace) fails. ```ucm:error test-5083/main> delete.term 1 ``` `rename.term`, which also expects a `HQSplit'`, also fails. ```ucm:error test-5083/main> rename.term 1 quux ``` Using the expanded term also fails. ```ucm:error test-5083/main> delete.term bar ``` But adding the missing namespace works. ```ucm test-5083/main> delete.term meh.bar ```

The issue seems to be that the Names returned by dependents aren’t qualified enough for HQSplit'-consuming commands to find them (understandable that we don‘t want delete to delete some random term with the same name that happens to be found).

I don’t think this is related to the numbered args changes, because I don’t see anything relevant around where dependentsNames are produced.

I think this is another face of #1298 – if we had absolute (or hash-qualified) names produced, there would be no confusion/ambiguity when they’re presented to delete.term.