Found some unreachable code while adding tests

[samuel-denton]

I've found some unreachable code while improving our test coverage. I will put examples here as I find them to remove in a followup PR.

[samuel-denton]

cylc/flow/exceptions.py lines 103 to 108 defines a CylcError class called TaskProxySequenceBoundsError. this error is never raised:

It looks like the one instance of it being raised was removed in this commit: edf61d3
Used to be on line 199 of cylc/flow/task_proxy.py

So we can remove the class definition, as well as the block which catches it (and the import).

[samuel-denton]

Not technically unreachable code, but an unnecessary else check here:

cylc/flow/task_outputs.py line 633

    def _is_compvar_complete(self, compvar: str) -> Optional[bool]:
        for message, _compvar in self._message_to_compvar.items():
            if _compvar == compvar:
                return self.is_message_complete(message)
        else:
            raise KeyError(compvar)

Since the early stop condition is a return, the else is redundant. If the condition is met at any time during the loop, the function exits, if the condition is never met, the rest of the function is walked and the raise would be triggered regardless. If the return was a break instead, the else would then be needed. Can be simplified to:

    def _is_compvar_complete(self, compvar: str) -> Optional[bool]:
        for message, _compvar in self._message_to_compvar.items():
            if _compvar == compvar:
                return self.is_message_complete(message)
        raise KeyError(compvar)

[samuel-denton]

I believe this raise line is unreachable, in cylc/flow/time_parser.py on lines 201 to 203:

    def parse_timepoint(
        self, expr: str, context_point: Optional['TimePoint'] = None
    ) -> 'TimePoint':
        if context_point is None:
            context_point = self.context_start_point
        point, offsets = self._get_point_from_expression(
            expr, context_point, allow_truncated=True
        )
        if point is not None:
            if point.truncated:
                point += context_point  # type: ignore[operator]
            for offset in offsets:
                point += offset
            return point
        raise CylcTimeSyntaxError(
            ("'%s': not a valid cylc-shorthand or full " % expr) +
            "ISO 8601 date representation")

It appears fine, but examining all possible branches within _get_point_from_expression(), it can never return a None value for point. It raises it's own errors in any case that could do so. It could be left as a guard for unexpected behaviour or changes to _get_point_from_expression() but at the moment I don't think it does anything.

EDIT: I was wrong there is a scenario where it can return None and this error raised. I have added a new test for it instead.

[oliver-sanders]

_get_point_from_expression(), it can never return a None value for point

Not sure I follow, there's one branch of the code which returns None:

cylc-flow/cylc/flow/time_parser.py

Line 415 in 552e28f

return None, []

[samuel-denton]

_get_point_from_expression(), it can never return a None value for point

Not sure I follow, there's one branch of the code which returns None:

Yep that's my bad, if the expression is None and context is given, it can return None for the point. For some reason I thought the: if context is None: clause was always going to catch this scenario and throw an error but it's possible for there to be a None expression with a valid context.

I have added a test for this scenario.