Eliminate Duplicate String "get_input"

[peter-michalski]

Details:

"get_input" Number of instances of string: 12 Number of files containing the string: 6 ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Descriptions.hs:"get_input" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/DrasilState.hs:"get_input" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/FunctionCalls.hs:"get_input" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Generator.hs:"get_input" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Modules.hs:"get_input" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Parameters.hs:"get_input"

[peter-michalski]

Proposing similar solution as in #2844. All such strings will be located in the same /code file.

[JacquesCarette]

And my reaction is the same. This particular one is even trickier, as I think that "get_input" actually ends up appearing in the generated code, so it is a rather important name associated to a concept, so we need to better understand what that concept is.

[peter-michalski]

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Descriptions.hs:

-- | Returns a description of the input constructor, checking whether each -- possible method that may be called by the constructor is defined, and -- including it in the description if so. inputConstructorDesc :: GenState Description

-- | Returns a description of what is contained in the Input Format module, -- if it exists.

inputFormatDesc :: GenState Description
inputFormatDesc = do
  g <- get
  let ifDesc False = ""
      ifDesc _ = "the function for reading inputs"
  return $ ifDesc $ "get_input" `elem` defList g

elem returns True if the list contains an item equal to the first argument

-- | Returns a description for the generated function that reads input from a file, -- if it exists.

inFmtFuncDesc :: GenState Description
inFmtFuncDesc = do
  g <- get
  let ifDesc False = ""
      ifDesc _ = "Reads input from a file with the given file name"
  return $ ifDesc $ "get_input" `elem` defList g

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/DrasilState.hs:

-- | Get input format to be exported (for @get_input@). -- See 'getExpDerived' for full logic details.

getExpInputFormat :: Name -> Choices -> [Input] -> [ModExp]
getExpInputFormat _ _ [] = []
getExpInputFormat n chs _ = fMod (modularity chs) (inputStructure chs)
  where fMod (Modular Separated) _ = [(giNm, "InputFormat")]
        fMod _ Bundled = []
        fMod Unmodular _ = [(giNm, n)]
        fMod (Modular Combined) _ = [(giNm, "InputParameters")]
        giNm = "get_input"

modular input?? not sure what's happening here

-- | Get input format defined in a class (for @get_input@). -- See 'getDerivedCls' for full logic details.

getInputFormatCls :: Choices -> [Input] -> [ClassDef]
getInputFormatCls _ [] = []
getInputFormatCls chs _ = ifCls (inputModule chs) (inputStructure chs)
  where ifCls Combined Bundled = [("get_input", "InputParameters")]
        ifCls _ _ = []

I don't see any get_input class in the project files, but I do see methods in several languages in nopcm, glassbr, pdcontroller, projectile. It is a method of InputParameters in Control.py

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/FunctionCalls.hs:

-- | Generates a call to the function for reading inputs from a file.

getInputCall :: (OOProg r) => GenState (Maybe (MSStatement r))
getInputCall = getInOutCall "get_input" getInputFormatIns getInputFormatOuts

Looks like a method here

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Generator.hs:

-- | Generates an entire SCS program as a single module.

genUnmodular :: (OOProg r) => GenState (SFile r)
genUnmodular = do
  g <- get
  umDesc <- unmodularDesc
  let n = pName $ codeSpec g
      cls = any (`member` clsMap g) 
        ["get_input", "derived_values", "input_constraints"]

Not sure what is meant by entire SCS program... Not clear what is happening in the last lines here. cls = any (member clsMap g) ? Some sort of mapping happening with the three strings.

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Modules.hs:

-- | Generates a constructor for the input class, where the constructor calls the -- input-related functions. Returns 'Nothing' if no input-related functions are -- generated.

