Proposal for Init

[emilypi]

Current internal structure

The current init script state machine operates thusly:

global flags + repo ctx
          | 
          | 
          v
     extend flags - - - - - - [Extend all the things]*
          | 
          | 
          v
write license if not None
          | 
          | 
          v
    write changelog
          | 
          | 
          v
     create dirs
          | 
          | 
          v
    create main.hs  
          | 
          |
          v
 create tests if eligible
          | 
          | 
          v
     write .cabal
          | 
          | 
          v
  generate warnings 
   	       



* Extend all the things

Is simple? -----> shortcircuit all the things implicitly
    | 
    | 
    v
Lib or Exec?
    | 
    | 
    v
get cabal version
    | 
    | 
    v
get package name
    | 
    | 
    v
get version
    | 
    | 
    v
get license
    |
    | 
    v
get author info
    | 
    | 
    v 
get homepage
    | 
    | 
    v
get synopsis
    | 
    | 
    v
get category
    | 
    | 
    v
get extra src files
    | 
    | 
    v
               | library - no app dir
get app dir ---| executable - get app dir
    |          | lib and exe - get app dir
    | 
    v
               | library - get src dir
get src dir ---| executable - no src dir
    |          | lib and exe - get src dir
    | 
    v
gen tests? <--- silently swallow prompt if exe, only gens for libs or lib+exe.
    | 
    | 
    v
get test dir <-- use global test dir or silently swallow if test suites not a yes
    | 
    | 
    v
 get language
    | 
    | 
    v
 get gen comments
    | 
    | 
    v
get modules build tools and deps <- mangle targets if lib + exe or exe or lib with or without testing. 

While the existing interface serves its purpose and fills our needs modulo bugs, the state machine that generates it is a mess in my opinion. Perhaps for historical reasons, it seems to be a consequence of new features being added, but code not being reworked to focus on maintainability, and as a result, the machine bleeds stateful concerns all over the place. It relies on invariants defined in previous steps to be semantically relevant. For example, when retrieving the test dir, the following code is called:

getTestDir :: InitFlags -> IO InitFlags
getTestDir flags = do
  dirs <- return (testDirs flags)
              -- Only need testDirs when test suite generation is enabled.
          ?>> if not (eligibleForTestSuite flags) then return (Just []) else return Nothing
          ?>> fmap (fmap ((:[]) . either id id)) (maybePrompt
                   flags
                   (promptList "Test directory" ["test"] (Just "test") id True))

  return $ flags { testDirs = dirs }

Q: Can you spot the bug that breaks the cascade of invariants?

A: The bugs are threefold, with one bug implied by a previous function:

1. In the previously called function getGenTests, which modifies the state initializing test suites, the code is written such that only Libraries can have test suites, unless they're defined to be initialized in the global config. If not globally defined, a call is then made to check if the package type is not an exe. The test suites are then initialized in the flags. Effectively, Executables can say yes to having test suites, but only in the global config, and this fact is brought in without warning.

2. In getTestDir, if globally defined test dirs are in scope, then these are chosen first regardless of eligibility of the package type. This in itself is a bug if the package type is executable, since they're not supposed to have tests.

3. If the test dirs are not globally defined, then eligibility is checked against the package type and whether test suites are initialized. The program skips test dirs if it's an executable, or if test suites are not initialized. This is a bug, because I can have an executable with globally defined test suite initialization, but no test dirs. Even if test suites are globally defined for an executable, if there is no globally defined test dir, then it is impossible for executables to choose a test dir, which they shouldn't be concerned about in the first place! Effectively, initalizeTestSuite is silently moot, and no warning is issued to that effect, but the code still exists, and the InitFlags are incorrect as a result. They just happen to be benevolently incorrect in the sense that the bug doesn't affect anything.

What we're seeing here is one example of perhaps 5 or 6 examples in the state machine (see #7262 for a recently fixed example). The main source of these issues is that we are treating InitFlags as something to be modified one at a time as one big monolith. This is a problematic design precisely because the relevant InitFlags are a function of the package type. Executable's have separate relevant subsets of the global flags than Library's.

Fix Proposal

I propose the following to fix this:

1. Index InitFlags as an ADT, where Lib, Exe and LibAndExe have separate flag sets available for modification.
2. Have different state machines for each package type. I.e. in the script, we must case on package type and dispatch on the correct state machine.
3. Let getLibOrExec occur before getSimpleProject, and let getSimpleProject be split up into reasonable default projects for the three package type as part of No. 2 (this is a general improvement imo).

I believe this will be more maintainable in the long run. If more package types (or combinations) are added in the future, then the stateful flow of the machines will be separate and more legible to contributors.

This all needs a haircut anyway, so I hope you accept this proposal. I would be ecstatic if I was allowed to do this. The use of (?>>) seems to me like a big smell that too many concerns are being smushed together in the same bits of code.

[emilypi]

@m-renaud @fgaz ^

[ptkato]

Closing this, #7344 has been merged.