error reporting includes columns by bavardage · Pull Request #2163 · python/mypy

bavardage · 2016-09-21T14:33:21Z

bavardage · 2016-09-21T15:14:37Z

(the last three commits are clean-ups from the rebase, I can interactive-rebase those away if desired)

bavardage · 2016-09-21T15:16:25Z

also, not sure if there's any CI verification? but

bduffield05-mac:mypy bduffield$ ./runtests.py
PARALLEL 2
SUMMARY  212 tasks selected
SUMMARY  all 212 tasks and 3525 tests passed
*** OK ***

gracew

(as someone totally new to this codebase) this generally makes sense to me, aside from a couple questions.

gracew · 2016-09-21T22:09:09Z

mypy/errors.py

                    errors[j][1] == errors[i][1]):
-                if errors[j] == errors[i]:
+                if (errors[j][3] == errors[i][3] and
+                        errors[j][4] == errors[i][4]):  # ignore column


don't the 0th and first elements need to be compared too?

also, is ignoring the column necessary b/c of the TODO in TypeConverter#generic_visit in fastparse.py?

they're taken care of because we're within the while loop.

Ignoring the column here, so that we only get the first instance of this particular error within the line. This logic could be tweaked, for sure. Maybe we actually do want to expose all instances of a given error. E.g.

invalid type at column 1

invalid type at column 5

gracew · 2016-09-21T22:15:16Z

mypy/lex.py

            last_tok.string += self.pre_whitespace + s
            self.i += len(s)
            self.line += 1
+            self.column = 0


why the assignment of 0? the value of i, or 1 character beyond the end of the string seems to make sense for describing a line break

so we're incrementing the line here, so have to reset column back to 0.
in this class, self.column is keeping track of the column we're at (i.e. within the line) whereas self.i keeps track of the position within the overall string.

gracew · 2016-09-21T22:21:17Z

mypy/nodes.py

inconsistent to return self? do we need to do the same stuff with self.initializer, self.variable, and self.initialization_statement as above in set_line?

perhaps? I just matched the behaviour of set_line. Not sure what the philosophy is generally regarding chaining.

just matching behaviour of set_line - not sure of philosophy around chaining generally

there are some places in the code where already we do
some_thing = SomeNode(blah).set_line(1)
so with this they become
some_thing = SomeNode(blah).set_line(1).set_column(2)