genInputConstructor :: (OOProg r) => GenState (Maybe (SMethod r))
genInputConstructor = do
  g <- get
  let dl = defList g
      genCtor False = return Nothing
      genCtor True = do 
        cdesc <- inputConstructorDesc
        cparams <- getInConstructorParams    
        ics <- getAllInputCalls
        ctor <- genConstructor "InputParameters" cdesc (map pcAuto cparams)
          [block ics]
        return $ Just ctor
  genCtor $ any (`elem` dl) ["get_input", "derived_values", 
    "input_constraints"]

Looks like "get_input", "derived_values", "input_constraints" are the functions.

-- | | Generates a function for reading inputs from a file.

genInputFormat :: (OOProg r) => ScopeTag -> 
  GenState (Maybe (SMethod r))
genInputFormat s = do
  g <- get
  dd <- genDataDesc
  let getFunc Pub = publicInOutFunc
      getFunc Priv = privateInOutMethod
      genInFormat :: (OOProg r) => Bool -> GenState 
        (Maybe (SMethod r))
      genInFormat False = return Nothing
      genInFormat _ = do
        ins <- getInputFormatIns
        outs <- getInputFormatOuts
        bod <- readData dd
        desc <- inFmtFuncDesc
        mthd <- getFunc s "get_input" desc ins outs bod
        return $ Just mthd
  genInFormat $ "get_input" `elem` defList g

"get_input" is the function being generated?

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Parameters.hs:

-- | The inputs to the function for reading inputs are the input file name, and -- the 'inParams' object if inputs are bundled and input components are separated. -- The latter is needed because we want to populate the object through state -- transitions, not by returning it.

getInputFormatIns :: GenState [CodeVarChunk]
getInputFormatIns = do
  g <- get
  let getIns :: Structure -> InputModule -> [CodeVarChunk]
      getIns Bundled Separated = [quantvar inParams]
      getIns _ _ = []
  getParams "get_input" In $ quantvar inFileName : getIns (inStruct g) (inMod g)

-- | The outputs from the function for reading inputs are the inputs.

getInputFormatOuts :: GenState [CodeVarChunk]
getInputFormatOuts = do
  g <- get
  getParams "get_input" Out $ extInputs $ codeSpec g

These also indicate that it's a function. Looks like "get_input" refers to a function in the non-Haskell project files. Please let me know if I am going in the right direction here.

[JacquesCarette]

To understand drasil-code (and drasil-gool), your best source of information will be Brooks' Master's Thesis, as well as our joint paper. They are both in the repo somewhere (I'm explicitly going to get you to search for them, as a means to have you get more familiar with what's in here)

So you're getting closer to what is really going on here, especially with "get_input" is the function being generated?. You would have gotten another hint through looking in code/stable. Also, many of the comments on the previous issue apply here too.

[smiths]

To save @peter-michalski some time, I'm not 100% sure that Brooks's thesis is on the Drasil site. There should be a link to it somewhere, but the actual thesis is in the se4sc repo. The papers though should be in the Drasil repo. :smile:

[peter-michalski]

@smiths Thank you for the link.

I have reviewed Brooks' thesis. "get_input" and related concepts were mentioned several times.

As previously discussed, InputParameters was confirmed as the module/class containing input variables. Get_input was noted as the function for reading input values from a file. Of particular interest were a few comments, including that "Currently, we have fixed the names of the core SCS modules, classes, and functions that Drasil’s code generator generates" and "These could be easily added to the design language and made user-defined (see Section 10.5), but that was not a high priority."

It appears that a solution to this duplicate string issue would be to go ahead and add a "get_input" concept field to the design language. Brooks mentioned this could be added "to the Choices structure to specify names for the various components of the generated code" and that this could be used instead of the instances of the string currently found in the generator.

[JacquesCarette]

Just for the record: after discussion, yes, we need to add the concept "get_input" to Choices as a configuration parameter.

[peter-michalski]

Following the pattern in Choices.hs, I was looking to begin adding the concept (with fixed name, for now) in the following code, but I have concerns with "get_input" as a Sentence. It appears that "get_input" is [Char] or [String] elsewhere in the code.

