(prepep) Preventing future issues with warnings as errors

Ideas
41

I don’t think simply making Syntax warnings filterable fully covers my use case if the direction is to warn about more things that are intended to remain in the language and which may be valid.

Part of the reason I use warnings in CI is to catch issues with dependencies. We have the resources at work to work with upstream libraries to help fix the things we rely on, but not to the extent of even caring about code paths we don’t use within those dependencies.

Syntax deprecations would matter, because if in the future the library won’t even import due to a syntax error, that’s within what we care about, but syntax warnings for valid syntax when a file is compiled are not really.

This is an awkward limitation even with filtering because of how and when these are emitted.

It may be worth changing syntax warnings to be changed so that the ast construction inserts a warning that is emitted when the line of code in question is reached at runtime, making it an actual runtime warning associated with a module and line of code, and having a separate setting to control whether warnings are also emitted for syntax warnings at compile time.

I don’t think syntax deprecation warnings should have the same treatment, because syntax deprecation indicates that in the future, it may not even be possible to compile the file.

The other option that I’ve given significant thought to at this point is to actively discourage use of warnings as errors in workflows that need stability, but provide a structured, documented, and stable output for those who want to do more with warnings. I’d be willing to spend time designing, contributing, and even maintaining this. This frees up the ability to stop worrying about some of the reasons warnings are disruptive, but I’m not sure if it helps with all of them. @pf_moore 's case of not wanting any unexpected output from an application is one I wasn’t previously aware of.

If it does adequately make adding warnings less disruptive for existing users, then this option gives more latitude to start warning for more things that are tricky edge cases within the language that have a reason to exist, but that we also want to ensure people are only using with intention and understanding.

2 Likes
42

To be clear, I don’t have a significant problem here. It’s something that has only happened on rare occasions, and has generally been easy to work around. My main reason for bringing it up was that I was surprised to find that there were people who cared enough about warnings to test with -Werror, but didn’t care enough to mind if their end users saw warnings when running in production. As you say, understanding each other’s use cases is a big part of the problem here.

FWIW, the specific case I can recall that affected me was with the deprecation of methods in the imp module. These were runtime warnings, not syntax warnings. Pip doesn’t test with -Werror, but parts of the Python test suite that exercise pip via the ensurepip module do, so we found that we were blocked on upgrading the bundled pip until we found a solution.

From what I recall (it’s been a while!), we simply used the warnings module to suppress the warning. We could do this relatively safely because we were just waiting for support windows to match up - we needed to drop support for an older version of Python that didn’t have the “new approach” available, but Python introduced the deprecation before we did so (we support Python versions that the core team no longer supports).

If we’d been hit with something like PEP 765, we would have simply fixed the code[1]. That could have been a pain if the fix was in a dependency, but as we vendor all of our dependencies, we could manage.

  1. In theory, if the fixed code needed new Python features that weren’t available in a version of Python that we still supported, we might have been stuck - but I think that’s unlikely enough that we should assume it won’t happen ↩︎

2 Likes
43

No specific quote, but I agree with the folks pointing out flaws with the dedicated compile time warning filter idea. I’m happy to have considered it, but even if it did help in some situations, those (arguable) benefits wouldn’t be worth the complexity it added.

The unexpected output concern reminded me of the situation with the attempted runtime warnings when locale coercion (from C to C.UTF-8) triggers. We had to take those out during the beta period because of situations where “no output permitted on stderr” commands were failing because of the new warning, even though the locale coercion itself was working entirely as intended.

Along the lines of what @mikeshardmind suggested above, perhaps we could define a filter mechanism that allowed warnings to be directed into the logging system rather than being printed directly to stderr?

For example, maybe default>debug uses the default rules for deciding when to emit a warning, but passes it to logging.debug instead of printing it directly to stderr when the warning is triggered. -Wdefault>error would then be a gentler alternative to -Werror that mapped runtime warnings to logged errors instead of throwing (presumably unhandled) exceptions.

The part before the > would be an arbitrary filter rule, so applications could log warnings from their dependencies at a lower level than warnings from their own code.

We’d need to decide whether such messages were sent to the root logger, to a named warnings logger, or whether the destination logger is configurable, but that doesn’t strike me as an impossible task.

4 Likes
44

I think we need to be careful to separate distinct issues here.

Let’s start by pointing out that I don’t think anyone is saying that there’s a problem with the existing runtime warning system. It can be improved, certainly, and there’s definitely value in exploring that, but it’s not the problem that prompted this discussion.

