Improve type checking (backport #679)

launch/doc/source/conf.py

@@ -32,6 +32,7 @@
 
 
 # -- Project information -----------------------------------------------------
+# type: ignore
 
 project = 'launch'
 copyright = '2018, Open Source Robotics Foundation, Inc.'  # noqa

launch/examples/disable_emulate_tty_counters.py

@@ -24,7 +24,6 @@
 
 import os
 import sys
-from typing import cast
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..'))  # noqa
 
 import launch  # noqa: E402
@@ -37,10 +36,9 @@ def generate_launch_description():
     ld.add_action(launch.actions.SetLaunchConfiguration('emulate_tty', 'false'))
 
     # Wire up stdout from processes
-    def on_output(event: launch.Event) -> None:
+    def on_output(event: launch.events.process.ProcessIO) -> None:
         for line in event.text.decode().splitlines():
-            print('[{}] {}'.format(
-                cast(launch.events.process.ProcessIO, event).process_name, line))
+            print('[{}] {}'.format(event.process_name, line))
 
     ld.add_action(launch.actions.RegisterEventHandler(launch.event_handlers.OnProcessIO(
         on_stdout=on_output,

launch/examples/launch_counters.py

@@ -24,7 +24,6 @@
 import os
 import platform
 import sys
-from typing import cast
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..'))  # noqa
 
 import launch  # noqa: E402
@@ -55,10 +54,9 @@ def main(argv=sys.argv[1:]):
 
     # Setup a custom event handler for all stdout/stderr from processes.
     # Later, this will be a configurable, but always present, extension to the LaunchService.
-    def on_output(event: launch.Event) -> None:
+    def on_output(event: launch.events.process.ProcessIO) -> None:
         for line in event.text.decode().splitlines():
-            print('[{}] {}'.format(
-                cast(launch.events.process.ProcessIO, event).process_name, line))
+            print('[{}] {}'.format(event.process_name, line))
 
     ld.add_action(launch.actions.RegisterEventHandler(launch.event_handlers.OnProcessIO(
         # this is the action     ^              and this, the event handler ^

launch/launch/__init__.py

@@ -32,8 +32,8 @@
 from .launch_description_source import LaunchDescriptionSource
 from .launch_introspector import LaunchIntrospector
 from .launch_service import LaunchService
-from .some_actions_type import SomeActionsType
-from .some_actions_type import SomeActionsType_types_tuple
+from .some_entities_type import SomeEntitiesType
+from .some_entities_type import SomeEntitiesType_types_tuple
 from .some_substitutions_type import SomeSubstitutionsType
 from .some_substitutions_type import SomeSubstitutionsType_types_tuple
 from .substitution import Substitution
@@ -57,8 +57,8 @@
     'LaunchDescriptionSource',
     'LaunchIntrospector',
     'LaunchService',
-    'SomeActionsType',
-    'SomeActionsType_types_tuple',
+    'SomeEntitiesType',
+    'SomeEntitiesType_types_tuple',
     'SomeSubstitutionsType',
     'SomeSubstitutionsType_types_tuple',
     'Substitution',

launch/launch/action.py

@@ -14,6 +14,7 @@
 
 """Module for Action class."""
 
+from typing import Dict
 from typing import Iterable
 from typing import List
 from typing import Optional
@@ -62,7 +63,7 @@ def parse(entity: 'Entity', parser: 'Parser'):
         from .conditions import UnlessCondition
         if_cond = entity.get_attr('if', optional=True)
         unless_cond = entity.get_attr('unless', optional=True)
-        kwargs = {}
+        kwargs: Dict[str, Condition] = {}
         if if_cond is not None and unless_cond is not None:
             raise RuntimeError("if and unless conditions can't be used simultaneously")
         if if_cond is not None:

launch/launch/actions/declare_launch_argument.py

@@ -14,7 +14,6 @@
 
 """Module for the DeclareLaunchArgument action."""
 
-from typing import Iterable
 from typing import List
 from typing import Optional
 from typing import Text
@@ -109,7 +108,7 @@ def __init__(
         *,
         default_value: Optional[SomeSubstitutionsType] = None,
         description: Optional[Text] = None,
-        choices: Iterable[Text] = None,
+        choices: List[Text] = None,
         **kwargs
     ) -> None:
         """Create a DeclareLaunchArgument action."""
@@ -196,7 +195,7 @@ def description(self) -> Text:
         return self.__description
 
     @property
-    def choices(self) -> List[Text]:
+    def choices(self) -> Optional[List[Text]]:
         """Getter for self.__choices."""
         return self.__choices
 

launch/launch/actions/execute_local.py

@@ -34,7 +34,7 @@
 
 import launch.logging
 
-from osrf_pycommon.process_utils import async_execute_process
+from osrf_pycommon.process_utils import async_execute_process  # type: ignore
 from osrf_pycommon.process_utils import AsyncSubprocessProtocol
 
 from .emit_event import EmitEvent
@@ -63,7 +63,7 @@
 from ..launch_context import LaunchContext
 from ..launch_description import LaunchDescription
 from ..launch_description_entity import LaunchDescriptionEntity
-from ..some_actions_type import SomeActionsType
+from ..some_entities_type import SomeEntitiesType
 from ..some_substitutions_type import SomeSubstitutionsType
 from ..substitution import Substitution  # noqa: F401
 from ..substitutions import LaunchConfiguration
@@ -97,8 +97,8 @@ def __init__(
         cached_output: bool = False,
         log_cmd: bool = False,
         on_exit: Optional[Union[
-            SomeActionsType,
-            Callable[[ProcessExited, LaunchContext], Optional[SomeActionsType]]
+            SomeEntitiesType,
+            Callable[[ProcessExited, LaunchContext], Optional[SomeEntitiesType]]
         ]] = None,
         respawn: Union[bool, SomeSubstitutionsType] = False,
         respawn_delay: Optional[float] = None,
@@ -193,9 +193,16 @@ def __init__(
         self.__sigterm_timeout = normalize_to_list_of_substitutions(sigterm_timeout)
         self.__sigkill_timeout = normalize_to_list_of_substitutions(sigkill_timeout)
         self.__emulate_tty = emulate_tty
-        self.__output = os.environ.get('OVERRIDE_LAUNCH_PROCESS_OUTPUT', output)
-        if not isinstance(self.__output, dict):
-            self.__output = normalize_to_list_of_substitutions(self.__output)
+        # Note: we need to use a temporary here so that we don't assign values with different types
+        # to the same variable
+        tmp_output: SomeSubstitutionsType = os.environ.get(
+                'OVERRIDE_LAUNCH_PROCESS_OUTPUT', output
+                )
+        self.__output: Union[dict, List[Substitution]]
+        if not isinstance(tmp_output, dict):
+            self.__output = normalize_to_list_of_substitutions(tmp_output)
+        else:
+            self.__output = tmp_output
         self.__output_format = output_format
 
         self.__log_cmd = log_cmd
@@ -336,7 +343,7 @@ def __on_signal_process_event(
     def __on_process_stdin(
         self,
         event: ProcessIO
-    ) -> Optional[SomeActionsType]:
+    ) -> Optional[SomeEntitiesType]:
         self.__logger.warning(
             "in ExecuteProcess('{}').__on_process_stdin_event()".format(id(self)),
         )
@@ -345,7 +352,7 @@ def __on_process_stdin(
 
     def __on_process_output(
         self, event: ProcessIO, buffer: io.TextIOBase, logger: logging.Logger
-    ) -> Optional[SomeActionsType]:
+    ) -> None:
         to_write = event.text.decode(errors='replace')
         if buffer.closed:
             # buffer was probably closed by __flush_buffers on shutdown.  Output without
@@ -396,7 +403,7 @@ def __flush_buffers(self, event, context):
 
     def __on_process_output_cached(
         self, event: ProcessIO, buffer, logger
-    ) -> Optional[SomeActionsType]:
+    ) -> None:
         to_write = event.text.decode(errors='replace')
         last_cursor = buffer.tell()
         buffer.seek(0, os.SEEK_END)  # go to end of buffer
@@ -423,7 +430,7 @@ def __flush_cached_buffers(self, event, context):
                 self.__output_format.format(line=line, this=self)
             )
 
-    def __on_shutdown(self, event: Event, context: LaunchContext) -> Optional[SomeActionsType]:
+    def __on_shutdown(self, event: Event, context: LaunchContext) -> Optional[SomeEntitiesType]:
         due_to_sigint = cast(Shutdown, event).due_to_sigint
         return self._shutdown_process(
             context,
@@ -578,9 +585,14 @@ async def __execute_process(self, context: LaunchContext) -> None:
             self.__logger.error("process has died [pid {}, exit code {}, cmd '{}'].".format(
                 pid, returncode, ' '.join(filter(lambda part: part.strip(), cmd))
             ))
-        await context.emit_event(ProcessExited(returncode=returncode, **process_event_args))
+        await context.emit_event(
+                ProcessExited(returncode=returncode, **process_event_args)
+                )
         # respawn the process if necessary
-        if not context.is_shutdown and not self.__shutdown_future.done() and self.__respawn:
+        if not context.is_shutdown\
+                and self.__shutdown_future is not None\
+                and not self.__shutdown_future.done()\
+                and self.__respawn:
             if self.__respawn_delay is not None and self.__respawn_delay > 0.0:
                 # wait for a timeout(`self.__respawn_delay`) to respawn the process
                 # and handle shutdown event with future(`self.__shutdown_future`)
@@ -608,7 +620,7 @@ def prepare(self, context: LaunchContext):
             # pid is added to the dictionary in the connection_made() method of the protocol.
         }
 
-        self.__respawn = perform_typed_substitution(context, self.__respawn, bool)
+        self.__respawn = cast(bool, perform_typed_substitution(context, self.__respawn, bool))
 
     def execute(self, context: LaunchContext) -> Optional[List[LaunchDescriptionEntity]]:
         """
@@ -663,7 +675,9 @@ def execute(self, context: LaunchContext) -> Optional[List[LaunchDescriptionEnti
             ),
             OnProcessExit(
                 target_action=self,
-                on_exit=self.__on_exit,
+                # TODO: This is also a little strange, OnProcessExit shouldn't ever be able to
+                # take a None for the callable, but this seems to be the default case?
+                on_exit=self.__on_exit,  # type: ignore
             ),
             OnProcessExit(
                 target_action=self,
@@ -678,9 +692,13 @@ def execute(self, context: LaunchContext) -> Optional[List[LaunchDescriptionEnti
             self.__shutdown_future = create_future(context.asyncio_loop)
             self.__logger = launch.logging.get_logger(name)
             if not isinstance(self.__output, dict):
-                self.__output = perform_substitutions(context, self.__output)
-            self.__stdout_logger, self.__stderr_logger = \
-                launch.logging.get_output_loggers(name, self.__output)
+                self.__stdout_logger, self.__stderr_logger = \
+                    launch.logging.get_output_loggers(
+                            name, perform_substitutions(context, self.__output)
+                            )
+            else:
+                self.__stdout_logger, self.__stderr_logger = \
+                    launch.logging.get_output_loggers(name, self.__output)
             context.asyncio_loop.create_task(self.__execute_process(context))
         except Exception:
             for event_handler in event_handlers:

launch/launch/actions/execute_process.py

@@ -29,6 +29,8 @@
 from ..frontend import expose_action
 from ..frontend import Parser
 from ..some_substitutions_type import SomeSubstitutionsType
+
+from ..substitution import Substitution
 from ..substitutions import TextSubstitution
 
 _global_process_counter_lock = threading.Lock()
@@ -254,7 +256,7 @@ def _parse_cmdline(
         :returns: a list of command line arguments.
         """
         result_args = []
-        arg = []
+        arg: List[Substitution] = []
 
         def _append_arg():
             nonlocal arg

launch/launch/actions/opaque_coroutine.py

@@ -17,7 +17,8 @@
 import asyncio
 import collections.abc
 from typing import Any
-from typing import Coroutine
+from typing import Awaitable
+from typing import Callable
 from typing import Dict
 from typing import Iterable
 from typing import List
@@ -29,19 +30,19 @@
 from ..event_handlers import OnShutdown
 from ..launch_context import LaunchContext
 from ..launch_description_entity import LaunchDescriptionEntity
-from ..some_actions_type import SomeActionsType
+from ..some_entities_type import SomeEntitiesType
 from ..utilities import ensure_argument_type
 
 
 class OpaqueCoroutine(Action):
     """
-    Action that adds a Python coroutine to the launch run loop.
+    Action that adds a Python coroutine function to the launch run loop.
 
-    The signature of a coroutine should be:
+    The signature of the coroutine function should be:
 
     .. code-block:: python
 
-        async def coroutine(
+        async def coroutine_func(
             context: LaunchContext,
             *args,
             **kwargs
@@ -52,7 +53,7 @@ async def coroutine(
 
     .. code-block:: python
 
-        async def coroutine(
+        async def coroutine_func(
             *args,
             **kwargs
         ):
@@ -63,7 +64,7 @@ async def coroutine(
 
     def __init__(
         self, *,
-        coroutine: Coroutine,
+        coroutine: Callable[..., Awaitable[None]],
         args: Optional[Iterable[Any]] = None,
         kwargs: Optional[Dict[Text, Any]] = None,
         ignore_context: bool = False,
@@ -73,7 +74,7 @@ def __init__(
         super().__init__(**left_over_kwargs)
         if not asyncio.iscoroutinefunction(coroutine):
             raise TypeError(
-                "OpaqueCoroutine expected a coroutine for 'coroutine', got '{}'".format(
+                "OpaqueCoroutine expected a coroutine function for 'coroutine', got '{}'".format(
                     type(coroutine)
                 )
             )
@@ -92,7 +93,7 @@ def __init__(
         self.__ignore_context = ignore_context  # type: bool
         self.__future = None  # type: Optional[asyncio.Future]
 
-    def __on_shutdown(self, event: Event, context: LaunchContext) -> Optional[SomeActionsType]:
+    def __on_shutdown(self, event: Event, context: LaunchContext) -> Optional[SomeEntitiesType]:
         """Cancel ongoing coroutine upon shutdown."""
         if self.__future is not None:
             self.__future.cancel()

launch/launch/actions/register_event_handler.py

@@ -23,6 +23,7 @@
 from ..event_handler import BaseEventHandler
 from ..launch_context import LaunchContext
 from ..launch_description_entity import LaunchDescriptionEntity
+from ..utilities import normalize_to_list_of_entities
 
 
 class RegisterEventHandler(Action):
@@ -57,6 +58,8 @@ def describe_conditional_sub_entities(self) -> List[Tuple[
         Iterable[LaunchDescriptionEntity],  # list of conditional sub-entities
     ]]:
         event_handler_description = self.__event_handler.describe()
+
         return [
-            (event_handler_description[0], event_handler_description[1])
+            (event_handler_description[0],
+                normalize_to_list_of_entities(event_handler_description[1]))
         ] if event_handler_description[1] else []

launch/launch/actions/timer_action.py

@@ -40,7 +40,7 @@
 from ..frontend import Parser
 from ..launch_context import LaunchContext
 from ..launch_description_entity import LaunchDescriptionEntity
-from ..some_actions_type import SomeActionsType
+from ..some_entities_type import SomeEntitiesType
 from ..some_substitutions_type import SomeSubstitutionsType
 from ..some_substitutions_type import SomeSubstitutionsType_types_tuple
 from ..utilities import create_future
@@ -137,7 +137,7 @@ def describe_conditional_sub_entities(self) -> List[Tuple[
         """Return the actions that will result when the timer expires, but was not canceled."""
         return [('{} seconds pass without being canceled'.format(self.__period), self.__actions)]
 
-    def handle(self, context: LaunchContext) -> Optional[SomeActionsType]:
+    def handle(self, context: LaunchContext) -> Optional[SomeEntitiesType]:
         """Handle firing of timer."""
         context.extend_locals(self.__context_locals)
         return self.__actions
@@ -157,7 +157,7 @@ def cancel(self) -> None:
             self._canceled_future.set_result(True)
         return None
 
-    def execute(self, context: LaunchContext) -> Optional[List['Action']]:
+    def execute(self, context: LaunchContext) -> Optional[List[LaunchDescriptionEntity]]:
         """
         Execute the action.