In issue27364, invalid escape sequences in string literals are deprecated. Currently the deprecation message is not so useful when fixing lots of files in one or more large projects. For example, I have two files foo.py and bar.py:

# foo.py
import bar

# bar.py
print('\d')

It gives:
$ python3.6 -W error foo.py
Traceback (most recent call last):
  File "foo.py", line 1, in <module>
    import bar
DeprecationWarning: invalid escape sequence '\d'

My idea is that the warning message can be improved to provide more information. In http://bugs.python.org/issue27364#msg269373 it's proposed to let a linter check such misuses. It's useful within a single project. For a project that depends on lots of external projects, a linter is not enough. Things are worse when __import__, imp or importlib are involved, or sys.path is modified. I have to either add some codes or use a debugger to show which module is imported.

For above reasons, I propose to add at least the filename and the line number to the warning message. For example:

$ ./python -W error foo.py
Traceback (most recent call last):
  File "foo.py", line 1, in <module>
    import bar
  File "/home/yen/Projects/cpython/build/bar.py", line 1
    print('\d')
         ^
SyntaxError: (deprecated usage) invalid escape sequence '\d'

With that I can know which file or project I should blame.

Added some of reviewers from issue27364 to nosy list.

And I'd like to ask Ned: for me it's an improvement of an existing feature, so I guess it can enter the 3.6 branch?

It sounds like a fix but let's see the final patch first.  If a core developer wants to apply it to the default branch for 3.7, we can decide whether it should go into 3.6, too.

The error can't be a SyntaxError, it must be a DeprecationWarning.  If you can improve the deprecation warning text, I'd be in favor of calling that a fix to the original feature and put it in 3.6.  The deprecation warning can even say that this will be a SyntaxError some time in the future.

Hello, and thanks! I'll work on a patch this week, or at most next week. I will make it so that it's completely uncontroversial to apply it to 3.6 as well (won't change the actual feature, only prettify the error message), so no need to worry about that :)

See also Issue 28028. Serhiy suggested translating warnings to SyntaxWarning in general. Looks like that may help narrowing down the location of escaping problems.

Besides converting the DeprecationWarning to a Syntax{Error,Warning}, I don't see an easy way to include the offending line (or even file). The place in the code where the strings are created has no idea *where* they are being defined. AIUI, either we special-case this, or we resolve #28028 (but I don't think the latter can go in 3.6).

Are SyntaxWarnings silent by default?  If not it can't even go into 3.7.

I hope the message can be improved for Python 3.6 as the warnings I see when running Django's test suite are rather useless to help find and fix the issues:

cpython/Lib/importlib/_bootstrap.py:205: DeprecationWarning: invalid escape sequence '\:'

Grepping for these characters patterns to try to figure out where they come from is quite difficult.

I did track down one instance in a docstring:
https://github.com/django/django/blob/82f8996785751c413e3b4ac12bf387f781c200d8/django/contrib/gis/maps/google/__init__.py#L41

Is the correct resolution to add an r prefix to the string? It could be nice if the release notes contained some guidance on that.

There definitely needs to be a better message for that. The problem is that the parser doesn't have access to the whole string (of course; it's being constructed!), so I think there are several possible venues here:

- Change DeprecationWarning to display the line which is faulty (unlikely);
- Promote these specific warnings to SyntaxErrors *only* if the warning is an error (i.e. Python was run with -W error), in which case the compilation of code is stopped either way, so there is no drawback (but only being a syntax error "sometimes" is likely to confuse people);
- Use a different warning class, but presumably we'd need a new one and we can't do that now since we're in beta (unless Ned thinks this is appropriate, but I doubt it).

Anything else, maybe? I'm sure there is a way to properly fix it; my personal favourite all things considered is the second option (which I've thought over and over recently), but I'm open to other, less intrusive alternatives.

