Allow UCM commands to have “unprocessed” arguments

[sellout]

Overview

There are some commands (like run) that don’t want expanded arguments passed to them. E.g., run main 1 should pass “1” to main, not the numbered result from a previous command.

Fixes #2805

Implementation notes

This makes a couple changes. First, it creates a Parameters structure that gives a more precise definition of the arguments a command can receive, and eliminates incorrect parameter definitions. It then adds an isStructured field to the ParameterType (née ArgumentType) record, which indicates whether arguments to parameters of that type should be parsed & have numbers expanded.

One of the complicated bits (as seen in foldParamsWithM) is that an “input argument” can be expanded to multiple arguments, and we have to process the arguments left to right, because we don’t know which parameter an argument applies to until after we’ve expanded all the ones ahead of it.

Test coverage

This adds a transcript for run that shows the new behavior (as best we can without capturing run output in transcripts).

Loose ends

This is an improved version of the first half of #5480, which also “fixed” #5193. The work here is also progress toward #5193, but the approach in #5480 isn’t great.

The new transcript here also explicitly use lib.install @unison/base, despite being in the transcripts-using-base directory, which I would think would make that unnecessary.

[aryairani]

Great!

[aryairani]

IIRC, it used to be that the transcript executable actually launched stack exec unison transcript _base.md <another.md>, which applies both transcripts to the same codebase. _base.md just loads and adds base.u.

transcript no longer launches subprocesses, but it looks like the multi-transcript functionality is preserved in the api

unison/unison-cli/transcripts/Transcripts.hs

Lines 129 to 151 in 2e6dd94

	buildTests :: TestConfig -> TestBuilder -> FilePath -> Maybe FilePath -> Test ()
	buildTests TestConfig {..} testBuilder inputDir outputDir = do
	io . putStrLn . unlines $
	[ "",
	"Searching for transcripts to run in: " ++ inputDir
	]
	files <- io $ listDirectory inputDir
	-- Any files that start with _ are treated as prelude
	let (prelude, transcripts) =
	files
	& sort
	& filter (\f -> let ext = takeExtensions f in ext == ".md" || ext == ".markdown")
	& partition (isPrefixOf "_" . snd . splitFileName)
	-- if there is a matchPrefix set, filter non-prelude files by that prefix - or return True
	& second (filter (\f -> maybe True (`isPrefixOf` f) matchPrefix))
	case length transcripts of
	0 -> pure ()
	-- EasyTest exits early with "no test results recorded"
	-- if you don't give it any tests, this keeps it going
	-- till the end so we can search all transcripts for
	-- prefix matches.
	_ -> tests (testBuilder runtimePath inputDir (fromMaybe inputDir outputDir) prelude <$> transcripts)

It looks like, for a given directory that we're going to run transcripts out of, we first look for any transcripts like _xxx.md, and run those first before each individual transcript, which should have the effect of incorporating base in this case (since _base.md is in transcripts-using-base/). So it ShOuLd be working automatically.

Apart from that, I noticed we do see this pattern in many transcripts now, e.g. idempotent/builtins.md

scratch/main> builtins.mergeio
scratch/main> load unison-src/transcripts-using-base/base.u
scratch/main> add

Will that work for fix-2805.md to avoid a clone?

[sellout]

IIRC, it used to be that the transcript executable actually launched stack exec unison transcript _base.md <another.md>, which applies both transcripts to the same codebase. _base.md just loads and adds base.u.
[...]
Will that work for fix-2805.md to avoid a clone?

Also, absent a trace-through on Transcripts.hs, it looks like transcripts saved within transcripts-using-base/ should be getting that automatically! 🤔

So, this is what caused my confusion last week – my previous usage of transcripts-using-base/ worked because base.u included the bits of @unison/base that I needed. But this time I am using things that don’t exist in base.u, and I hadn’t realized that transcripts-using-base/ referred to base.u instead of @unison/base (which is somewhat intentional, since it’s meant to be a subset … but it’s not obvious without working through Transcripts.hs that this is what’s happening), so it was baffling that some definitions from @unison/base worked, while others didn’t.

I don’t need to manually do the load/add (and probably things that explicitly do it in idempotent/ should move to transcripts-using-base/?), but I need to either add a few definitions to base.u or change my transcript to use stuff that already exists in base.u.

And separately, renaming transcripts-using-base/_base.md/base.u to “prelude” or something instead could help avoid this confusion. As well as adding something to transcripts-using-base/README.md about what’s happening.

[aryairani]

So, this is what caused my confusion last week – my previous usage of transcripts-using-base/ worked because base.u included the bits of @unison/base that I needed. But this time I am using things that don’t exist in base.u, and I hadn’t realized that transcripts-using-base/ referred to base.u instead of @unison/base (which is somewhat intentional, since it’s meant to be a subset … but it’s not obvious without working through Transcripts.hs that this is what’s happening), so it was baffling that some definitions from @unison/base worked, while others didn’t.

Gotcha, makes sense.

I don’t need to manually do the load/add (and probably things that explicitly do it in idempotent/ should move to transcripts-using-base/?), but I need to either add a few definitions to base.u or change my transcript to use stuff that already exists in base.u.

I think adding a few definitions to base.u would be ok, or we could update it with the source of a newer version of base, if that works? But really the whole current system is a mess IMO. The "better" (though not good) solution I think would be to have a CI step that preps and caches a codebase with install @unison/base and then fork that codebase for each of the transcripts-using-base/ transcripts (which could or not also be idempotent).

We also have #5543 ready to merge, though I'm not clear on whether this helps in this scenario or not. I'm thinking "no" until we are trying to cache multiple distinct libraries on a transcript-by-transcript basis.

And separately, renaming transcripts-using-base/_base.md/base.u to “prelude” or something instead could help avoid this confusion. As well as adding something to transcripts-using-base/README.md about what’s happening.

Yeah I'm thinking more just that we wait on this step until we come up with a way for the whole thing to go away.

Maybe when Share Sync v2 is finished, we'll have a setting to cache certain library bundles, which would let them be reused efficiently in CI.

---

I guess for now I'd say let's just modify base.u to include whatever you need, or to be a newer version of base, to avoid a slow clone during CI.

A side note, even typechecking base.u is slow, though not as slow as cloning, and it would be nice if we could skip that step too or achieve it faster differently.

[sellout]

Ok, I updated the new test to not install a library, and instead add a few definitions to base.u.

This also fixes a few new/updated commands in the merge to use the new Parameters structure.

[aryairani]

The printLine output doesn't show up in the transcript output; I forget whether it's expected to? But in any case, is-is it doesn't show what we want. Maybe just return the value instead of printing it? Or does main have to return ()?

[sellout]

No, the run output doesn’t get included in the transcript output yet. But yeah, I spaced on being able to return the Text. I’ll fix that. I comforted myself with the fact that the failures were errors, but it still didn’t feel right 😅

[aryairani]

Oh yup.

We should fix #1172 too at some point, but only if it's pretty easy :-\
I could see it getting hairy for more complex handle-manipulation, but maybe it wouldn't?