-- | Sets component names.
data ComponentName = GetInput      -- ^ Sets input component name.

-- | Renders options for component names.
instance RenderChoices ComponentName where 
  showChs GetInput = S "get_input"

Should I also be making some changes to RenderChoices, adding something like

showChsChar :: a -> [Char]

and then having

-- | Sets component names.
data ComponentName = GetInput      -- ^ Sets input component name.

-- | Renders options for component names.
instance RenderChoices ComponentName where 
  showChsChar GetInput = "get_input"

Please let me know your thoughts.

[JacquesCarette]

Note: String in Haskell is [Char], by definition. Also, I can't remember where everything is (Drasil has gotten large), so it is best to remind me which package defines Choices.hs (now I know it's drasil-code).

Your code above does fit the pattern established in that file. No, you should not be adding something like showChsChar, not unless there is a strong reason for it.

[peter-michalski]

I added the following code into Language.Drasil.Choices (and updated defaultChoices). This alone compiles nicely.

-- | Sets component names.
data ComponentName = GetInput -- ^ Sets input component name.

-- | Renders options for component names.
instance RenderChoices ComponentName where 
  showChs GetInput = S "get_input"  

Now, one of the "get_input" strings that I am trying to eliminate is found in Language.Drasil.Code.Imperative.Parameters . Specifically, it is found as an argument to getParms, One such example is:

getParams "get_input" In $ quantvar inFileName : getIns (inStruct g) (inMod g)

Since getParams is expecting an argument of type [Char], and not of type ComponentName, I'm not sure how to proceed. I'm not confident that I should be abandoning the style found in Choices.hs, specifically the creation of a new type for ComponentName, since creating types is helpful when working with other 'choices', but I'm also not sure if I should be fiddling with getParams, since it's used extensively in the file.

[JacquesCarette]

You are correct that you should not abandon that style.

The first thing to investigate is what getParams actually does with its first argument. It might be that making that a Sentence is not a big deal. If it is a big deal, that will inform us further about the design.

[peter-michalski]

getParams passes its first argument n into the first argument of two other functions getInputVars and getConstVars

  inVs <- getInputVars n pt (inStruct g) Var inpVars
  conVs <- getConstVars n pt (conStruct g) (conRepr g) conVars

getInputVars uses n with Map.lookup:

getInputVars n pt Bundled Var _ = do
  g <- get
  let cname = "InputParameters"
  return [quantvar inParams | Map.lookup n (clsMap g) /= Just cname && isIn pt]

getConstVars passes n to getInputVars as well: getInputVars n pt (inStruct g) cr cs

So n is a key for looking in the map returned by clsMap g (ClassDefinitionMap where cname = "InputParameters"?).

Looking at the type signature of Map.lookup, lookup :: Ord k => k -> Map k a -> Maybe a, the first argument must be of a type that has an ordering. Sentence is not such a type.

I'm also not sure why Map.lookup n (clsMap g) is followed by /= Just cname && isIn pt. The mapping is not equal to any type of cname && the Paramtype is input or output?

[JacquesCarette]

This is going to iterate over the elements of g (whatever those are), which might be all the classes in current scope. Then it finds all the names associated to n what are not "inputParameters" (not sure about the isIn pt).

I'm guessing this has something to do with bundled/unbundled names.

But the main point: the n is used to look things in up a Map. So it needs to have an Ord instance. All in all, it does seem like the representation of the name should be 'renderable', likely to ASCII for things to work.

So it looks like we are going to need multiple classes for rendering, that go to different types. At the very least, to String and to Sentence.

[peter-michalski]

So it looks like we are going to need multiple classes for rendering, that go to different types. At the very least, to String and to Sentence.

This is implemented in #2919.

A future pull request will use the new rendering class, RenderChoicesStr, to render "get_input" in files containing the String.

[peter-michalski]

This is being addressed with #2966.