My intention is to make this deprecation the least painful possible for application and third-party library developers. As such, I think adding a note to the docs (possibly in What's New, but probably also under a few other related places) is good, and probably doing a preemptive SO question+answer for people who stumble upon this.

I've added to nosy a couple of people whose feedback I believe could be very helpful.

Adding a "DeprecatedSyntaxWarning" that's a subclass of both DeprecationWarning and SyntaxWarning (and hence silenced by default while still having the extra SyntaxWarning attributes) strikes me as a reasonable step to take here, even though we're into the beta period already - if we couldn't effectively respond to this kind of pre-release usability feedback, the beta period would be nowhere near as effective.

Ideally, this would also let you hook into the same system that ensures syntax warnings are correctly attributed to the code containing them from a warning filter perspective, rather than the import statement triggering the compilation. One possible way to do that would be to postpone generating this error to a later stage of the code generation pipeline rather than doing it directly in the parser.

Otherwise folks trying to constrain "-Werror" to just their own code and not their dependencies are going to have a bad time of it. If we find we can't readily fix that aspect of the change, then I'd go so far as proposing to revert it and trying again for 3.7. If we do find we need to take that step, we could still encourage linter developers to start complaining about unknown escapes during the 3.6 cycle.

P.S. As general background, the "No new features after beta 1" rule is more accurately written as "No new features after beta 1, *except* for features needed to resolve usability problems reported with changes that already landed in beta 1". The biggest one of those I've seen was the PEP 492 implementation for 3.5, where the implementation data model was originally still just "generators with a particular flag set" (similar to the 3.4 model), but the Tornado and Cython folks found that unworkable after the beta went out, so we made both the Python API and C API expose them as distinct classes with different attributes (cr_* instead of gi_*) and different ABCs that the asyncio checked for.

Thank you Nick for the useful feedback! I think that a subclass of DeprecationWarning and SyntaxWarning would be a good idea; I'll play around with that.

As far as when the warning should occur, I agree that erroring out at the compile step isn't optimal, however (AFAIK) strings aren't checked again once they've been created; they're assumed to be correct (and checking them at runtime would presumably add a performance hit to *all* strings, which I think is too big a price to pay).

On the other hand, a way to ignore invalid escapes in certain files could be considered, but we need to be somewhat careful since some people might choose to silence all warnings, thus defeating the purpose.

BTW, this will also help to make warnings more friendly in #26182.

I realised I wasn't entirely clear about the "warning misattribution" problem that's implied by Chi Hsuan's problem report, so here's the behaviour when using "-W all" rather than "-W error":

    $ echo "print('\d')" > bad_escape.py
    $ echo "import bad_escape" > escape_warning.py
    $ ./python -W all escape_warning.py 
    _frozen_importlib:205: DeprecationWarning: invalid escape sequence '\d'
    \d

That's a misattribution - the warning should be reported against the module being imported, not against the import system itself.

Compare that to the attribution of the old SyntaxWarning for assignments prior to a scope declaration:

    $ cat syntax_warning.py 
    def f():
        x = 1
        global x
    $ python3 -c "import syntax_warning" 
    /home/ncoghlan/devel/py36/syntax_warning.py:3: SyntaxWarning: name 'x' is assigned to before global declaration
      global x

(Run with 3.5, as that became a full SyntaxError for 3.6)

Regarding when I think the error should be generated, we definitely want the warning to be happening at compile time, but the "compile step" is actually a series of substeps.

The point where the string parser is processing string escapes is *not* the best place to complain about unrecognised ones, as it doesn't have the ability to report the appropriate context for where the offending string exists in the code base (hence this issue report, and the misattribution problem in the generated warning).

Instead, you probably want to look at delaying generation of the error until the point where AST Bytes and Str nodes are being generated for inclusion in the AST, as at that point the code generator has access to full file, line, and column information regarding the location of the problematic escape.

However, I also expect you'll run into a problem where you'll need to be able to embed *something* in the processed string that lets the latter stage of the pipeline detect that there was an unknown escape rather than a properly escaped usage of "\\" (which is presumably why the "immediate warning" approach was attempted first - it doesn't need that ability to communicate with the latter stage of the pipeline).

Hmm, I see; I'll need to dig a bit deeper get to and understand that part of the compile process better. I'll look up where SyntaxErrors are generated (since they have access to at least the line number at that point), and try to hook it up from there.

I've added Eric Smith to the nosy list as well, as he's been working on that part of the compilation pipeline for the f-string implementation.

Eric, the context here is that the new deprecation warning for unknown escapes is currently being misattributed to the code doing the compilation rather than to the code containing the offending (non-)escape sequence.

That's making it difficult for folks to act effectively on the warnings when they receive them, and also poses problems for correctly filtering out warnings coming from code in dependencies rather than an application's own code.

Couldn't we create a private version of PyUnicode_DecodeUnicodeEscape, to be called by ast.c, which passes back invalid escape info? Then have the actual warning raised by ast.c, which knows enough about the context to generate a better error/warning. I think we'd only be able to report on he first error in a string, though, but I haven't thought it all the way through.

I believe we'd only need to modify decode_unicode_with_escapes() in ast.c, which is called for both regular strings and f-string. And of course PyUnicode_DecodeUnicodeEscape would have to call the new private version and do the right thing.

Here is an extremely rough patch that shows the basic concept. I named the private function _PyUnicode_DecodeUnicodeEscape.

The problems with this patch are:
1. it always raises an error, not a warning
2. the private function isn't declared in a .h file
3. the name of the private function needs some thought
4. only the first invalid escape in a string is reported
5. I don't report the correct location in the string with the invalid escape
6. there may well be a memory leak
7. PEP 7 problems

#1 is because I was too lazy to refactor ast_error() to format the string I need without raising an error.

#5 could be solved with a callback and something to record multiple bad escapes per string, if we want to go that far. We'd have to decide how to show this. Multiple warnings, or one warning with multiple bad chars?

The rest of it is just quality of implementation stuff that we can work out if the approach is sound.

I forgot: this is what Nick's example now looks like:

$ ./python -Wall escape_warning.py
Traceback (most recent call last):
  File "escape_warning.py", line 1, in <module>
    import bad_escape
  File "/home/eric/local/python/cpython/bad_escape.py", line 1
    print('\d')
         ^
SyntaxError: invalid escape sequence \d

Also, I assume this is a problem with all such syntax warnings: you only see this warning/error when the file is originally compiled. Once the .pyc file exists, you'll never see a warning or error. Maybe that's okay, but it means there's a certain class of installations (such as .pyc compiled at install time) that won't be able to know these warnings exist.

This screwed me up for a while when I was developing this patch. The warnings disappeared!

Personally I'd be fine with only one warning, reporting the first invalid escape. Presumably the string would be checked as a whole, or would get an r prefix. Patch seems like a good start; bytes would also need updating in that regard. Don't worry too much about the details, we can iron that out later :)

Also yeah, the fact that warnings are triggered at compile time results in surprising behaviour even for me. I'm liking the idea of a DeprecatedSyntaxWarning more.

I'll try to get some work done in that regard by the end of next week, but life's been somewhat busy so I can't give any guarantees. Thank you for your patch draft though, it helps me to figure out how I should tackle this!

Eric, your patch was good enough to allow me to easily identify and fix all the warnings in Django: https://github.com/django/django/pull/7254. Thanks!

The basic concept LGTM.

first_invalid_escape_char is redundant, it is just s[first_invalid_escape_idx]. Or maybe better to return a pointer instead of an index.

bytes literals need similar solution.

Tim: Cool! That's way more useful than I thought it would be.

Serhiy: It's a proof of concept. Lots of design remains to be done. I'm not sure we've agreed on the concept yet, so I don't think it's worthwhile designing the API.

Eric's basic approach sounds fine to me.

The "pre-compiled .pyc files won't trigger SyntaxWarning" problem isn't new, as it exists for the old 3.5 warnings as well (-B prevents writing bytecode, which may be handy while working on this. Unfortunately, there's no equivalent to prevent reading it except deleting the offending bytecode file):

  $ python3 -B -c "import syntax_warning" 
  /home/ncoghlan/devel/py36/syntax_warning.py:3: SyntaxWarning: name 'x' is assigned to before global declaration
    global x
  $ python3 -c "import syntax_warning" 
  /home/ncoghlan/devel/py36/syntax_warning.py:3: SyntaxWarning: name 'x' is assigned to before global declaration
    global x
  $ python3 -c "import syntax_warning" 
  $ rm __pycache__/syntax_warning.cpython-35.pyc 
  $ python3 -B -c "import syntax_warning" 
  /home/ncoghlan/devel/py36/syntax_warning.py:3: SyntaxWarning: name 'x' is assigned to before global declaration
    global x

