Don't report errors inside unannotated functions #1334

Closed
gnprice opened this Issue Apr 6, 2016 · 7 comments

Comments

Projects
None yet
5 participants
@gnprice
Collaborator

gnprice commented Apr 6, 2016

When typechecking the following file, we give an error:

$ cat /tmp/foo.py
def f(foo):
  if foo is not None:
    dumps = foo.dumps
  else:
    from json import dumps

$ mypy /tmp/foo.py
/tmp/foo.py: note: In function "f":
/tmp/foo.py:5: error: Name 'dumps' already defined

Because the function f doesn't have a type annotation, we ought to let this code be without complaining about it -- that makes it easier to start using mypy in parts of a large file, and pay the cost of fixing up code like this to work with mypy incrementally.

(Thanks to @varenc for reporting this issue -- he tried putting an annotation on one new function in a 3kLOC file with 188 existing functions, and it had this one error which stopped him.)

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Apr 6, 2016

Collaborator

I think we should fix this issue not by fixing this particular case but by entirely not type checking unannotated functions. This would also speed up mypy on codebases that still have a significant fraction of functions unannotated.

I think there may be some cases (related to classes?) where we derive information from unannotated functions (maybe just __init__?). My inclination would be to address these cases by not gathering that information, if possible (e.g. resulting in classes with unannotated __init__ methods getting Any typed). @JukkaL, do you know when we currently do this?

Collaborator

ddfisher commented Apr 6, 2016

I think we should fix this issue not by fixing this particular case but by entirely not type checking unannotated functions. This would also speed up mypy on codebases that still have a significant fraction of functions unannotated.

I think there may be some cases (related to classes?) where we derive information from unannotated functions (maybe just __init__?). My inclination would be to address these cases by not gathering that information, if possible (e.g. resulting in classes with unannotated __init__ methods getting Any typed). @JukkaL, do you know when we currently do this?

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Apr 6, 2016

Member

I'm just speculating, but I wonder if this particular error is happening due to the slightly ad-hoc way we try to deal with conditional imports. The general rule is indeed that we don't type-check code inside un-annotated functions, but this particular error may come out of the semantic analysis, which always runs (or which may be triggered by the presence of the import in one of the branches).

Debugging tip: if you want to know where an error is generated, put a debug breakpoint (e.g. import pdb; pdb.set_trace()) in the code that generates the error (in this case name_already_defined() in semanal.py) and go up the stack a few frames to find out where it's being called and why. I've learned a ton this way.

Member

gvanrossum commented Apr 6, 2016

I'm just speculating, but I wonder if this particular error is happening due to the slightly ad-hoc way we try to deal with conditional imports. The general rule is indeed that we don't type-check code inside un-annotated functions, but this particular error may come out of the semantic analysis, which always runs (or which may be triggered by the presence of the import in one of the branches).

Debugging tip: if you want to know where an error is generated, put a debug breakpoint (e.g. import pdb; pdb.set_trace()) in the code that generates the error (in this case name_already_defined() in semanal.py) and go up the stack a few frames to find out where it's being called and why. I've learned a ton this way.

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Apr 6, 2016

Collaborator

This error is coming from semantic analysis, so skipping type checking wouldn't help here. We can probably not run type checking for unannotated functions if we move some things to an earlier phase. The reason we run type checking for all functions is actually so that mypy can export better type information for all AST nodes. These types were used by a compiler prototype that I wrote back in the day, but the information isn't very useful any more, though future features like IDE integration might still benefit from it.

Some behavior might change if we didn't run type checking for unannotated code. Nested annotated functions / classes wouldn't get type checked. We could perhaps special case these, but this seems like a pretty marginal use case. Also, not sure how attribute definitions would behave. In any case, we could infer attribute types during semantic analysis in unannotated functions, as the type would always be Any. We could also tweak semantic analysis to not complain about certain things such as name already defined in an unannotated function.

The best way to fix the original problem would be to support the idiom both in annotated and unannotated functions.

Collaborator

JukkaL commented Apr 6, 2016

This error is coming from semantic analysis, so skipping type checking wouldn't help here. We can probably not run type checking for unannotated functions if we move some things to an earlier phase. The reason we run type checking for all functions is actually so that mypy can export better type information for all AST nodes. These types were used by a compiler prototype that I wrote back in the day, but the information isn't very useful any more, though future features like IDE integration might still benefit from it.

Some behavior might change if we didn't run type checking for unannotated code. Nested annotated functions / classes wouldn't get type checked. We could perhaps special case these, but this seems like a pretty marginal use case. Also, not sure how attribute definitions would behave. In any case, we could infer attribute types during semantic analysis in unannotated functions, as the type would always be Any. We could also tweak semantic analysis to not complain about certain things such as name already defined in an unannotated function.

The best way to fix the original problem would be to support the idiom both in annotated and unannotated functions.

@ahaven

This comment has been minimized.

Show comment
Hide comment
@ahaven

ahaven Apr 6, 2016

I like the idea of avoiding unannotated functions entirely -- our codebase has a number of errors showing up in unannotated functions. (For example, issue #984)

ahaven commented Apr 6, 2016

I like the idea of avoiding unannotated functions entirely -- our codebase has a number of errors showing up in unannotated functions. (For example, issue #984)

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Apr 6, 2016

Member
Member

gvanrossum commented Apr 6, 2016

@ahaven

This comment has been minimized.

Show comment
Hide comment
@ahaven

ahaven Apr 6, 2016

I was liking @ddfisher's suggestion of not reporting errors for unannotated functions. It would get us much closer to having mypy running without errors on our codebase.

ahaven commented Apr 6, 2016

I was liking @ddfisher's suggestion of not reporting errors for unannotated functions. It would get us much closer to having mypy running without errors on our codebase.

@gvanrossum gvanrossum added this to the 0.3.2 milestone Apr 7, 2016

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Apr 7, 2016

Member

David and I discussed this a bit and we decided that (without looking at the code) the most reasonable approach is to keep doing the semantic analysis for unannotated functions, but to somehow suppress errors from it.

Member

gvanrossum commented Apr 7, 2016

David and I discussed this a bit and we decided that (without looking at the code) the most reasonable approach is to keep doing the semantic analysis for unannotated functions, but to somehow suppress errors from it.

@ddfisher ddfisher changed the title from Error on conditional import inside unannotated function to Don't report errors inside unannotated functions Apr 14, 2016

@ddfisher ddfisher self-assigned this Apr 14, 2016

@gvanrossum gvanrossum assigned gvanrossum and unassigned ddfisher Apr 20, 2016

gvanrossum added a commit that referenced this issue Apr 20, 2016

@ddfisher ddfisher closed this in #1415 Apr 22, 2016

ddfisher added a commit that referenced this issue Apr 22, 2016

Suppress errors from semantic analysis in dynamic (unannotated) funct…
…ions. (#1415)

* Suppress errors from semantic analysis in dynamic (unannotated) functions. Fix #1334.

* Don't suppress 'continue/break' outside loop.

* Rough and tumble fix to make tests pass. Not sure all these are right. Please review.

* Clean up SemanticAnalyzer.__init__().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment