unison icon indicating copy to clipboard operation
unison copied to clipboard

show fully qualified names on error about ambiguous suffix

Open aryairani opened this issue 1 year ago • 6 comments

From @ceedubs:

When we complain about an ambiguous suffix, we should show the whole very.long.name.wow.so.long, not a suffixified wow.so.long (so that debugging the issue is easier).

From @aryairani

Can we look at the specific situation where not having the full name is making debugging issues harder?

aryairani avatar Sep 26 '24 19:09 aryairani

Can we look at the specific situation where not having the full name is making debugging issues harder?

Sure. Here is the original error message that I was trying to debug:

image (1)

The fact that it says or instead of + is covered by #5371. But even if it used + and just given a unique suffix, it sould have been something like

  • unison_base_1.2.3.Char.Class.+
  • up.base.Char.Class.+

From this output it isn't at all obvious where that bottom one is coming from. Maybe I can figure out with names?

Edit I realized that I switched projects at this point and that names does show the result in this aws lib since it is a direct and not a transitive dependency. But I'm leaving this around because I think that you can run into similar situations that are confusing.

@cloud/nimbus/main> names Class.+
 
  Term
  Hash:   ##Char.Class.or
  Names:  lib.ansi_1_0_0.lib.base_2_9_1.Char.Class.+
          lib.ansi_1_0_0.lib.base_2_9_1.Char.Class.or
          lib.unison_aws_4_1_0.lib.ini_1_0_1.lib.base_2_8_0.Char.Class.+
          lib.unison_aws_4_1_0.lib.ini_1_0_1.lib.base_2_8_0.Char.Class.or
          lib.unison_aws_4_1_0.lib.json_1_0_0.lib.json.lib.base.Char.Class.or
          lib.unison_base_3_11_2.Char.Class.+
          lib.unison_base_3_11_2.Char.Class.or
          lib.unison_base_3_14_0.Char.Class.+
          lib.unison_base_3_14_0.Char.Class.or

Nope none of those are the one that is causing the ambiguity. If I do a lot of sleuthing I can find it with a very explicit names command:

@cloud/nimbus/main> names lib.unison_aws_4_1_0.lib.xml_1_1_3.up.base.Char.Class.+
 
  Term
  Hash:   #p3s3njphif
  Names:  lib.unison_aws_4_1_0.lib.xml_1_1_3.up.base.Char.Class.+

If the ambiguity error message had used lib.unison_aws_4_1_0.lib.xml_1_1_3.up.base.Char.Class.+ in the first place it would have saved me a lot of confusion.

Though separately it seems like an issue that a term would be considered ambiguous but wouldn't actually show up when I search for it with names.

ceedubs avatar Sep 27 '24 13:09 ceedubs

Here is another example:

image

The supplied name of one of those is a suffix of the other, so it really isn't even providing a unique suffix. In this case they are both actually the same thing (same hash), but the up one has been added to my scratch file with edit.namespace and the other one is from a transitive dependency.

ceedubs avatar Sep 27 '24 13:09 ceedubs

Ok makes sense, thanks.

aryairani avatar Sep 27 '24 17:09 aryairani

The supplied name of one of those is a suffix of the other, so it really isn't even providing a unique suffix. In this case they are both actually the same thing (same hash), but the up one has been added to my scratch file with edit.namespace and the other one is from a transitive dependency.

Hrm, that seems like a bug. A suffix matching an indirect dependency and something in your file should resolve to the thing in the file.

mitchellwrosen avatar Sep 27 '24 17:09 mitchellwrosen

@ceedubs I've confirmed at least in a simple case that is the behavior:

```ucm
scratch/main> builtins.merge lib.builtin
```

```unison
lib.foo.lib.bar.honk = 10
my.honk = 20
```

```ucm
scratch/main> add
```

```unison
thing = honk + honk
> thing
```
    2 | > thing
          ⧩
          40

Could you try to capture a minimal repro if possible?

mitchellwrosen avatar Sep 27 '24 17:09 mitchellwrosen

@mitchellwrosen I am also having trouble reproducing the non-unique suffix issue with a minimal example. There have been a few changes in this area recently and it's possible that one of them addresses this, so I'd say don't worry about it unless it comes up again.

But I still think that error messages about ambiguous names should show a fully-qualified name.

ceedubs avatar Sep 27 '24 17:09 ceedubs