Skip to content

Support unprocessed UCM command arguments #5470

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

Closed
wants to merge 2 commits into from

Conversation

sellout
Copy link
Contributor

@sellout sellout commented Nov 27, 2024

Overview

Give InputPatterns access to the original String arguments, before expanding numbers or fuzzy resolution. This means that literal numbers can now be provided to commands that take advantage of this (notably run).

Fixes #2805.

Implementation notes

Previously unsupportedStructuredArg was used to error if an argument that shouldn’t be a numbered arg was. That has been replaced with accesses of the original number provided in the UCM command.

Interesting/controversial decisions

Some zero-argument commands used to just ignore extra arguments. They now consistently fail if extra args are provided. This is trivial to revert if it goes against the intention of those commands.

Test coverage

Unfortunately, Unison doesn’t capture run output in transcripts, so this isn’t very testable, but the following example (from #2805) now does the expected thing:

main _ = printLine ("Hello " ++ Optional.getOrElse "" (head !getArgs) ++ "!")
scratch/main> run main 1
Hello 1!

  ()

scratch/main> find isAscii

  ☝️
  
  I couldn't find matches in this namespace, searching in 'lib'...


  1. lib.base.Char.ascii.isAscii : Char -> Boolean
  2. lib.base.Char.ascii.isAscii.doc : Doc
  

scratch/main> run main 1
Hello 1!

  ()

Loose ends

This is a draft because I’m not 100% sold on the implementation (but it was easy).

The most serious issue is that the list of unprocessed arguments doesn’t necessarily line up with the expanded arguments. This is because of ranges. find 1-3 has 1 unprocessed argument (the string "1-3"), but 0-3 processed arguments (the first three numbered args from a previous result). This is an issue for commands like run, where the first arg gets processed, but latter ones aren’t. E.g., with the changes in this PR

  • run 1 2 3 4 will expand 1 to the appropriate numbered arg, then pass "2" 3" "4" as command-line args;
  • run 1–3 4 will expand 1-3 to numbered args, use the first as the function, discard the rest, and pass "4" as a command-line arg (this might be fine, but maybe it should error); but
  • display.to 1-3 4 definitely misbehaves – It uses 1-3 as a filename (correct), but then displays numbered args 2–4 to the file, rather than just numbered arg 4. This is because the string args are ["1-3", "4"] and the processed args are [expandNumber 1, expandNumber 2, expandNumber 3, expandNumber 4]. So when we take the first string arg and drop the first processed arg, we end up with overlap.

A secondary issue is that we add the expanded form to the history, before knowing which arguments the command wants to expand. It executes the correct command, but using the history will result in the incorrect command in future.

The solution to both of these is to give each command the option to incrementally expand arguments as it wants them. The standardParser and bareParser helpers will expand all or none, respectively, leaving just a handful of commands that need to explicitly manage that.

This begins to allow commands to parse things differently than the
current split on whitespace, expand natural numbers, use FZF resolution
approach used by most commands.

In addition to `standardParser`, which replicates the existing behavior,
this also adds a `constParser` for commands that don’t expect any
argument. `constParser` does change the existing behavior in some cases
– some commands would just ignore extra arguments, but now we always
error on them. We can easily restore the “lax” behavior, but I wasn’t
sure if it was intentional or just oversight.
Previously, it was impossible to, say, pass numbers to `run foo`,
because they would be parsed as numbered args, and then would error when
a non-string was given as a command-line arg. This now replaces all
usage of `unsupportedStructuredArgument` (which would fail on numbers)
with handling of the original string.

This improves the following commands:
- `load`
- `display.to`
- `debug.tab-complete`
- `debug.lsp-name-completion`
- `debug.fuzzy-options`
- `help-topics`
- `help`
- `docs.to-html`
- `run`
- `compile`
- `run.native`
- `compile.native`
- `create.author`
- `release.draft`

Fixes unisonweb#2805.
@sellout sellout requested a review from aryairani November 27, 2024 21:09
@sellout
Copy link
Contributor Author

sellout commented Nov 29, 2024

I have a different approach that leans on ArgumentType rather than the parsers. It addresses the loose ends as well.

@sellout sellout closed this Nov 29, 2024
@sellout sellout deleted the avoid-numbered-args branch December 4, 2024 23:23
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.

ucm run : Numeric arguments are changed to find results
1 participant