The problem arises with syntax warnings. These happen (if I understand the mechanism correctly) before the main program starts being executed, so there is no way for application code to influence how they are handled. If that’s not true, then could someone explain what I’m missing?

For people who are testing with -Werror, that’s not so much of an issue. Adding extra flags to the command that starts the test run isn’t an impossible burden. Command lines might get a bit long with invocations like “make warnings into errors, but ignore the syntax warning about return in finally in the folowing 3 files” is a little messy to express when we don’t have warning IDs to work with. But that’s not insurmountable.

This should (as I understand it) cover what @Liz needs. In that use case, it’s perfectly OK if code at runtime produces output from syntax warnings - that is simply unexpected output (that users will just have to ignore), and not an actual problem.

But for use cases where the output that users see at runtime is important, we can’t rely on interpreter flags, because we can’t guarantee that users will supply the necessary flags. The fact that syntax warnings aren’t triggered by precompiled bytecode mitigates this problem, but it doesn’t entirely remove it - and in some ways, it makes it more annoying, as the problem becomes an intermittent bug, which goes away when you try to recreate it.

The only way of fully removing syntax warning output from production, at the moment, is to treat the warnings as errors, and change the code to “fix” them. That’s clearly not the semantics of a warning - it’s far more heavy handed than it should be.

The problem with this is that it requires the end user to supply additional interpreter flags to activate that mechanism. And even if we ignore that flaw, I don’t see how the syntax warning mechanism can direct output to the logging system, as that system hasn’t been initialised yet, has it?

The issue is that “no output permitted on stderr” is a condition that doesn’t require -Werror to check. So if a project enforces that constraint, it is almost certainly expecting to do so purely in runtime code. The warnings module allows runtime warnings to be managed in that way, but syntax warnings bypass that because they happen before the main application code has started running.

To me, this is the crux of the problem here. Syntax warnings (at least currently) have to be treated as a solution of last resort, and only for the most serious issues, because there’s no way to control them at runtime, meaning that application developers cannot ignore them short of building their own custom launcher (the standard “entry point” launchers don’t allow specifying interpreter flags). If we want to avoid future issues, we either need a runtime way of controlling syntax warnings, or we need the core devs to be far more reluctant to use syntax warnings than they currently are.

Or, of course, we can take the view that we add interpreter flags to cover the -Werror use case, and reject the “no unexpected output on stderr” use case as too uncommon to worry about[1].

There is another use case, which is essentially the one pip actually encountered, which is that you are being used as a library by someone else, and that other party uses -Werror, but is not willing to special case your code to suppress the syntax warning (or maybe can’t for some reason). In that case, you are left with no choice other than to fix the warning[2]. Once again, we have a syntax warning being escalated to the status of an error. This one, I could imagine us refusing to fix, as it’s the project using -Werror and pushing the problem back to their libraries that’s at fault. But ironically, in pip’s case it was the core CPython test suite that uses -Werror - and it does so in a way that wouldn’t have been easy to modify to include a pip-specific exception. So there’s a real use case where the consumer isn’t as easy to blame :slightly_smiling_face:

  1. with the advice to “precompile your bytecode” as a mitigation ↩︎

  2. or tell your user to get lost :slightly_smiling_face: ↩︎

4 Likes
45

@pf_moore If we’re considering the “how and where syntax warnings are emitted” to be the actual root cause of most of the issues, and I do think that is, then I think we only have 2 solutions that actually cover even the use cases we know about, at least ones that actually support those use cases, it is still an option to end up at “we can’t support this”

On the social side, we could change 765 to not be a SyntaxWarning, and going forward, only use Syntax Warnings for things that are always incorrect, accepting that they are too heavy-handed for anything else.

On the technical side, we could change syntax warnings to be something inserted into ast during compile, and not triggered until being reached at runtime.

The latter is what I was trying to get at with the below, but I think you laid out the reasons something like it would be needed in a more direct way.

4 Likes
46

Ah, thank you. Yes, that is a way to make syntax warnings participate in the standard runtime warning mechanisms. I like it as an option, as it avoids all the special cases. But it might be considered by some as too intrusive, in which case we’re left with social solutions.

2 Likes
47

Here’s a code block with all the syntax warnings (slightly edited):

So, what’s literally always incorrect here?

  • Literal divided by zero (currently not a warning):
    1 / 0
    Equivalent to:
    raise ZeroDivisionError("division by zero")
  • Return that’s overwritten in a finally block (warning in a different place):
    try:
    return 1 # <- warn here
finally:
    return 2 # <- not here
    Equivalent to:
    try:
    pass
