This is reasonable. But I did find some places where it makes things worse. E.g.

$ ./python.exe _.py
  File "_.py", line 1
    (0x+1)
       ^
SyntaxError: invalid hexadecimal literal

this positions the caret just past the invalid token (0x), whereas previously it was positioned at the final character:

$ python3 _.py
  File "_.py", line 1
    (0x+1)
      ^
SyntaxError: invalid token

It would behoove someone to carefully double-check all cases where a syntax error originates in the lexer -- there are a variety of cases.

Aah, good catch, I'll go through the errors and add a lot more test cases.

That issue has been resolved, I had failed to change tokenizer.c which still used 1-indexed column offsets. Added a bunch more test cases but they're still not exhaustive, I'd appreciate someone giving it a second look to find where else syntax errors can bubble up from.

Yup, that's what I did and then I grepped for check_syntax_error in the test suite and made sure I had a test for each case there.

Another potential downside is that this is a breaking change to PyErr_SyntaxLocationObject, which is exposed in the public C api.

Yeah that sounds good to me, lets keep around the new tests and make it all consistent to 1-indexed.

I thought that the offset is 0-based in most case. Just in some cases it points to the start of invalid token, and in other cases it points past the end of it. This depends on the place of raising an error: tokenizer, AST, compiler. In case of 1-character tokens this looks as 1-based offsets.

Anywhere its 0-indexed seems like a mistake because the caret printing code uses offset > 0 instead of offset >= 0, and there's old test cases like this: 503d6c5
where it looks like the offset is 1-indexed.

fwiw I think Guido's intuition about tools breaking from changing to 0-indexing is right, a vim plugin that does syntax checking for vim assumes SyntaxError's offset is 1-indexed: vim-syntastic/syntastic@6c91e8d

I presume 'now' in "Column offsets are now 0-indexed" meant after rather than before the original patch, though not 'now', after the patch revision.

tk and hence IDLE use 1-based line numbers and 0-based column slice offsets. The tk/IDLE text cursor is a vertical 'slice' bar positioned between characters, as in this comment box, and many (most?, all?) GUI editors. IDLE currently subtracts 1 (in three places) from what it assumes to be 1-based offsets. At the left margin it reports 0 on the status display and passes 0 as the start-slice argument when coloring a slice. Hence, IDLE highlights the ' ' before 1 in for 1 in a: pass, when the offset is 0-based. I most care about consistency.

I pulled this PR to a master-based branch and the Python caret and IDLE highlight still mark the space before 1, not 1 itself. So something does not seem to be fixed yet.

I personally would prefer 0-based slice positions and would happily remove the '-1's. But I notice that Notepad++ (not written in Python), reports 1, not 0, for the left-margin position. I can imagine that some python-based code might expect the same. AFAIK, text-based editors with characters cursors (underline or over-block), imitating terminals, use 1-based positions.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

That's weird, here's what I get on Linux:

$ ./python /tmp/s.py
  File "/tmp/s.py", line 1
    for 1 in a: pass
        ^
SyntaxError: can't assign to literal

and this is Windows:

λ win32\python D:\workspace\s.py
  File "D:\workspace\s.py", line 1
    for 1 in a: pass
        ^
SyntaxError: can't assign to literal

there's also a test that covers that case here: https://github.com/python/cpython/pull/9338/files#diff-435de67ff9c3be9a3ec7a5fd9550c1c0R216
5 is where the '1' ends up with 1-indexing

I almost never test patches with changes to .c files, so I inadvertently verified that the new tests fail without the new code compiled and running. Looks good now.

I guess the auto-merge doesn't work, either because some Azure tests flaked or because some reviewers didn't yet approve.