fix: Tests.Action.Prog: defaultPrimsProg clobbers 'main'

[brprice]

The definition of defaultEmptyProg is a hack, hardcoding an ID of 0 for
'main' and 2 for 'other', regardless of whether they are fresh or not.
This leads to two different defs having ID 0 in defaultPrimsProg as
normally used in progActionTest, and it turns out that 'main' is
overwritten with a primitive definition. Thus one cannot do any edits to
ID 0 in tests.

Note that whilst we have worked around this symptom, the implementation
of defaultEmptyProg is still wrong: these IDs will clash with some node
IDs in the current usage.

[brprice]

Reviewer note: this targets brprice/rename-prim-clash, but doesn't really depend on it, I just am submitting the PR as it appears in my history. I am happy to rebase onto main if wanted.

[dhess]

I'd prefer to fix the core issue. It shouldn't be difficult, right?

(In one of my recent testing commits, I needed to construct some a non-trivial test program for use in an App and ended up copying some code into the test harness from an unexported bit of code in Primer.App. Point is — we should probably make it easier to construct valid programs programatically.)

[brprice]

I'd prefer to fix the core issue. It shouldn't be difficult, right?

I don't think it will be too difficult, but is a pain. The way I can see to do it is to return the ID of main and other alongside the Prog, or to just look them up by name. In either case, this requires making a bunch of tests less readable. Perhaps you have a better idea in mind?

I agree in principle, and am happy to circle back to this after I have my imports PR up. (I needed to make this change to make an import test work properly, and this was the expedient change.) I could either modify this PR, or open a new PR. In either case basing the import PR off this hopefully keeps it easier to review.

[dhess]

Here's what I did in the primer-rel8 test harness. (As noted in the comment, I argue that this should be moved into Primer.App, as there is a hardcoded implementation of it there already that could be subsumed by this one.)

primer/primer-rel8/test/TestUtils.hs

Line 338 in d34a3db

mkTestDefs :: [ASTDef] -> Map Name PrimFun -> (Map ID Def, ID)

You give it a list of user-defined definitions, and a map of primitive functions, and it gives back a properly-constructed progDefs, plus the next valid ID.

[brprice]

One thing to bear in mind here is that we plan to add namespacing to modules: we should scope IDs/names to a module, and then this clobbering will not be a problem any more. See https://github.com/hackworthltd/primer/pull/293/files#r821588210. Thus it may be more efficient to address this issue properly at that time.

[georgefst]

I don't really mind whether we merge this now or wait for a proper fix via the module system.

[dhess]

I'm OK either way, as well.

[brprice]

I'll open an issue to circle back after finishing modules
EDIT: see #296

[brprice]

I rebased onto brprice/rename-prim-clash, which itself has been rebased onto main. @dhess: once CI passes, could you merge? Thanks