perhaps this should be removed for now (until I figure out (in FLUP?) how the delegation should work

I find return self an anti-pattern that encourages unneeded cleverness and confuses side effects with functions.

Maybe set_line() should get an optional column method? That would also beautifully allow you to call it with a Token or Node and copy both line and column from there.

(Also, what's FLUP? Googling was inconclusive. :-)

(And, if you can remove the return self from set_line() and fix the fallout, if any, that would be great.)

FLUP oops.. :P follow-up, as in - in a future PR

and yep, will combine them/see how bad the fallout is if we take out the return self (think it shouldn't be that bad at all, from the places I've seen set_line so far)

gracew · 2016-09-21T22:23:31Z

mypy/nodes.py

do we need to handle self.arguments?

yep, probably - was going to investigate that in future

off the top of my head I can't remember how tokens -> argument node happens, it's possible they already have the column (and actually from editing the tests, my gut says they do)

perhaps this should be removed for now, until I figure out how (in FLUP?) to set columns for args

gvanrossum · 2016-09-21T22:57:39Z

Thanks! This PR clearely represents a significant contribution.

Normally the tests run automatically on Travis-CI. I don't know why they didn't run this time but I suspect the sheer size of the diff might explain that.

I find such a huge diff (66 files!) hard to review myself. Could you perhaps come up with a way to reduce most of the "trivial" changes e.g. the endless "E:" -> "E:0:" in the tests and the addition of of ", 0" in the errors.report() calls? (Maybe something with argument default values?)

bavardage · 2016-09-22T02:28:19Z

Not sure if there have been contributions from a fork repo before - not familiar with travis, but for circle (https://circleci.com/) there's some explicit setting to run builds for PRs from forks.

Yep, the structure of the tests (that the tests test error messages and this changes the error messages, as a feature :P) makes this a very scary PR.

Some thoughts of approaches here:

I could split off some of this into independent PRs
- the first four commits of this PR are (I believe) valuable standalone
- I can additionally chop out the non-error-reporting changes from the 5th commit (7f38445)
- at this point we have the column information in the Nodes, but no changes to error messages. I could write unit tests (i.e. not checking output) to verify that this is good.
- we'd at some point have some fairly scary-large change when the error reporting change actually happens, but that would be all it was
I could modify the way the test-checking works, have the 'Expected' not be compared absolutely but instead as a pattern.
- The existing E: blah could produce the pattern file:line:(\d+:)? error: blah
- Then a sufficient subset of the test assertions could be modified (but a lot less than 1800 of them or whatever it was) such that we were comfortable about the error messages.
- There would still be the fairly large change from updating all of the tests that assert on the output directly (in an output section) rather than using the # E: .. syntax

refi64 · 2016-09-22T02:53:19Z

Travis is supposed to always run on PRs. @gvanrossum What if you try closing-and-reopening this PR? That sometimes works for me.

gvanrossum · 2016-09-23T19:14:25Z

OK, trying that...

refi64 · 2016-09-23T19:15:13Z

Looks like they're running now.

gvanrossum · 2016-09-23T19:39:51Z

And it's failed. Somehow GitHub then removed the Travis CI results, so here's the link:
https://travis-ci.org/python/mypy/jobs/162288057

And indeed the breakage looks like it's been caused by some new tests that were added recently.

Maybe we could add a command-line flag that must be set to enable column numbers? That way only a few tests would need to be updated (say, tests just for this feature).

Finally, I'm getting crashes when running mypy --fast-parser mypy; a quick pdb session suggests the problem is caused by a Return node not having a col_offset attribute, but that may well be the tip of the iceberg.

I would really like to merge this, but I want to see a smaller diff. Please?

gvanrossum · 2016-09-23T19:40:37Z

(Ih wait, now the test results are back. I wonder if it just takes a really long time for some reason?)

bavardage · 2016-09-24T06:10:29Z

Command line flag sounds reasonable - will try to get to it this weekend.

Will also look into the error you're seeing with --fast-parser :(

bavardage · 2016-09-24T23:49:08Z

tests passing! diff now ~ 1/10th of the old size
currently working on some tests explicitly for the column reporting...

gvanrossum · 2016-09-24T23:54:04Z

mypy/fastparse.py

Could you rename this back? That would reduce the diff size a bit more. I know the name would not be completely covering the semantics but then again the name isn't all that intuitive either way. I don't think the churn caused by the rename is worth it.

done - could totally change the name at some point in the future in a single-purpose PR if it becomes confusing

gvanrossum · 2016-09-24T23:56:09Z

(But thanks for reducing the diff size!)

bavardage · 2016-09-25T00:16:35Z

test-data/unit/check-expressions.test

 main:3: error: "str" has no attribute "x"
-main:4: error: Incompatible return value type (got "str", expected "int")
 main:4: error: Incompatible types in assignment (expression has type Callable[[], str], variable has type Callable[[], int])
+main:4: error: Incompatible return value type (got "str", expected "int")


the churn here (and in some other *.test files) is because now errors on earlier columns come first

That's fine!

…date tests

and thus.. - revert tests - modify the few tests so that they work with the new ordering (errors on smaller columns are shown first, regardless of whether the column number is printed or not)

…umbers in error reporting

additionally, set_line no longer returns self (and thus do associated cleanup)

gvanrossum · 2016-09-25T03:55:53Z

mypy/fastparse.py

Heh, I figured out why (by putting a pdb trap here). It's because visit_lambda() synthesizes a Return node and sets only the lineno. Once you fix that I think the getattr() call is no longer necessary (and it shouldn't be!).

sweet, yep - works

bavardage · 2016-09-25T20:45:24Z

mypy/fastparse.py


    def generic_visit(self, node: ast35.AST) -> None:
-        raise TypeCommentParseError(TYPE_COMMENT_AST_ERROR, self.line)
+        raise TypeCommentParseError(TYPE_COMMENT_AST_ERROR, self.line,


unfortunately I think we still need the getattr here... not all AST nodes come with column info, only the ones deriving from stmt, expr, etc...

(https://github.com/python/typeshed/blob/master/third_party/3/typed_ast/ast35.pyi)

That's fine!

gvanrossum

I'm going to merge this now, unless some quick tests reveal more issues. Thank you so much for your work on this PR, and for being flexible in response to my review comments!

gvanrossum · 2016-09-25T20:56:20Z

mypy/fastparse.py


    def generic_visit(self, node: ast35.AST) -> None:
-        raise TypeCommentParseError(TYPE_COMMENT_AST_ERROR, self.line)
+        raise TypeCommentParseError(TYPE_COMMENT_AST_ERROR, self.line,


That's fine!

bavardage force-pushed the bd/columns branch from 05caa92 to 02695d5 Compare September 21, 2016 15:12

gracew reviewed Sep 21, 2016

View reviewed changes

gvanrossum closed this Sep 23, 2016

gvanrossum reopened this Sep 23, 2016

gvanrossum requested changes Sep 24, 2016

View reviewed changes

bavardage commented Sep 25, 2016

View reviewed changes

Ben Duffield added 12 commits September 24, 2016 20:21

add column to context

8efc46d

add columns to nodes made by fastparse{,2}

3eb173f

lexer adds column to tokens

e8f9edb

semanal add columns

de8589d

add columns to error reporting, add columns to (deprecated) parse, up…

3811846

…date tests

fix whitespace in fastparse2 (after rebase)

d733c7e

fix semanal rebase

8457e7a

update tests

ec3b91d

some ast nodes do not come with column information

2c3391c

add a switch to determine whether columns are shown (defaults to off)

fb1205a

and thus.. - revert tests - modify the few tests so that they work with the new ordering (errors on smaller columns are shown first, regardless of whether the column number is printed or not)

plumb command-line option (--show-column-numbers) to turn on column n…

e580367

…umbers in error reporting

test helper no longer required

94d8079

Ben Duffield added 2 commits September 24, 2016 20:21

with_line_and_column -> with_line

1603b44

add some 'smoketests' for columns

7c17e92

bavardage force-pushed the bd/columns branch from 7c976a4 to 7c17e92 Compare September 25, 2016 00:29

Ben Duffield added 2 commits September 24, 2016 22:16

set_line now also does the job of set_column

99a0496

additionally, set_line no longer returns self (and thus do associated cleanup)

minor tweaks/cleanups

77e7399

gvanrossum reviewed Sep 25, 2016

View reviewed changes

correctly synthesize the AST node for lambda (i.e. include col_offset)

75bd4a8

bavardage commented Sep 25, 2016

View reviewed changes

gvanrossum approved these changes Sep 25, 2016

View reviewed changes

gvanrossum merged commit fde83d5 into python:master Sep 25, 2016

brettcannon mentioned this pull request Dec 14, 2018

Support column numbers from mypy microsoft/vscode-python#3707

Closed

Uh oh!

Conversation

bavardage commented Sep 21, 2016

Uh oh!

bavardage commented Sep 21, 2016

Uh oh!

bavardage commented Sep 21, 2016

Uh oh!

gracew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gracew Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Sep 21, 2016

Uh oh!

bavardage commented Sep 22, 2016

Uh oh!

refi64 commented Sep 22, 2016

Uh oh!

gvanrossum commented Sep 23, 2016

Uh oh!

refi64 commented Sep 23, 2016

Uh oh!

gvanrossum commented Sep 23, 2016

Uh oh!

gvanrossum commented Sep 23, 2016

Uh oh!

bavardage commented Sep 24, 2016

Uh oh!

bavardage commented Sep 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Sep 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gracew Sep 21, 2016 •

edited

Loading