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

Add machinery for marking places we subvert the type system #5467

Merged
merged 6 commits into from Aug 15, 2018
Merged
Diff settings

Always

Just for now

Copy path View file
@@ -0,0 +1,22 @@
"""A Bogus[T] type alias for marking when we subvert the type system
We need this for compiling with mypyc, which inserts runtime
typechecks that cause problems when we subvert the type system. So
when compiling with mypyc, we turn those places into Any, while
keeping the types around for normal typechecks.
Since this causes the runtype types to be Any, this is best used

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

runtype -> runtime?

in places where efficient access to properties is not important.
For those cases some other technique should be used.
"""

from mypy_extensions import FlexibleAlias
from typing import TypeVar, Any

T = TypeVar('T')

SUPPRESS_BOGUS_TYPES = False
if SUPPRESS_BOGUS_TYPES:
Bogus = FlexibleAlias[T, Any]
else:
Bogus = FlexibleAlias[T, T]
Copy path View file
@@ -15,6 +15,8 @@
from mypy.util import short_type
from mypy.visitor import NodeVisitor, StatementVisitor, ExpressionVisitor

from mypy.bogus_type import Bogus


class Context:
"""Base type for objects that are valid as error message locations."""
@@ -174,10 +176,10 @@ class SymbolNode(Node):
# TODO do not use methods for these

@abstractmethod
def name(self) -> str: pass
def name(self) -> Bogus[str]: pass

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

I would add a comment here (the place Bogus first appears in this file) explaining what is going on. My understanding is that there are quite a few places where name or fullname can be actually None while this is not reflected in the type.

Btw, I am surprised you need this for name too. I have seen several bugs because of fullname being None, but I have never encountered a situation where name is None. Did you see actual problems with it, or you just decided this from analogy with fullname?

This comment has been minimized.

@msullivan

msullivan Aug 14, 2018

Author Collaborator

I saw actual problems, but they can be easily fixed by setting a default _name.

This comment has been minimized.

@msullivan

msullivan Aug 14, 2018

Author Collaborator

To the empty string, that is. MypyFile appears to never have a real name set?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

I would prefer something like id.split('.')[-1], if it is possible of course.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

(for MypyFiles)


@abstractmethod
def fullname(self) -> str: pass
def fullname(self) -> Bogus[str]: pass

@abstractmethod
def serialize(self) -> JsonDict: pass
@@ -195,9 +197,9 @@ class MypyFile(SymbolNode):
"""The abstract syntax tree of a single source file."""

# Module name ('__main__' for initial file)
_name = None # type: str
_name = None # type: Bogus[str]
# Fully qualified module name
_fullname = None # type: str
_fullname = None # type: Bogus[str]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

I meant actually setting these two in MypyFile.__init__ (it seems to me id should be always available when we create it). But if it is to hard, then skip this.

This comment has been minimized.

@msullivan

msullivan Aug 14, 2018

Author Collaborator

It's doable but would require changing some types around and threading the module name into part of the parser where it isn't. Doesn't seem to be an advantage over patching up _fullname in semanal like everything else.

_name, meanwhile, looks like it never gets set at all, so probably doesn't need to exist.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

OK.

This comment has been minimized.

@msullivan

msullivan Aug 14, 2018

Author Collaborator

Though fixing name() to return something useful seems fine also so

