Avoid discarded (error) values in runtime

[wdanilo]

This task is automatically imported from the old Task Issue Board and it was originally created by Radosław Waśko.
Original issue is here.

---

Whenever a function result is discarded (not assigned to a variable, passed as an argument or returned from the function) but it is different from Nothing, it likely indicates a programming error.

For example, if we have:

# fun is expected to take two arguments and perform some side effects
foo fun x =
    IO.println "hello"
    fun x x
    IO.println "bye"

main =
    foo (a -> b -> c -> IO.println a+b+c) 10
    foo (a -> b -> IO.println a+b) 10

The first foo call within main will only print hello and bye, but the call to fun will not proceed because it expects more arguments. We can detect that a statement that was supposed to be side-effectful returned something else than a Nothing and thus know that likely a mistake has happened. The second foo call within main will indeed print 20 alongside hello and bye.

Of course in some situations it may make sense to discard a non-Nothing result of some action, but if this is intended behaviour, it can always be marked by assigning it to an unused variable name, like so:

_ = foo "bar"

In such case the intent to discard the value is clearly marked. In other cases, discarding values is most likely a programming error and so it may be worth to warn about such cases.

A runtime linter pass could check values of statements which discard the result and if the value is different from Nothing it can report a "runtime warning".

There are a few considerations to such runtime lints though:

1. While a static lint checks each code location once, a runtime check may fire every time some function is called. We definitely do not want thousands of warnings to be fired for the same code location if some specific function is called thousands of times. So the linting mechanism probably will need some way to ensure that the warning is reported only the first time the problem is encountered for a given location and all further warnings for that same location should be suppressed to avoid repetition.
2. It may be worth to completely suppress these checks in 'production' mode. It is important to consider if the checks influence performance in a measurable way.
3. In case of static lints, the code location is usually enough to understand why it fired - because the conditions checked only depend on statically known information present in the code (structure, type signatures). In case of run-time lints, whether the problematic condition is encountered or not may depend on actual values of variables in the context and other dynamic conditions, so just reporting a code location where the problem has happened may not be enough to diagnose it (although in many cases it may already provide enough information). To give a good diagnostic, it may be necessary to provide additional data like values of variables in scope and a stack trace. Alternatively, an ability to set a debugger breakpoint at the moment where the value is discarded could allow the user to interactively analyse the situation that caused the problem.

Comments:

To be reviewed as to whether this is the correct path forward. (James Dunkerley - Feb 3, 2023)

---

[radeusgd]

I've found yet another place where this could save issues.

The cloud Enso_File were not running runnable from their own file because we forgot an additional required argument:

Index: test/Base_Tests/src/Network/Enso_Cloud/Enso_File_Spec.enso
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/Base_Tests/src/Network/Enso_Cloud/Enso_File_Spec.enso b/test/Base_Tests/src/Network/Enso_Cloud/Enso_File_Spec.enso
--- a/test/Base_Tests/src/Network/Enso_Cloud/Enso_File_Spec.enso	(revision 527a9c221a2583352f46aee8264a31b80f3acabb)
+++ b/test/Base_Tests/src/Network/Enso_Cloud/Enso_File_Spec.enso	(date 1708002992169)
@@ -192,6 +192,7 @@
             nested_file.exists . should_be_false
 
 main =
+    setup = Cloud_Tests_Setup.prepare
     suite = Test.build suite_builder->
-        add_specs suite_builder
+        add_specs suite_builder setup
     suite.run_with_filter

If not any value other than Nothing, at least discarding a Function value without an explicit _ = is almost always a mistake, so IMO we should consider adding some lint pass that issues a warning (at runtime) when something like this happens.

[JaroslavTulach]

Runtime linting pass

I have no idea what runtime pass may mean? There are no passes in runtime.

[radeusgd]

Runtime linting pass

I have no idea what runtime pass may mean? There are no passes in runtime.

I imagine it's a metaphor. It would have to be implemented by creating e.g. UnusedVariableNode that is added in places where a call result is not assigned to a variable, e.g.:

foo =
   bar 1 2 3
   r = 2
   r + 3

the result of bar 1 2 3 could be passed to UnusedVariableNode.consume - which would then perform the check if the discarded value was Nothing - so it is ok, or if it was something else and we should log a warning.

[radeusgd]

Runtime linting pass

I have no idea what runtime pass may mean? There are no passes in runtime.

Afterthought: I probably meant a static pass that inserts a runtime check.