As long as folks are running their tests at least once in fresh environments they'll see the warning.

Ping. I'd like for this to get merged in beta 2; should I (or Eric if he wants to) get to work on this? Is the DeprecatedSyntaxWarning subclass route still desired?

Eric's basic approach sounds fine to me, as it gets the traceback in the right place (i.e. blaming the code being compiled, not the code doing the import).

For beta 2, how about we just go with a plain SyntaxWarning? Since users running pre-compiled modules won't see it regardless, so it will be effectively silenced by default for end users of pre-built Python applications.

Sorry for not responding earlier. It's unlikely I'll have time for this before beta 2, although I can probably get to it after that and before beta 3. Don't let me stop someone else from improving on the patch.

Following Ned's email to Python-Dev, I'm upping the priority of this issue to "deferred blocker".

I haven't had a lot of time to work on this, but I had time to think about it, and I think that resolving #28028 would be the best way to solved this. I pinged this issue as well and added several people to nosy there. I've also added it as a dependency of this issue.

To be clear, I really like Eric's approach to this, but I think that resolving #28028 would let us kill a couple of issues with the same patch. My priority is getting this issue here resolved, though, so either way works. I doubt I'll have much time to actually work on this; life's been busy and Python isn't my priority right now (as much as I'd like it to be).

Petr, is there any chance you or someone from your team could take a look at this and the other issue Emanuel referenced? These warnings are likely to pose a usability problem when upgrading Python in Fedora in their current state, so it would be good to see them addressed prior to the 3.6.0 release.

I'll work on this later today.

Here's an updated patch, that fixes some problems with the earlier patch, and adds equivalent support for bytes.

HOWEVER, I can't get the warnings machinery to raise a DeprecationWarning that would have all of the equivalent information that an actual SyntaxError would have. So this patch still raises SyntaxError, but with a better error context.

I'm going to keep plugging away at this, but I'm hoping that someone with more experience with warnings using the C-API can chime in with some advice. Given the tight deadline, I can use all of the help I can get.

The two functions that need help are decode_bytes_with_escapes and decode_unicode_with_escapes in ast.c.

Oops, use 28128-3.diff.

Thank you Eric. Have you looked at making a new DeprecatedSyntaxWarning subclass of both DeprecationWarning and SyntaxWarning? Hopefully that's of some help.

I don't see a review link, but from a quick glance this looks good. Thanks :)

I'll take a look at it, Emanuel. But I can't promise how much progress I'll be able to make today. I also think that at that point it becomes so complex that it fails Ned's test for inclusion in 3.6.

Updated patch emits DeprecationWarning instead of SyntaxError. I still not tested it.

Fixed bytes literals decoding (test_codecs was failed).

Serihy:

I had tried this approach earlier, but it doesn't work. With your -5.diff patch, the output is (using Nick's test case):

