Merge varDec and varDecDef in GOOL?

[B-rando1]

@samm82 mentioned in #3854 that because varDec and varDecDef are often implemented similarly, we might consider merging them in the DeclStatement typeclass. The idea is that the merged function would take Maybe SValue as its second input, and act like one of the two current functions depending on whether the Maybe SValue is Nothing. I can see some pros and cons for it, so I'd like to lay them out so we can discuss them.

• The external interface of GOOL. I think that if a merged function appeared in actual GOOL code we write, it would make the code harder to read and write. The extra parameter would just be confusing in my opinion. Fortunately this is a nonissue, as we can use smart constructors to keep the external interface the same, even if we change how the typeclasses are organized.
• Julia and Python. For these, varDec only affects GOOL's internal state, and varDecDef does the same thing, but adds an extra line of code to assign the value to the variable. In the PR linked above, both functions are implemented using the same helper function, which definitely shows the advantages of merging the functions.
• Java, C#, C++, and Swift. The first 3 of these use C.varDec, and all 4 use C.varDecDef. They show the advantages of keeping them separate, as C.varDecDef calls the varDec from the typeclass as part of its implementation. This is what allows Swift to use its own implementation for varDec, but then also use C.varDecDef.
  • With that said, we could do something similar if we were to merge them. The difference would be that instead of varDec being a part of the instance that C.varDecDef can just call, it would need to be a separate function that is passed explicitly to C.varDecDef. It's slightly less elegant, but it achieves the same thing.

What are your thoughts? I don't really mind how it is now, but it might remove a small amount of redundant code if we were to merge the functions.

[samm82]

Thanks for this post and for going more in-depth into the consequences of such a change! Solely based on the impacts this seems to have on other languages, I definitely think it makes more sense to only "merge" them on a case-by-case basis, as you did in 050ba02 and not rush in to a bigger design change that might just get in the way later on!

[B-rando1]

Thanks for your reply @samm82! Do you think I should wait to see what others have to say, or are we good to close it now?

[smiths]

From my point of view, it looks like the two of you have sorted this out and you could close the issue to focus on other things. Having said, I'm not an expert. 😄 Why don't you leave it open for a bit. If nobody has weighed in after a couple of weeks, you can safely close the discussion.

[JacquesCarette]

From a pure code point of view, I can definitely see why one might want to merge these.

However, at a deeper meaning level, we really shouldn't. The point is that, for some languages, varDec should never be used, because all variables should be defined at the same time as they are declared. So for those languages, varDec should throw an error!

So, in fact, we should be moving towards deprecating the use of varDec in most of our examples, as it will actually prevent us from generating code in some languages when it is used.

[JacquesCarette]

For this specific example: if Julia has if expressions ( which it does ) then it should be

var endIdx::Int = z > 0 ? myOtherList.cout : -1

Otherwise, another way to translate the above would be

var endIdx::Int = -1
if z > 0
    endIdx = myOtherList.count
end

[B-rando1]

That's a fair point. While there may be edge-cases like that, they are few, and we can always use a default value if there's no other option.

With that said, I'm not sure if all situations fall nicely into either category. For example, something like this, where the variables need to be defined based on the user's OS:

if OS.type == "windows":
  screen_size = ...
  network_config = ...
  ...
elif OS.type == "MacOS":
  ...
...

We don't want to use the ternary operator for each one, since we just need to check the OS type once to set all of them. But having them default to the Windows options, and only changing them to the MacOS, etc. options after that seems slightly wrong to me. For one thing, it removes the symmetry between OS options. It also makes it possible for the programmer to add a new variable that needs to be configured, but forget to add the MacOS value to the body of the if-statement.

Maybe this isn't something we need to be worried about in the context of Drasil, but I just want to make sure we won't regret removing a feature.

Either way, I can look through drasil-code and see how often we actually use varDec in practice. Like I said, the cases where I would want to use it are few.

[JacquesCarette]

Getting to see where it is used would be good.

Your example is good: we would indeed want to have the code usually looks like that. But: how would you write such code in a language that requires variables to have values upon declaration?

Note that your case brings to mind a missing feature: we want some kind of multi-assign generalization of a case statement (which could be implemented exactly as above). I really mean "piecewise function with known dispatch, that gives multiple results".

