Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Reference from StructuredArgument #5019

Merged
merged 2 commits into from
May 30, 2024

Conversation

sellout
Copy link
Contributor

@sellout sellout commented May 29, 2024

Overview

Almost everywhere we produce a Reference for numbered args, we also have a
HashQualified Name handy, which is much more consumable by commands.

The only case we don’t have an HQ is in the todo command output, so that now explicitly builds
a HQ.HashOnly.

This also fixes an issue with StructuredArgument handling where alias.term
and alias.type wouldn’t make an alias to a HQ.HashOnly StructuredArgument.

Fixes #4898.

Test coverage

There is a new transcript to replicate the reported issue.

Almost everywhere we produce a `Reference` for numbered args, we also have a
`HashQualified Name` handy, which is much more consumable by commands.

The only case we don’t have an `HQ` is in the `todo` command output, so that now explicitly builds
a `HQ.HashOnly`.

This also fixes an issue with `StructuredArgument` handling where `alias.term`
and `alias.type` wouldn’t make an alias to a `HQ.HashOnly` `StructuredArgument`.

Fixes unisonweb#4898.
@sellout sellout marked this pull request as ready for review May 29, 2024 23:32
@aryairani
Copy link
Contributor

aryairani commented May 30, 2024

The following comments aren't specifically about this PR, but:

Almost everywhere we produce a Reference for numbered args, we also have a HashQualified Name handy, which is much more consumable by commands.

Makes me think we should have something isomorphic to (Name, Reference) rather than These Name ShortHash.

The only case we don’t have an HQ is in the todo command output, so that now explicitly builds a HQ.HashOnly.

todo does have a name available that it prints though, it just doesn't load it into numbered-args? or no?

@aryairani aryairani merged commit 0f597f1 into unisonweb:trunk May 30, 2024
19 checks passed
@sellout
Copy link
Contributor Author

sellout commented May 30, 2024

The only case we don’t have an HQ is in the todo command output, so that now explicitly builds a HQ.HashOnly.

todo does have a name available that it prints though, it just doesn't load it into numbered-args? or no?

Yes, so I’m actually a bit confused about when exactly the Reference would get used. The change in question is https://github.com/unisonweb/unison/pull/5019/files#diff-db89d574f836d421e840d72b91dad3f90f89f3a897d448c812ee1d441a3e7630L1442-R1442, and you can see just two lines below setNumberedArgs is respondNumbered $ TodoOutput. And when that TodoOutput is processed, it calls setNumberedArgs using HashQualified Name … so is the linked setNumberedArgs just dead code, or is that there in case something else fails along the way and the later HashQualified args never get set?

@aryairani
Copy link
Contributor

Oh weird. It looks like dead code to me.

@sellout sellout deleted the delete-dependents branch May 30, 2024 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

delete.term with numbered arg fails and with a cryptic message
2 participants