# Path to the file (None if not known)
path = ''
# Top-level definitions and statements
@@ -236,10 +238,10 @@ def __init__(self,
else:
self.ignored_lines = set()

def name(self) -> str:
def name(self) -> Bogus[str]:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

Hm, the above problem is probably only because of MypyFile. Can we just fix this instead by always setting _name in __init__. Also I suppose we can fix _fullname for MypyFile because the module id is always known immediately.

return self._name

def fullname(self) -> str:
def fullname(self) -> Bogus[str]:
return self._fullname

def accept(self, visitor: NodeVisitor[T]) -> T:
@@ -404,12 +406,12 @@ def __init__(self) -> None:
self.is_static = False
# Name with module prefix
# TODO: Type should be Optional[str]
self._fullname = cast(str, None)
self._fullname = cast(Bogus[str], None)

@abstractmethod
def name(self) -> str: pass

def fullname(self) -> str:
def fullname(self) -> Bogus[str]:
return self._fullname


@@ -660,7 +662,7 @@ def __init__(self, func: FuncDef, decorators: List[Expression],
def name(self) -> str:
return self.func.name()

def fullname(self) -> str:
def fullname(self) -> Bogus[str]:
return self.func.fullname()

@property
@@ -725,7 +727,7 @@ def __init__(self, name: str, type: 'Optional[mypy.types.Type]' = None) -> None:
super().__init__()
self._name = name # Name without module prefix
# TODO: Should be Optional[str]
self._fullname = cast(str, None) # Name with module prefix
self._fullname = cast(Bogus[str], None) # Name with module prefix
# TODO: Should be Optional[TypeInfo]
self.info = cast(TypeInfo, None) # Defining class (for member variables)
self.type = type # type: Optional[mypy.types.Type] # Declared or inferred type, or None
@@ -748,7 +750,7 @@ def __init__(self, name: str, type: 'Optional[mypy.types.Type]' = None) -> None:
def name(self) -> str:
return self._name

def fullname(self) -> str:
def fullname(self) -> Bogus[str]:
return self._fullname

def accept(self, visitor: StatementVisitor[T]) -> T:
@@ -780,7 +782,7 @@ class ClassDef(Statement):
"""Class definition"""

name = None # type: str # Name of the class without module prefix
fullname = None # type: str # Fully qualified name of the class
fullname = None # type: Bogus[str] # Fully qualified name of the class
defs = None # type: Block
type_vars = None # type: List[mypy.types.TypeVarDef]
# Base class expressions (not semantically analyzed -- can be arbitrary expressions)
@@ -2054,7 +2056,7 @@ class is generic then it will be a type constructor of higher kind.
the appropriate number of arguments.
"""

_fullname = None # type: str # Fully qualified name
_fullname = None # type: Bogus[str] # Fully qualified name
# Fully qualified name for the module this type was defined in. This
# information is also in the fullname, but is harder to extract in the
# case of nested class definitions.
@@ -2195,7 +2197,7 @@ def name(self) -> str:
"""Short name."""
return self.defn.name

def fullname(self) -> str:
def fullname(self) -> Bogus[str]:
return self._fullname

def is_generic(self) -> bool:
Copy path View file
@@ -18,6 +18,7 @@
)
from mypy.sharedparse import argument_elide_name
from mypy.util import IdMapper
from mypy.bogus_type import Bogus

T = TypeVar('T')

@@ -341,8 +342,8 @@ def accept(self, visitor: 'TypeVisitor[T]') -> T:
return visitor.visit_any(self)

def copy_modified(self,
type_of_any: TypeOfAny = _dummy,
original_any: Optional['AnyType'] = _dummy,
type_of_any: Bogus[TypeOfAny] = _dummy,
original_any: Bogus[Optional['AnyType']] = _dummy,

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

Maybe add also a short comment about Bogus here (just recall that _dummy is Any)?

) -> 'AnyType':
if type_of_any is _dummy:
type_of_any = self.type_of_any
@@ -743,22 +744,22 @@ def __init__(self,
self.def_extras = {}

def copy_modified(self,
arg_types: List[Type] = _dummy,
arg_kinds: List[int] = _dummy,
arg_names: List[Optional[str]] = _dummy,
ret_type: Type = _dummy,
fallback: Instance = _dummy,
name: Optional[str] = _dummy,
definition: SymbolNode = _dummy,
variables: List[TypeVarDef] = _dummy,
line: int = _dummy,
column: int = _dummy,
is_ellipsis_args: bool = _dummy,
implicit: bool = _dummy,
special_sig: Optional[str] = _dummy,
from_type_type: bool = _dummy,
bound_args: List[Optional[Type]] = _dummy,
def_extras: Dict[str, Any] = _dummy) -> 'CallableType':
arg_types: Bogus[List[Type]] = _dummy,
arg_kinds: Bogus[List[int]] = _dummy,
arg_names: Bogus[List[Optional[str]]] = _dummy,
ret_type: Bogus[Type] = _dummy,
fallback: Bogus[Instance] = _dummy,
name: Bogus[Optional[str]] = _dummy,
definition: Bogus[SymbolNode] = _dummy,
variables: Bogus[List[TypeVarDef]] = _dummy,
line: Bogus[int] = _dummy,
column: Bogus[int] = _dummy,
is_ellipsis_args: Bogus[bool] = _dummy,
implicit: Bogus[bool] = _dummy,
special_sig: Bogus[Optional[str]] = _dummy,
from_type_type: Bogus[bool] = _dummy,
bound_args: Bogus[List[Optional[Type]]] = _dummy,
def_extras: Bogus[Dict[str, Any]] = _dummy) -> 'CallableType':
return CallableType(
arg_types=arg_types if arg_types is not _dummy else self.arg_types,
arg_kinds=arg_kinds if arg_kinds is not _dummy else self.arg_kinds,
@@ -1457,8 +1458,9 @@ class TypeType(Type):
# a generic class instance, a union, Any, a type variable...
item = None # type: Type

def __init__(self, item: Union[Instance, AnyType, TypeVarType, TupleType, NoneTyp,
CallableType], *, line: int = -1, column: int = -1) -> None:
def __init__(self, item: Bogus[Union[Instance, AnyType, TypeVarType, TupleType, NoneTyp,
CallableType]], *,
line: int = -1, column: int = -1) -> None:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 14, 2018

Collaborator

I am a bit surprised the Bogus is needed here, and not in make_normalized, where the actual hack happens. But most probably I am just missing something.

This comment has been minimized.

@msullivan

msullivan Aug 14, 2018

Author Collaborator

The crash does indeed happen in make_normalized, when it tries to coerce the arguments to the type required by __init__

"""To ensure Type[Union[A, B]] is always represented as Union[Type[A], Type[B]], item of
type UnionType must be handled through make_normalized static method.
"""
Copy path View file
@@ -0,0 +1,15 @@
[mypy]
disallow_untyped_defs = True
disallow_subclassing_any = True
warn_no_return = True
strict_optional = True
no_implicit_optional = True
disallow_any_generics = True
disallow_any_unimported = True
warn_redundant_casts = True
warn_unused_configs = True
always_true = SUPPRESS_BOGUS_TYPES

# needs py2 compatibility
[mypy-mypy.test.testextensions]
disallow_untyped_defs = False
Copy path View file
@@ -8,6 +8,7 @@ disallow_any_generics = True
disallow_any_unimported = True
warn_redundant_casts = True
warn_unused_configs = True
always_false = SUPPRESS_BOGUS_TYPES

# needs py2 compatibility
[mypy-mypy.test.testextensions]
Copy path View file
@@ -3,6 +3,7 @@ flake8
flake8-bugbear; python_version >= '3.5'
flake8-pyi; python_version >= '3.6'
lxml==4.2.4
git+git://github.com/python/mypy.git@5ed6919604c38a4ecfe3ec20d00687c267d8c6ca#egg=mypy_extensions&subdirectory=extensions
psutil==5.4.0
pytest>=3.4
pytest-xdist>=1.22
ProTip! Use n and p to navigate between commits in a pull request.