[B-rando1]

I looked around drasil-code to see where varDec is used, and how much work it would be to get rid of it. Here's some of the things I found:

Creating a file handle object before opening a file

Here's an example in Java:

Drasil/code/stable/projectile/projectile_u_p_nol_u_wi_v_d/src/java/Projectile/Projectile.java

Lines 160 to 161 in f3b3ffe

	PrintWriter outputfile;
	outputfile = new PrintWriter(new FileWriter(new File("output.txt"), false));

The issue here is that assigning the file handle to the variable is taken care of by the openFileX method (where X can be A, R, or W) from InterfaceCommon. This means that if we want the option of declaring the variable as we assign it to the newly opened file, we'd need new copies of these three functions where they also declare it.

• This makes me wonder if we could merge the three (soon to be 6?) functions into 1 function that takes extra parameters to decide the specifics of its behaviour.

Declaring variables to hold the output of a multi-return function

Here's an example in Java:

Drasil/code/stable/projectile/projectile_u_p_nol_u_wi_v_d/src/java/Projectile/Projectile.java

Lines 25 to 31 in f3b3ffe

	double v_launch;
	double theta;
	double p_target;
	Object[] outputs = get_input(filename);
	v_launch = (double)(outputs[0]);
	theta = (double)(outputs[1]);
	p_target = (double)(outputs[2]);

An another in C++, to show the differences:

Drasil/code/stable/projectile/projectile_u_p_nol_u_wi_v_d/src/cpp/Projectile.cpp

Lines 31 to 34 in f3b3ffe

	double v_launch;
	double theta;
	double p_target;
	get_input(filename, v_launch, theta, p_target);

There's actually two problems with this, both resulting from inOutFunc from InterfaceCommon:

• As with openFileX, it takes care of assigning the variables. This means that if we want the option to declare the variables as we assign them, we'd need to either make a new copy of the function or give it an extra parameter.
• In both the C++ and C# renderers, inOutFunc doesn't assign the variables at all - it just passes them to the function, which mutates them. This means that we need to declare them ahead of time, with no obvious initial value.

Declaring variables to be used later in functions

Here's an example:

Drasil/code/stable/dblpend/src/java/DblPend/Calculations.java

Lines 23 to 31 in f3b3ffe

	ArrayList<Double> theta;
	ODEStepHandler stepHandler = new ODEStepHandler();
	ODE ode = new ODE(m_1, m_2, L_1, L_2);
	double[] curr_vals = {1.3463968515384828, 0.0, 2.356194490192345, 0.0};
	FirstOrderIntegrator it = new DormandPrince54Integrator(1.0e-3, 1.0e-3, 1.0e-6, 1.0e-6);
	it.addStepHandler(stepHandler);
	it.integrate(ode, 0.0, curr_vals, 20.0, curr_vals);
	theta = stepHandler.theta;

theta is declared above because it makes the subsequent lines much more straightforward to translate. I believe this is the line where the corresponding GOOL code is produced. Basically, getting the values for stepStmts probably wouldn't be as easy, as you might have to keep track of which variables have been declared. This might not be an issue if we can safely assume that all variables being assigned in functions like this are immutable and therefore only assigned once.

convStmt

There's one constructor for FuncStmt that seems to only translate to varDec. From a quick search it doesn't look like it's actually used anywhere though. Here's the code anyway:

Drasil/code/drasil-code/lib/Language/Drasil/Code/Imperative/Import.hs

Lines 593 to 598 in f3b3ffe

	convStmt (FDecDef v (Matrix [[]])) = do
	vari <- mkVar v local -- TODO: get scope from state
	let convDec (C.List _) = listDec 0 vari
	convDec (C.Array _) = arrayDec 0 vari
	convDec _ = varDec vari
	fmap convDec (codeType v)

[JacquesCarette]

Excellent investigation. This definitely convinces me that making sure that all variables are initialized on creation is definitely not something that can be done in the generated code. So in sufficiently low-level parts of GOOL, we'll need to be able to separate declaration from initialization.

Having said that, I'd still want to make sure that most of our higher-level idioms make sure that all declared variables will get values before they are read!

[B-rando1]

From this discussion, are there any actionable changes we can make?