Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give an error if there is a variable annotation within a function but no signature #3948

Closed
JukkaL opened this issue Sep 12, 2017 · 21 comments · Fixed by #13851
Closed

Give an error if there is a variable annotation within a function but no signature #3948

JukkaL opened this issue Sep 12, 2017 · 21 comments · Fixed by #13851

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 12, 2017

Mypy should perhaps give an error about a variable annotation in an otherwise unanotated function. Example:

def f():
    a: int = 'x'   # Maybe this annotation should be an error?

The rationale is that mypy will ignore the type annotation since the function is still considered unannotated, but this is confusing because there is an annotation within the function so it can appear to be annotated.

If --check-untyped-defs is being used this error shouldn't be generated.

Originally reported in #3945.

@elazarg
Copy link
Contributor

elazarg commented Sep 12, 2017

Maybe mypy should simply consider function as being annotated if there is any annotation inside it. Not only arguments.

@ilevkivskyi
Copy link
Member

Maybe mypy should simply consider function as being annotated if there is any annotation inside it. Not only arguments.

I think a warning/note would be better. People are sometimes unfamiliar with --check-untyped-defs, when reminded, they can decide what to do.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Sep 13, 2017

I agree with @ilevkivskyi. Also, it's a nice property that you can always recognize an annotated function by just looking at the signature.

@gvanrossum
Copy link
Member

Let's do it.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Feb 26, 2018

Another instance: #4636. Increasing priority since this could give a pretty bad impression for new users.

@rotemdan
Copy link

rotemdan commented Feb 26, 2018

@JukkaL

I don't think it's just about new users. I can imagine cases of experienced users forgetting to annotate a function return type or working on a partially annotated codebase that's in the process of conversion (possibly by multiple people).

Also, as a longtime TypeScript user. I'm used to having function return type inferred as undefined (rough equivalent of void/None) by default if a return statement is not included in the function body. I find it strange why it isn't the case here as well.

@rotemdan
Copy link

rotemdan commented Feb 26, 2018

@JukkaL

In this example (one of the examples I gave in #4636) the main function body doesn't include an annotated variable. So I guess the error described in this issue wouldn't apply here?

from typing import List

def f(a: List[List[int]]) -> List[List[int]]:
	# a.append([1,2,3]) # <-- uncommenting this line seems to have no effect as well
	return a

def main(): # <-- would there be a warning here?
	test = f([])
	test[0]['abcd'] = 'hi' # <- doesn't error?

@JukkaL
Copy link
Collaborator Author

JukkaL commented Feb 26, 2018

@rotemdan If you use --disallow-untyped-defs mypy will complain about your example.

@rotemdan
Copy link

@JukkaL

Thanks. I've already discovered and using it :) (and enabled several other options).

It seems quite confusing/unexpected that errors do appear in unannotated functions in some circumstances -- In such a way that it might give a false sense that the functions are actually being fully checked. If there were no errors at all it would have been easier to suspect that was possibly related.

I believe that many users would rather 'stick to the defaults' and not tinker with command line arguments unless they felt it was necessary -- for that reason I would suggest to consider not to show any errors at all for unannotated functions, or alternatively, have the 'check-all-methods' flag as an 'opt-out' instead of an 'opt-in'.

Also, the error/warning described here should probably include the case where the function scope has at least one call/assignment to/from an annotated function/attribute/property etc. -- since it would be natural to assume the arguments passed and return value would be checked correctly (after all, it does appear to be annotated, as the IDE might support that assumption as well).

@JukkaL
Copy link
Collaborator Author

JukkaL commented Feb 26, 2018

It seems quite confusing/unexpected that errors do appear in unannotated functions in some circumstances -- In such a way that it might give a false sense that the functions are actually being fully checked. If there were no errors at all it would have been easier to suspect that was possibly related.

Do you remember which errors have you seen in unannotated functions? Most errors are disabled in unannotated functions.

Also, the error/warning described here should probably include the case where the function scope has at least one call/assignment to/from an annotated function/attribute/property etc. -- since it would be natural to assume the arguments passed and return value would be checked correctly (after all, it does appear to be annotated, as the IDE might support that assumption as well).

There are various --disallow-* options that you can use to achieve this, but they tend to generate a lot of errors, some of which are hard to work around (Any types in stubs or missing stubs, etc.).

@rotemdan
Copy link

rotemdan commented Feb 26, 2018

@JukkaL

Trying again seems like the errors seen in the annotated functions were actually from pylint, not mypy! :) Sorry for that! I'm running both at the same time (and also reasonably new to mypy/Python).

Anyway, this seems like a usability issue, rather than having to do with the checking itself. I've tried some of the flags, in particular --disallow-untyped-defs and I found it to be generally unusable since the amount of unaddressable errors was too high (personally even 1 is too many for me). I did enable --disallow-incomplete-defs and found it helpful.

Anyway, without --check-untyped-defs enabled it now feels like having this added mental overhead of always being concerned about some method not being checked for the silly reason that it doesn't have a spurious -> None attached to it. This sort of defeats the purpose of a type checker -- that's supposed to protect against common or careless mistakes. I would have expected having something like an opt-out @unchecked decorator or # mypy: unchecked comment at the head or middle of the file, though right now I'd have no use for these personally.

I might also consider suggesting the vscode extension authors to have --check-untyped-defs as a default. Right now their defaults are --ignore-missing-imports and --follow-imports=silent.