$ rm -rf __pycache__/ ; ./python -Werror escape_warning.py 
Traceback (most recent call last):
  File "escape_warning.py", line 1, in <module>
    import bad_escape
DeprecationWarning: invalid escape sequence \d
$ 

With my -4.diff patch, you get the desired full stack trace:

$ rm -rf __pycache__/ ; ./python -Wall escape_warning.py 
Traceback (most recent call last):
  File "escape_warning.py", line 1, in <module>
    import bad_escape
  File "/home/eric/local/python/cpython/bad_escape.py", line 1
    print('\d')
         ^
SyntaxError: invalid escape sequence \d
$ 

The trick is: how to make the DeprecationWarning version produce output similar to the SyntaxError case? Note that with DeprecationWarning, you don't see the line in bad_escape.py that actually contains the string with the invalid escape.

Added additional tests.

Also, you'll note that with or without your patch, you get the same behavior. The code in hg already raises DeprecationWarning, just in a different place. So unless we can improve the DeprecationWarning output, we're better off doing nothing.

Following patch just raises SyntaxError if DeprecationWarning was raised as error. Still needed tests for this.

> Also, you'll note that with or without your patch, you get the same behavior.

Not the same. New warnings contain correct information about a file and a line.

$ ./python-unpatched -Wa escape_warning.py
_frozen_importlib:205: DeprecationWarning: invalid escape sequence '\d'
\d

