Skip to content

Commit

Permalink
Exclude boto from tests due to broken Liskov Substitution Principle
Browse files Browse the repository at this point in the history 
Starting with python/mypy#2521 mypy is performing stricter function signature
checks.

This makes the stubs diverge from the actual implementation but makes the stubs
internally consistent.  Since this is an actual typing issue in the base
implementation, we need to defer to the original authors to fix it.

Sadly, in this case the breakage is rather fundamental and unlikely to get
fixed by upstream. Consider:

```
  class AWSAuthConnection(object):
    def make_request(self, method, path, headers=None, data='', host=None,
      auth_path=None, sender=None, override_num_retries=None,
      params=None, retry_handler=None): ...

  class AWSQueryConnection(AWSAuthConnection):
    def make_request(self, action, params=None, path='/', verb='GET'): ...
```

Hence, until we have a workaround for the error produced by Mypy, we're
excluding those stubs from being tested against.
  • Loading branch information
Lukasz Langa committed Dec 22, 2016
1 parent 576ada7 commit 4084296
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 3 deletions.
1 change: 1 addition & 0 deletions tests/mypy_blacklist.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
third_party/2and3/boto/.*
2 changes: 1 addition & 1 deletion third_party/2and3/boto/connection.pyi
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class AWSQueryConnection(AWSAuthConnection):
ResponseError = ... # type: Any
def __init__(self, aws_access_key_id: Optional[Any] = ..., aws_secret_access_key: Optional[Any] = ..., is_secure: bool = ..., port: Optional[Any] = ..., proxy: Optional[Any] = ..., proxy_port: Optional[Any] = ..., proxy_user: Optional[Any] = ..., proxy_pass: Optional[Any] = ..., host: Optional[Any] = ..., debug: int = ..., https_connection_factory: Optional[Any] = ..., path: str = ..., security_token: Optional[Any] = ..., validate_certs: bool = ..., profile_name: Optional[Any] = ..., provider: str = ...) -> None: ...
def get_utf8_value(self, value): ...
def make_request(self, action, params: Optional[Any] = ..., path: str = ..., verb: str = ..., *args, **kwargs): ...
def make_request(self, action, params: Optional[Any] = ..., path: str = ..., verb: str = ..., *args, **kwargs): ... # FIXME: signature incompatible with base class

This comment has been minimized.

Sign in to view

Copy link
@gvanrossum

gvanrossum Dec 23, 2016

Member

Could you change this (and other places) to # type: ignore # signature incompatible with base class -- then everything will be better, and we don't need to blacklist stubs (in the past, blacklisted stubs have tended to rot). The current stubs aren't even importable without triggering an error.

This comment has been minimized.

Sign in to view

Copy link
@ambv

ambv Dec 24, 2016

Contributor

Yeah, I can change the line as you suggest. Let me elaborate a second why I didn't, please advise.

The current stubs aren't even importable without triggering an error.

As I commented on python/mypy#2521, I think this should be fixed in Mypy. typeshed stubs describe reality that the user of the type checker cannot easily affect, both for third-party libraries and the standard library. So while it would be preferable for boto to fix their broken API, a stub defining the current API shouldn't be punished by being precise.

In general, I don't have good intuition on how much we should shape the stubs to work around Mypy's limitations (another example being python/mypy#1551 that affects #771 now). My gut feeling is that we should rather annotate the types correctly and fix Mypy forward. In the mean time, we can modify the stubs to work around issues but we should definitely mark those cases somehow to be able to revisit them.

My worry is that the Any and # type: ignore that we put to silence Mypy are going to also silence bugs for users in the future, long after the original limitations of Mypy were solved.

This comment has been minimized.

Sign in to view

Copy link
@gvanrossum

gvanrossum Dec 24, 2016

Member

Internally at Dropbox we use a convention where we use # type: ignore # <url> where <url> links to the GitHub tracker issue that should be fixed so that the # type: ignore can be removed. This is especially nice in combination with mypy --warn-unused-ignores which will flag those # type: ignore lines that are not suppressing any mypy errors. (That flag is not the default as there are some situations where the error is only generated for certain Python versions or platforms, but running it occasionally is very useful.)

The # type: ignore lines I am asking you to add here should link to python/mypy#1237 which is the exact same issue. As you can see from the discussion there it is not so clear that that is a simple mypy bug -- we would probably have to update PEP 484 and we would have to decide whether the hole this would open up in the type system (violating Liskov) is worth it.

IMO we are getting lots of value out of running the stubs through mypy -- before we started doing this we would have many broken stubs. So I strongly prefer to add the # type: ignore lines rather than skipping the entire file. For those cases (like #1551) which are actually mypy bugs, maybe seeing which lines in typeshed are affected serves as a motivation to fix the bug.

This comment has been minimized.

Sign in to view

Copy link
@gvanrossum

gvanrossum Dec 24, 2016

Member

There's no mypy flag to check for unnecessary Anys, but we can still add a comment to the source with an explanation of the Any and a link to the mypy issue. Occasionally people can check for these, or we can add a comment to the issue pointing back to the places in typeshed that can be fixed (or a recipe for how to find them).

There are also some clearly sub-optimal Anys in a few places that exist to work around the lack of dependent types -- but those would require a substantial addition to PEP 484 to fix. Examples include the return types of open() and pow(). For those we could link to an issue in the python/typing repo.
def build_list_params(self, params, items, label): ...
def build_complex_list_params(self, params, items, label, names): ...
def get_list(self, action, params, markers, path: str = ..., parent: Optional[Any] = ..., verb: str = ...): ...
Expand Down
4 changes: 2 additions & 2 deletions third_party/2and3/boto/s3/connection.pyi
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from .bucket import Bucket

from typing import Any, Optional, Text, Type
from typing import Any, Dict, Optional, Text, Type
from boto.connection import AWSAuthConnection
from boto.exception import BotoClientError

Expand Down Expand Up @@ -68,4 +68,4 @@ class S3Connection(AWSAuthConnection):
def lookup(self, bucket_name, validate: bool = ..., headers: Optional[Dict[Text, Text]] = ...): ...
def create_bucket(self, bucket_name, headers: Optional[Dict[Text, Text]] = ..., location: Any = ..., policy: Optional[Any] = ...): ...
def delete_bucket(self, bucket, headers: Optional[Dict[Text, Text]] = ...): ...
def make_request(self, method, bucket: str = ..., key: str = ..., headers: Optional[Any] = ..., data: str = ..., query_args: Optional[Any] = ..., sender: Optional[Any] = ..., override_num_retries: Optional[Any] = ..., retry_handler: Optional[Any] = ..., *args, **kwargs): ...
def make_request(self, method, bucket: str = ..., key: str = ..., headers: Optional[Any] = ..., data: str = ..., query_args: Optional[Any] = ..., sender: Optional[Any] = ..., override_num_retries: Optional[Any] = ..., retry_handler: Optional[Any] = ..., *args, **kwargs): ... # FIXME: signature incompatible with base class

0 comments on commit 4084296

Please sign in to comment.