finally:
    return 2
  • Assert with parentheses:
    assert (x, "msg")
    Equivalent to:
    pass
  • Invalid operations on displays:
    {}()
{1}[1]
"a"["b"]
    Equivalent to:
    raise TypeError("'dict' object is not callable")
raise TypeError("'set' object is not subscriptable")
raise TypeError("string indices must be integers, not 'str'")

What should display a warning anyway?

  • Ambiguous syntax:
    x = [y + 0x1for y in [1]]
    Equivalent to:
    x = [y + 0x1f or y in [1]]
  • Strict equality (only works with small numbers):
    x = y is 1
    Equivalent to:
    x = y == 1 and isinstance(y, int)
  • Single backslashes (only works with invalid escape sequences):
    x = "\99"
x = f"\{1}"
    Equivalent to:
    x = "\\99"
x = f"\\{1}"

And what’s more controversial:

  • Supressing exceptions in finally:
    try:
    input() # Ctrl+C
finally:
    return 1
    Equivalent to:
    try:
    input() # Ctrl+C
except:
    pass
return 1
48

This one’s not necessarily incorrect. This could be to early terminate some process in the try block without also having a typechecker yell about the return type. It might be dubious, but it has a well defined meaning that could be intentional.

This one is always incorrect, because it’s only well defined meaning relies on an implementation detail subject to change. That it ever has the behavior a user expects is solely a reflection of an internal implementation detail.

49

How? Please give an example. (Note that return is unconditional here)

50

It’s unconditional in that specific minimal example, but you claimed the fact that there was even a return in the try block when it would be overriden is always an error.

It does not require an example to say that has an exact well-defined meaning, but please go read through the discussion thread of 765 if you believe that this is always erroneous, this isn’t new ground and not helpful to say that’s always an error still.

51

Rather than send you into that long thread, I have an example from code I changed as a result of 765 here which I shared before: Avoid returning in finally · Rapptz/discord.py@d08fd59 · GitHub

Note that the only base exceptions that this could reasonably swallow in theory can’t actually be swallowed here (MemoryError can, but that has other consequences that prevent it from actually being swallowed) , as there is signal handling for an asyncio event loop in play, so it wasn’t erroneously swallowing exceptions, and was early exiting via return. There’s even a type ignore on it cause a type checker didnt understand the return never mattered, and the code predated typechecking the code here.

52

sorry, I just realized this wasnt even one catching base exception, so that bit of the anlysis doesnt even matter for this one of the changes. is a little more obscured

53

This is really drifting too far into retread territory. Whether or not anyone agrees with writing code that way, the SC said there wasn’t an intent to remove it, and code written that way matches the explicit intent of how the syntax was designed for the language (Also previously confirmed), so it falls under warning for something that can be correct.

2 Likes
54

(Edit: I started writing this before @Nineteendo reposted the catalog of warnings from the previous thread, but was interrupted, and then didn’t realise this thread had moved on when I came back and finished it)

It isn’t that they happen before __main__ starts running, but rather that, if they happen at all, then they happen when modules are imported. With eager imports, that’s often during test discovery rather than during test execution. They also end up being reported for all code in dependencies, not just the code that is actually used by the running application (or the code under test).

On that front, I realised that we should explicitly note all the syntax warnings that are currently emitted (just by searching the CPython code base for SyntaxWarning):

  • unknown backslash escape sequences
>>> "\g"
<python-input-5>:1: SyntaxWarning: invalid escape sequence '\g'
'\\g'
  • invalid octal escape sequences
>>> "\400"
<python-input-17>:1: SyntaxWarning: invalid octal escape sequence '\400'
'Ā'
  • subscripting a type known not to support subscript lookup
>>> lambda: 1[2]
<python-input-23>:1: SyntaxWarning: 'int' object is not subscriptable; perhaps you missed a comma?
<function <lambda> at 0x705dfb04b920>
  • subscripting a known container type with an invalid literal type
>>> lambda: [][""]
<python-input-3>:1: SyntaxWarning: list indices must be integers or slices, not str; perhaps you missed a comma?
<function <lambda> at 0x705dfb04bb00>
  • using is (or is not) to compare against a non-singleton literal:
>>> int is 1
<python-input-6>:1: SyntaxWarning: "is" with 'int' literal. Did you mean "=="?
False
  • assertions with non-empty tuples (e.g. attempting to use parentheses to span multiple lines)
>>> assert (False, "message")
<python-input-8>:1: SyntaxWarning: assertion is always true, perhaps remove parentheses?
  • attempting to call things that definitely aren’t callable