$ ./python-patched -Wa escape_warning.py
/home/serhiy/py/cpython-3.6/bad_escape.py:2: DeprecationWarning: invalid escape sequence \d
  print('\d')
\d

$ ./python-unpatched -We escape_warning.py
Traceback (most recent call last):
  File "escape_warning.py", line 1, in <module>
    import bad_escape
DeprecationWarning: invalid escape sequence '\d'

$ ./python-patched -We escape_warning.py
Traceback (most recent call last):
  File "escape_warning.py", line 1, in <module>
    import bad_escape
  File "/home/serhiy/py/cpython-3.6/bad_escape.py", line 2
    print('\d')
         ^
SyntaxError: invalid escape sequence \d

I'm not in front of a computer at the moment, but the output looks good. Also, my very quick glance at -7.diff's warn_invalid_escape_sequence looks reasonable, although I can't say for sure whether raising the error in PyErr_WarnExplicitObject followed by raising another error in ast_error is absolutely correct (it's outside my expertise).

New changeset 259745f9a1e4 by Eric V. Smith in branch 'default':
Issue 28128: Print out better error/warning messages for invalid string escapes.
https://hg.python.org/cpython/rev/259745f9a1e4

I've pushed this to the default branch. I'll watch the buildbots.

Then Ned can decide if this goes in to 3.6.

Searching on GitHub it seems to me that the most frequent issue with supporting Python 3.6 is eliminating or silencing warnings about invalid escape sequences. Any help with this is very important.

As Nick pointed out in an earlier message on this thread and as Serhiy observed on GitHub issues, backporting this patch to 3.6 is a must. Large projects' use of Python 3.6 has shown that it's hard to track down the actual cause of the error; it only makes sense to improve that before the final release.

Serhiy, are you working on #28028 alongside this, or can it be removed from the dependencies?

I agree it would be nice to get this in to 3.6. I'm not sure I'd go so far as to say it's a must and can't wait for 3.6.1. It's a non-trivial change, and it's up to Ned to say if it can go in to 3.6.

If you don't run with -Wall or -Werror, then you won't notice any new behavior with invalid escapes, correct?

Maybe post to python-dev and see if we can get more reviewers?

I agree that the current behavior for 3.6 is very user-unfriendly so I think the risks of making such an extensive change at this point in the release cycle are outweighed by the severity of the problem.  So let's get it into 3.6 now; there's still time for it to make 360b3.

Even better than what I was aiming for :)

The patch is working well to identify warnings when running Django's test suite. Thanks!

I'll work on this as soon as I can, coordinating with Ned.

The error message is much better now, thanks you all!

Seems the ^ pointer is not always correct. For example, in the function scope it's correct:

$ cat test.py 
def foo():
    s = 'C:\Program Files\Microsoft'

$ python3.7 -W error test.py
  File "test.py", line 2
    s = 'C:\Program Files\Microsoft'
           ^
SyntaxError: invalid escape sequence \P

On the other hand, top-level literals confuses the pointer:

$ cat test.py               
s = 'C:\Program Files\Microsoft'

$ python3.7 -W error test.py
  File "test.py", line 1
    s = 'C:\Program Files\Microsoft'
       ^
SyntaxError: invalid escape sequence \P

Is that expected?

Using 259745f9a1e4 on Arch Linux 64-bit

New changeset ee82266ad35b by Eric V. Smith in branch '3.6':
Issue 28128: Print out better error/warning messages for invalid string escapes. Backport to 3.6.
https://hg.python.org/cpython/rev/ee82266ad35b

New changeset 7aa001a48120 by Eric V. Smith in branch 'default':
Issue 28128: Null merge with 3.6: already applied to default.
https://hg.python.org/cpython/rev/7aa001a48120

Chi Hsuan Yen:

I'll investigate, and open another issue as needed.