Promote warnings to errors?

[aaaaalbert]

(Note: The mechanics of the problem were first discussed under the tangentially related #69.)

There are a few code constructs that can raise Warnings (see Python docs), which can cause trouble in nested VirtualNamespaces (as used by dylink for example).

For example, consider the snippet

assert(False, "A message")

This does not raise an AssertionError, because assert is a keyword (not a function), so the parenthesized expression evaluates as a tuple (rather than as arguments), which is truthy. This generates SyntaxWarning: assertion is always true, perhaps remove parentheses? on stderr as a notification about this problem, but the program will continue to run.

If you run RepyV2 from the command line, you will see the SyntaxWarning. If you run the above snippet on a vessel, you won't see the message. This might or might not be a problem for you.

However, if you run the snippet inside of a VirtualNamespace, or in a program that uses dylink, the SyntaxWarning causes this weird traceback, courtesy of Python's warning library that gets invoked when the nested VirtualNamespace is checked for safety and found to contain the potential syntax problem (see #69):

python repy.py restrictions.default dylink.r2py test-assert.r2py

---
Uncaught exception!

---
Following is a full traceback, and a user traceback.
The user traceback excludes non-user modules. The most recent call is displayed last.

Full debugging traceback:
  "repyV2/repy.py", line 177, in execute_namespace_until_completion
  "/Users/albert/Desktop/seattle/seattle_repy/repyV2/virtual_namespace.py", line 117, in evaluate
  "/Users/albert/Desktop/seattle/seattle_repy/repyV2/safe.py", line 588, in safe_run
  "dylink.r2py", line 546, in <module>
  "dylink.r2py", line 397, in dylink_dispatch
  "dylink.r2py", line 155, in _dy_module_code

User traceback:
  "dylink.r2py", line 546, in <module>
  "dylink.r2py", line 397, in dylink_dispatch
  "dylink.r2py", line 155, in _dy_module_code

Exception (with type 'exceptions.Exception'): Failed to initialize the module (test-assert.r2py)! Got the following exception: 'RunBuiltinException("Unsafe call 'open' with args '('test-assert.r2py', 'rU')', kwargs '{}'",)'

---

We could probably stash the unsafe calls required by warnings in safe.py somehow, as we do with a couple of others (like getattr), and through this enable the behavior of SyntaxWarning (and other warnings) as in the non-dylinked / non-VirtualNamespace case.

I would personally prefer to promote warnings to errors instead, so that proper exceptions are raised for every warning. This requires a simple one-line patch,

--- a/safe.py
+++ b/safe.py
@@ -92,7 +92,7 @@ except ImportError:
 import warnings
 warnings.simplefilter('ignore')
 import compiler     # Required for the code safety check
-warnings.resetwarnings()
+warnings.simplefilter('error')

and does two things: It stops the program, and outputs the warning message in a visible manner, regardless of whether you can see stderr:

Exception (with type 'exceptions.Exception'): Failed to initialize the module (test-assert.r2py)! Got the following exception: 'SyntaxWarning('assertion is always true, perhaps remove parentheses?',)'

[awwad]

On the face of it, it feels like a big deal to decide that all warnings are now exceptions. Why do you suggest this, Albert? (Beyond that it's easy. I'm sure there's plenty I'm not aware of.)

[awwad]

I'll admit that my thinking is probably stuck in the normal world, and I'm not really thinking through the implications of warnings in restricted python. (What would be the primary source of warnings? Syntax things from core python? Deprecations with newer python versions? How often are we even going to allow code base changes? We're not sure if/when python 3 is going to happen, I suppose.... Thinking aloud.)

Options:

1. Your suggestion on its own would implicitly mean that the r2py interface would expect code to have except Warning, w: throughout where warnings might arise, in order to be stable.
2. Sorcery I'll get to below.
3. We could do the work of making the warning library work by providing it what it needs somehow through safe.py, and making sure that's safe. Given that work, we could establish some location to stick warnings - just a log() format, maybe? Vlad pointed out that some languages provide a dictionary. That could work. Or we could establish some default log file when it's not configured...? warn.log or some such?

As for option 2: Since you say that that single line change would raise all warnings as exceptions, and since you imply that that requires no retooling of safe.py, what if we do that and have a wrapper basically except Warning, w for all code, and do Something Appropriate with it (like the last bits of option 3)?

[aaaaalbert]

I'm suggesting this change so as to minimize the surprise that the user encounters when the Repy runtime (as opposed to Repy user code) raises warnings, given the rare but confusing dylink traceback in the assert example above.

Sources of warnings: At the moment, we have one single suppressed warning from the Repy runtime (importing compiler which is deprecated). There are maybe a handful ways of triggering warnings from the Repy runtime if you really want to, or are misled (e.g. this version-dependent format issue, and my assert example above).

Unlike other libraries and projects (numpy, sqlish things), the Repy runtime doesn't use warnings for its own signalling. It's possible and allowed that Repy user code defines its own warnings and uses them, but from a few quick experiments the idiom doesn't seem any different from using exceptions. (There is no interface into the warnings module, so Repy user code cannot choose whether / how warnings will be treated).

Overall, warnings that are raised from the runtime are both seldom, and usually right. Tongue-in-cheekly, what were the 10 last warnings you saw from Repy or the Python standard libs, and over what timespan did you see them? Me, I'm at two overall, over 6 years. More importantly, warnings in our case indicate that user code does something that's very likely unintended anyway.

Replying to your points,

1. You wouldn't engineer your code to expect warnings from the runtime to occur. excepting the SyntaxWarning for the assert example still does not make the assertion do what it is supposed to, just like except NameError doesn't create a missing variable or function by itself.
2. I'm not sure I get the point. Assume the user code raises an ex-warning-now-exception. We cannot return to user code from an outside error handler. The handler could only log the warning. Well, it already does this with unexcepted exceptions.
3. Yeah, stashing etc. would be feasible, but that's a complex solution for a rare problem.

So for the runtime side of things where warnings probably arise inadvertently, I think my patch makes sense. For the user code side where own warnings could be defined and used, I don't think it makes a difference.

[awwad]

You're right on all counts (EDIT: i.e. on your responses to 1, 2, and 3), I think. The catch-all-warnings-raised-as-exceptions thing was a backward thought. 😅

I still don't love it on principle, but if as you say it's that exceptionally rare... OK. (: I'll try to keep it in the back of my mind in case some parallel thing needs to be added to safe.py later.

I'll write a unit test for this.

I had a thought about a full integration/regression test for things like this - say, picking some canonical Seattle experiment and having it run and confirming that output fits... but existing unit tests probably suffice. In any case, before we get to something like that, we should probably fix all the unit tests. (:

[awwad]

Others can compare the non-informative result one would get from a dylink-run test currently (pre-commit), above, with the result when your commit is included, below. It's noisy, but at least it shows the actual warning at the end there.

(v2s) s:RUNNABLE s$ python repy.py restrictions.test dylink.r2py ut_repyv2api_warnings.py
---
Uncaught exception!
---
Following is a full traceback, and a user traceback.
The user traceback excludes non-user modules. The most recent call is displayed last.

Full debugging traceback:
  "repy.py", line 154, in execute_namespace_until_completion
  "/Users/s/w/repy_v2/RUNNABLE/virtual_namespace.py", line 117, in evaluate
  "/Users/s/w/repy_v2/RUNNABLE/safe.py", line 592, in safe_run
  "dylink.r2py", line 546, in <module>
  "dylink.r2py", line 397, in dylink_dispatch
  "dylink.r2py", line 155, in _dy_module_code

User traceback:
  "dylink.r2py", line 546, in <module>
  "dylink.r2py", line 397, in dylink_dispatch
  "dylink.r2py", line 155, in _dy_module_code

Exception (with type 'exceptions.Exception'): Failed to initialize the module (ut_repyv2api_warnings.py)! Got the following exception: 'SyntaxWarning('assertion is always true, perhaps remove parentheses?',)'

[awwad]

Above, you say:

If you run RepyV2 from the command line, you will see the SyntaxWarning. If you run the above snippet on a vessel, you won't see the message. This might or might not be a problem for you.

(Without the fix commit,) I ran such a snippet on a vessel via seash and the warning appears in the log file, just as an error would.

Here's the example, a script named example.r2py:

log('Log before assert statement.\n')
assert(False, "Potatoes")
log('Log after assert statement.\n')

And then I do this in seash:

sawwad@browsegood !> upload example.r2py
sawwad@browsegood !> start example.r2py
sawwad@browsegood !> show log
Log from 'f4e6fd77730f028210a0e5b8e9842ff005a2ccb9:1224:v2':
example.r2py:4: SyntaxWarning: assertion is always true, perhaps remove parentheses?
========================================
Running program: example.r2py
Arguments: []
========================================
Log before assert statement.
Log after assert statement.

What am I missing?

[awwad]

Confirmed with Albert that the warnings actually appear in the log on VMs - so that's not a problem.

The issue when dylink is used, though, remains that warnings are both fatal and incomprehensible without Albert's patch. So we should still go through with it. I'm wrestling with another possible workaround first.