>>> lambda: []()
<python-input-9>:1: SyntaxWarning: 'list' object is not callable; perhaps you missed a comma?
<function <lambda> at 0x705dfb04bce0>
  • running keywords up against a preceding numeric literal
>>> 1and 2
<python-input-21>:1: SyntaxWarning: invalid decimal literal
2
  • the new control flow in finally warning added by PEP 765

Tangent: I found the “perhaps you missed a comma?” hints to be a bit cryptic, so I filed an issue to suggest amending the hint wording to be more explicit.

If we were to make emitting syntax warnings a new runtime opcode instead of emitting them eagerly at compile time, I think that would solve a bunch of problems with them:

  • they’d always be emitted when the code ran, regardless of whether the code was precompiled or not
  • in test cases, they’d be emitted at test execution time, not test discovery time
  • it would fix the current problems with them not having correct location information without needing major changes to pass module info to the various compilation APIs
  • in dependencies, we’d only get syntax warnings for the code we actually run rather than implicitly scanning every line of code
  • the various warning actions like always would work properly with them

A potential technical difficulty with the idea is that not all syntax warnings are emitted in the opcode generation step: some warnings related to invalid escape sequences in f-strings and t-strings are emitted in the tokenizer, while the one for missing whitespace between numeric literals and a trailing keyword is emitted by the lexer. (While the PEP 765 warnings are currently emitted in the AST parsing phase, that caused unanticipated problems, so they’re going to move to the opcode generation step with most of the others)

(cc @storchaka since this is a potentially different approach to solving the warning location problem)

2 Likes
55

I believe I read the entire discussion… But yeah you’re right, an unconditional return wouldn’t always be incorrect (I believe Guido said something like that). We could still raise a warning for the unconditional case though.

try:
    return 1
finally:
    if x:
        return 2

My point here was simply that there are other warnings that do what the author intended.

56

More equivalent to:

x = y == 1 and type(y) is int

[1] though this still ignores the part where the identity check is interpreter-dependent, so this is non-portable code that could break at any time. IMO the warning is valid on the basis that this code is fundamentally fragile.

Similarly, the invalid backslash example is fragile in that a future version of Python might give a new meaning to that. Unlikely with the examples you gave, but "\e" has a good chance of changing in meaning from "\\e" to "\x1b" in the future.

  1. notably, True should not pass this check ↩︎

57

The invalid backslash warnings are emitted for different reason. They are emitten because there arfe valid backslash escapes. If you have path "C:\new" hardcoded in your code, it will not work as you expected, and you can have a hard time to figure what is wrong if you are not exerienced Python programmer. But if you get a warning for "C:\Windows" and "C:\Program Files", you learned to use raw strings for Windows paths and do it automatically. There is also a good chance that valid and invalid escape sequences occurred in one string, so by fixing a warning you will fix a bug.

There are similar errors in docstrings and regular expressions.

So the purpose of thes warnings is education and reminding.

Other warnings make only sense when they are emitted at the compile time: [][] and ()(). This is already a TypeError, burt warnings provide better context in case when you forgot a comma in a long multiline sequence of lists or tuples:

a = [
    [1, 2, 3],
    [4, 5, 6]
    [7, 8, 9]
]

In some cases a warning can be emitted in the code that will never be executed (rare case or eliminated by the optimizer).

Emitting a warning at runtime will annoy end users, this will lead to silencing the warnings by default (as happened with deprecation warning), this will lead to ignoring warnings.

2 Likes
58

That seems like a reasonable view to me. So perhaps a hybrid of the status quo and runtime syntax warnings as @Liz originally suggested would make sense?

  • Define a new RuntimeSyntaxWarning subclass of SyntaxWarning
  • Emit those via opcode injection rather than eagerly
  • Switch the PEP 765 warnings over to that mechanism
  • Leave the other existing syntax warnings alone (aside from potentially fixing their reported warning locations)

Any future syntax warnings added would then be classified as either compile time warnings or runtime warnings and emitted accordingly.

59

compile time is the right tme.

Just make this work:

import warnings 
warnings.filterwarnings("ignore", message="'return' in a 'finally' block", category=SyntaxWarning)

def main():
    try:
        return 4
    finally:
        return 3

if __name__ == "__main__":
    print(main())
60

That can’t work with compile time syntax warnings, as the entire module is compiled as a single unit, and hence cannot alter the warning filters that apply to its own compilation.

It would work with the RuntimeSyntaxWarning proposal, as the warnings filter will have been adjusted by the time main() is executed (which is another piece of evidence that this might be the right direction to go).

1 Like