@kamahen
Copy link
Contributor

kamahen commented Feb 26, 2018 via email

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jun 14, 2019

A user was confused by this again in #6986.

@ethanhs
Copy link
Collaborator

ethanhs commented Jul 29, 2021

I think it would be better to make --check-untyped-defs a multi-state argument. If we enabled warning about type annotations in untyped defs by default I'm not sure how one would turn it off without sprinkling a bunch of type ignores across the code base.

I propose the following instead, allowing an escape hatch for those that do not want this behavior:

--untyped-defs takes 3 values:

  • check (which is the same as the current --check-untyped-defs)
  • warn, which would become the default (we check the body and warn about types in the body)
  • silent, which is the current default, we ignore everything in the body of an untyped def.

@erictraut
Copy link

Has there been any consideration of enabling "--check-untyped-defs" by default — or removing the option entirely and always analyzing all functions? I see frequent mypy bug reports where users are confused about why obvious type errors are not reported, and often it's because the signature has no annotation. If all unannotated parameters are assumed to be Any, there's little or no risk of false positive errors. Is the concern more about performance?

For comparison, pyright always analyzes all code without regard for whether it's annotated. It doesn't even provide a way to disable analysis for non-annotated functions. We have not had problems with false positives.

@ethanhs
Copy link
Collaborator

ethanhs commented Jul 30, 2021

For comparison, pyright always analyzes all code without regard for whether it's annotated. It doesn't even provide a way to disable analysis for non-annotated functions. We have not had problems with false positives.

Personally the main reason I prefer the current default is it makes gradual adoption of typing much easier. If someone adds mypy to a new code base with --check-untyped-defs the default, it would add thousands of errors. We already hear a lot of feedback about the high number of errors making adoption discouraging.

@JelleZijlstra
Copy link
Member

Another way of looking at that is that the current gradual typing system doesn't do a good job of making typing really gradual: mypy already produces tons of errors even for a codebase without annotations. So maybe we need a better heuristic than the current one, which is to ignore the bodies of functions with no type annotations.

@erictraut
Copy link

@ethanhs, like I mentioned, pyright doesn't provide any way to skip analysis of individual functions based on whether their signature is annotated. We haven't found that to be an impediment to gradual typing. If some mypy users find that --check-untyped-defs = False is useful in some circumstances, the option could be retained even if it defaults to True.

Here are some other cases where the current default has tripped up mypy users who have reported the default behavior as a bug:
#10895, #10841, #10643, #10055, #9207, #9072, #8775, #8566, #8531, #8434. That's a pretty strong signal!

@ethanhs
Copy link
Collaborator

ethanhs commented Jul 31, 2021

Another way of looking at that is that the current gradual typing system doesn't do a good job of making typing really gradual: mypy already produces tons of errors even for a codebase without annotations. So maybe we need a better heuristic than the current one, which is to ignore the bodies of functions with no type annotations.

Absolutely! I bet a lot of errors we give are for cases where someone passes Optional[T] to a function expecting T due to some missed narrowing. But checks like those are really important and provide a lot of the value of a type checker, so I'd be very hesitant to turn them off.

pyright doesn't provide any way to skip analysis of individual functions based on whether their signature is annotated. We haven't found that to be an impediment to gradual typing.

Hm, how are you evaluating that it isn't an impediment to people's adoption of gradual typing? I think it would be quite hard to measure that.

If some mypy users find that --check-untyped-defs = False is useful in some circumstances, the option could be retained even if it defaults to True.

This is certainly an option. I worry that "welcome to static typing in Python, download mypy, here are a list of flags you will want to add to not get a thousand errors" is a bad UX, but I think that is certainly a balancing act with "why is this error not getting caught".

Here are some other cases where the current default has tripped up mypy users who have reported the default behavior as a bug:
#10895, #10841, #10643, #10055, #9207, #9072, #8775, #8566, #8531, #8434. That's a pretty strong signal!

Certainly! And I am very aware of it, but I also want to keep in mind the signal of people I've talked to in conferences, meetups, and online who have said the large number of initial errors is demoralizing and been a blocker to adopting typing.

Perhaps mypy could just say "not reporting errors in untyped functions, for more see " or something and exit 0 still. I don't love this option either but I think it is a reasonable middle ground.

@erictraut
Copy link

Hm, how are you evaluating that it isn't an impediment to people's adoption of gradual typing? I think it would be quite hard to measure that.

This is based on personal experience of gradually typing a half-million-line Python code base. Admittedly, other users' experience may differ from mine. But I developed pyright with gradual typing in mind.

@ethanhs
Copy link
Collaborator

ethanhs commented Jul 31, 2021

This is based on personal experience of gradually typing a half-million-line Python code base. Admittedly, other users' experience may differ from mine. But I developed pyright with gradual typing in mind.

Mypy can certainly be strict in some cases where it shouldn't be but we've definitely made improvements to make gradual typing easier. Unfortunately I expect you and I both are poor test subjects as we already have familiarity with static typing, but perhaps I should try gradually typing a codebase with pyright and see what we can improve on in mypy :)

ilevkivskyi added a commit that referenced this issue Oct 9, 2022
Closes #3948

This however gives a note instead of an error, so that type checking
result will be success. I also added an error code for the note so that
people can opt-out if they want to.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment