Check for non-`None` expectations

[daviddrysdale]

From https://github.com/daviddrysdale/python-phonenumbers/issues/200#issuecomment-910799543:

it is possible for None to be returned, but a lot of implementation code expects a value and doesn't check for None

applies to phonenumberutil.(example_number | invalid_example_number | example_number_for_type | example_number_for_non_geo_entity | region_code_for_number | ndd_prefix_for_region) and to a lesser extent re_util.fullmatch, as it is only ever used in a boolean context.

[daviddrysdale]

@AA-Turner was there tooling/scripts etc. that allowed you to spot these potential problems? Thanks.

[AA-Turner]

Mainly by running mypy --exclude pb2/ -p tests and manually going through the errors to pick out patterns.

https://github.com/AA-Turner/python-phonenumbers/commit/a77ac161c0c30fbcbce23b9f17dfaf2784103f0a (in https://github.com/AA-Turner/python-phonenumbers/tree/find-errors) explicitly denotes the errors by adding either assert statements or # type: ignore comments. The minor issue with type checking the test suite is that some of these errors are wanted (when testing type edge cases or errors), but I'd probably focus on the assert calls for errors -- as I said most of these are related to handling of None.

Typeshed does note that best practice is to avoid union return types (https://github.com/python/typeshed/blob/master/CONTRIBUTING.md#conventions -- "avoid union return types: https://github.com/python/mypy/issues/1693"), although it notes that detecting where handling None has been missed is a useful feature, and so I erred on the side of more potential errors but more correctness.

There are 21 functions or methdos that return union types currently:

Public API:

• PhoneMetadata.metadata_for_region
• PhoneMetadata.short_metadata_for_region
• PhoneMetadata.metadata_for_nongeo_region
• PhoneMetadata.metadata_for_region_or_calling_code
• phonenumberutil.example_number
• phonenumberutil.invalid_example_number
• phonenumberutil.example_number_for_type
• phonenumberutil.example_number_for_non_geo_entity
• phonenumberutil.region_code_for_number
• phonenumberutil.ndd_prefix_for_region

Internal public functions:

• re_util.fullmatch

Internal private functions/methods:

• shortnumberinfo._region_code_for_short_number_from_region_list
• PhoneNumberMatcher._find
• PhoneNumberMatcher._extract_match
• PhoneNumberMatcher._extract_inner_match
• PhoneNumberMatcher._parse_and_verify
• phonenumberutil._choose_formatting_pattern_for_number
• phonenumberutil._example_number_anywhere_for_type
• phonenumberutil._number_desc_by_type
• phonenumberutil._region_code_for_number_from_list
• prefix._find_lang

I'd suggest most of the errors come from these -- as statically it can't be determined what the function will return, given that it is a value based call.

A

[daviddrysdale]

Mainly by running mypy --exclude pb2/ -p tests and manually going through the errors to pick out patterns.

Hmm, I'm not getting many errors with this (mypy-3.9 --exclude pb2 --show-error-codes -p tests) – I get the {REAL,CARRIER}_*_DATA attr-defined errors that your commit has fixes for, and the error that #215 fixes, but nothing else. Which mypy version do you have?

[daviddrysdale]

CI run also has no errors: https://github.com/daviddrysdale/python-phonenumbers/runs/3542166579?check_suite_focus=true

[AA-Turner]

Ahh, I see the difference -- I pulled in my standard mypy config file, which sets configs equal to the following mypy --exclude pb2 --show-error-codes --show-error-context --strict --allow-untyped-calls --allow-untyped-defs -p tests

If you run with that, you should see all the errors. mypy -V gives 0.910

The actual config is in an untracked pyproject.toml file -- excerpt below:

# mypy configuration
[tool.mypy]
# help finding errors
show_error_codes = true
show_error_context = true

# exclude protobuf directory
exclude = "pb2/.*"
# stubs
warn_incomplete_stub = true

# mypy --strict config:
warn_unused_configs = true
disallow_any_generics = true
disallow_subclassing_any = true
# disallow_untyped_calls = true  # turn off for tests/
# disallow_untyped_defs = true  # turn off for tests/
disallow_incomplete_defs = true
check_untyped_defs = true
disallow_untyped_decorators = true
no_implicit_optional = true
warn_redundant_casts = true
warn_unused_ignores = true
warn_return_any = true
no_implicit_reexport = true
strict_equality = true

A

[daviddrysdale]

I wonder if there's any way to persuade mypy that self.assertTrue(thing is not None, ...) has the same effect as assert thing is not None …

[AA-Turner]

I tried the obvious thing (to me) of a function that calls self.assertTrue(thing is not None, msg=...) and assert thing is not None, but it doesn't seem to work...

There are a few threads/issues around unittest and type narrowing, but from a few years ago (~2018) and with no resolution it seems.

Another thing I thought of was a genericly typed overload function on the same lines as the above (replacing calls to self.assertTrue(... is not None, ...) etc), but if you took e.g. T | None as your input and returned T or raised an assertation error, it wouldn't help here -- as the input generic type T would be bound to e.g. PhoneMetadata | None, as that's the type going in.

So seems this works by default with pytest, as they just use plain assert statements, but not with unittest//other testing libraries, unfortunately (unless I've misssed something)

A