[issue28128] Improve the warning message for invalid escape sequences

[Donald Stufft]

Changes by Donald Stufft :


--
pull_requests: +922

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

Chi Hsuan Yen:

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

--
resolution:  -> fixed
stage: commit review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Chi Hsuan Yen]

Chi Hsuan Yen added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Tim Graham]

Tim Graham added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

Even better than what I was aiming for :)

--
dependencies:  -Convert warnings to SyntaxWarning in parser
priority: deferred blocker -> release blocker

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Ned Deily]

Ned Deily added the comment:

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.

--
stage: patch review -> commit review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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

Then Ned can decide if this goes in to 3.6.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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 
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 
import bad_escape
  File "/home/serhiy/py/cpython-3.6/bad_escape.py", line 2
print('\d')
 ^
SyntaxError: invalid escape sequence \d

--
Added file: http://bugs.python.org/file45293/28128-7.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Added additional tests.

--
Added file: http://bugs.python.org/file45292/28128-6.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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 
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 
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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Fixed bytes literals decoding (test_codecs was failed).

--
Added file: http://bugs.python.org/file45290/28128-5.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--
Added file: http://bugs.python.org/file45288/28128-4.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


Added file: http://bugs.python.org/file45287/28128-3.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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 :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

Oops, use 28128-3.diff.

--
Added file: http://bugs.python.org/file45285/28128-3.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Changes by Eric V. Smith :


--
stage: needs patch -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Changes by Eric V. Smith :


Added file: http://bugs.python.org/file45284/28128-2.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Changes by Eric V. Smith :


--
assignee:  -> eric.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

I'll work on this later today.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Ezio Melotti]

Changes by Ezio Melotti :


--
nosy: +ezio.melotti

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--
nosy: +encukou

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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).

--
dependencies: +Convert warnings to SyntaxWarning in parser
priority: normal -> deferred blocker

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Brett Cannon]

Changes by Brett Cannon :


--
nosy:  -brett.cannon

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Tim Graham]

Tim Graham added the comment:

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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 
import bad_escape
  File "/home/eric/local/python/cpython/bad_escape.py", line 1
print('\d')
 ^
SyntaxError: invalid escape sequence \d

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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.

--
keywords: +patch
Added file: http://bugs.python.org/file44694/28128.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Eric V. Smith]

Eric V. Smith added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--
nosy: +eric.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Nick Coghlan]

Nick Coghlan added the comment:

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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Nick Coghlan]

Nick Coghlan added the comment:

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)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Yury Selivanov]

Yury Selivanov added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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.

--
nosy: +brett.cannon, ncoghlan, rhettinger, yselivanov

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Tim Graham]

Tim Graham added the comment:

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.

--
nosy: +Tim.Graham

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[R. David Murray]

R. David Murray added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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).

--
assignee: ebarry -> 

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Martin Panter]

Martin Panter added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Emanuel Barry]

Emanuel Barry added the comment:

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 :)

--
assignee:  -> ebarry
components:  -Library (Lib)
nosy: +ebarry
stage:  -> needs patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[R. David Murray]

R. David Murray added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Ned Deily]

Ned Deily added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Chi Hsuan Yen]

New submission from Chi Hsuan Yen:

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 
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 
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.

--
components: Interpreter Core, Library (Lib)
messages: 276285
nosy: Chi Hsuan Yen, martin.panter, r.david.murray, serhiy.storchaka
priority: normal
severity: normal
status: open
title: Improve the warning message for invalid escape sequences
type: enhancement
versions: Python 3.6, Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28128] Improve the warning message for invalid escape sequences

[Chi Hsuan Yen]

Chi Hsuan Yen added the comment:

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?

--
nosy: +ned.deily

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com