[WIP] Add some typing hints for module_utils

[felixfontein]

SUMMARY

Adds some typing hints for module_utils.

I was originally providing more, but didn't manage to get all of them into a good shape. For example, strict_optional = False prevents proper overloads for AnsibleModule.run_command() since the return types depend on whether encoding is None or a string; with strict_optional = False mypy always complains that the overloads do not differ since str includes None.

Not sure how to properly classify this (bugfix, feature, ...), for that reason I also didn't add a changelog fragment yet.

Right now the PR contains #85259, for that reason I've marked it WIP.

ISSUE TYPE

• Refactoring Pull Request

[webknjaz]

Aren't these inferred? This is only useful if you add an item type.

[felixfontein]

mypy complains about them once it checks the constructor 🤷 I guess it would prefer to have list[xxx] for some xxx, but can't figure out what xxx is.

[webknjaz]

Oh, right, it can't infer element types from empty containers.
Since it seems to be unused, why not make it always-empty in typing?

Suggested change

	self._legal_inputs: list = [] # no longer used?
	self._options_context: list = list() # no longer used?
	self._legal_inputs: list[t.Never] = [] # no longer used?
	self._options_context: list[t.Never] = [] # no longer used?

[felixfontein]

t.Never cannot be used in module utils.

[webknjaz]

I don't see any place in the module that treats this like a dict. The default is an abomination… This could probably be

Suggested change

	self._clean: dict | str | None = {}
	self._clean: str | None = None

instead.

Alternatively, we could allow an empty dict only:

Suggested change

	self._clean: dict | str | None = {}
	self._clean: dict[t.Never, t.Never] | str | None = {}

[felixfontein]

I think the empty dict is the better choice for now. I'd prefer if this PR avoids behavior changes when not strictly necessary to pass type checking.

[webknjaz]

So use the second suggestion, only allowing it to be empty on the typing level.

[felixfontein]

t.Never is only supported for Python 3.11+. module utils typing must pass with Python versions from 3.9 to 3.13. So the second suggestion will not work.

[webknjaz]

Older versions represent the same as typing.NoReturn. So this should work then:

Suggested change

	self._clean: dict | str | None = {}
	self._clean: dict[t.NoReturn, t.NoReturn] | str | None = {}

Alternatively, we could check if typing-extensions has it.

[webknjaz]

I think this should be something like

Suggested change

	self.cleanup_files: list[str | bytes | os.PathLike] = []
	self.cleanup_files: list[bytes] | list[str] | list[os.PathLike[bytes]] | list[os.PathLike[str]] = []

without a union type for the items.

[felixfontein]

Uhm, why? You can add all four different types during the same module run.

[webknjaz]

But probably shouldn't? So why not make MyPy encourage the end-users to use same types?

[felixfontein]

You cannot do that with Python typing. Which type would you add to AnsibleModule.add_cleanup_file()'s path parameter then?

[webknjaz]

add typing to these too?

[felixfontein]

I forgot about add_file_common_args, but I didn't want to add types for the others since they tend to be very complicated (you can use lists or tuples, and elements can again be lists and tuples). Also I didn't want endless discussions about these types to delay this PR.

[bcoca]

I've been toying with adding module_utils/common/typing and adding some 'custom complex types' for places were we take 'some iterables or string that can be , separated' and such, PATH is a big one as it can be unicode/bytes/file/Pathlib object/None
in many places

[felixfontein]

Suggested change

	mutually_exclusive=None, required_together=None,
	required_one_of=None, add_file_common_args=False,
	mutually_exclusive=None, required_together=None,
	required_one_of=None, add_file_common_args: bool = False,

[felixfontein]

I've been toying with adding module_utils/common/typing and adding some 'custom complex types' for places were we take 'some iterables or string that can be , separated' and such, PATH is a big one as it can be unicode/bytes/file/Pathlib object/None

I think that would be a very good idea!

[felixfontein]

BTW, one of the main motivations to start with this was adding typing to to_bytes and to_text, since these functions are pretty common and their return type is totally clear to humans, but not to mypy. AnsibleModule.run_command is another case where typing would be really helpful, but without being able to add proper overloads I guess typing there does more harm than it does good, since you need to add explicit casts to a million places as a result.

[bcoca]

to_text/to_bytes should be going away, they were created for py2/py3 compatibility but they also did 'weird' things like passthrough non text types, depending on options. At this point they should be obsolete and either removed or just use str instead ... depending on use case.

[webknjaz]

Should this accept other filename variants? There's no runtime checks for non-bytes, after all.

Suggested change

	def backup_local(self, fn: str | os.PathLike[str]) -> str:
	def backup_local(self, fn: bytes | str | os.PathLike[bytes] | os.PathLike[str]) -> str:

[felixfontein]

There isn't, but the filename of the backup will be totally screwed up if you pass bytes here:

backupdest = '%s.%s.%s' % (fn, os.getpid(), ext)

You'd a file named b'original_filename'.123.01-02-2023@01:02:03~.

[bcoca]

fstrings, then !r should fix it either way

[felixfontein]

That only makes the situation worse for str-based paths. If you want to fix it you need to use to_text/to_native, or use to_bytes and use bytes instead of str for backupdest.

[webknjaz]

Can these be turned into type aliases?

Suggested change

	_ErrorHandlers = _t.Literal[
	# Composed error handlers, see above:
	"surrogate_or_replace",
	"surrogate_or_strict",
	"surrogate_then_replace",
	# Error handlers from https://docs.python.org/3/library/codecs.html#codec-base-classes:
	"strict",
	"ignore",
	"replace",
	"backslashreplace",
	"surrogateescape",
	"xmlcharrefreplace",
	"namereplace",
	"surrogatepass",
	]
	def to_bytes(obj, encoding='utf-8', errors=None, nonstring='simplerepr'):
	_NonString = _t.Literal[
	"simplerepr",
	"empty",
	"passthru",
	"strict",
	]
	_NonStringNotPassthru = _t.Literal[
	"simplerepr",
	"empty",
	"strict",
	]
	_ErrorHandlers: _t.TypeAlias = _t.Literal[
	# Composed error handlers, see above:
	"surrogate_or_replace",
	"surrogate_or_strict",
	"surrogate_then_replace",
	# Error handlers from https://docs.python.org/3/library/codecs.html#codec-base-classes:
	"strict",
	"ignore",
	"replace",
	"backslashreplace",
	"surrogateescape",
	"xmlcharrefreplace",
	"namereplace",
	"surrogatepass",
	]
	_NonString: _t.TypeAlias = _t.Literal[
	"simplerepr",
	"empty",
	"passthru",
	"strict",
	]
	_NonStringNotPassthru: _t.TypeAlias = _t.Literal[
	"simplerepr",
	"empty",
	"strict",
	]

[felixfontein]

TypeAlias has been added in Python 3.10, so it won't work with Python 3.9.

(I don't really understand why type checking needs to work with every single Python version supported on the target anyway. After all there's from __future__ import annotations...)

[webknjaz]

It needs to work because people would be writing target code against those Python versions 🤷‍♂️

[webknjaz]

Get it from typing-extensions, then: https://typing-extensions.readthedocs.io/en/latest/#typing_extensions.TypeAlias. Perhaps, it's even better to consider NewType instead.

[webknjaz]

Let's use

Suggested change

	def jsonify(data: _t.Any, **kwargs) -> str:
	def jsonify(data: object, **kwargs: object) -> str:

if possible.

[felixfontein]

I think it's a bad idea to add types that are wrong. Acceptable keyword arguments like skipkeys, ensure_ascii, check_circular, ... accept values of specific types, and not of type object. I think in such situations it's better to not add types so places like this which need more work are easier to find.

[webknjaz]

It's best to use a TypedDict or better mechanisms for kwargs. But object is almost always better than Any. Everything in Python inherits from object. So it's always correct (unless attributes/interfaces that are present in more specific subtypes are accessed).

https://mypy.readthedocs.io/en/stable/dynamic_typing.html#any-vs-object

[webknjaz]

It looks like this is

Suggested change

	def container_to_bytes(d: _t.Any, encoding: str = 'utf-8', errors: _ErrorHandlers = 'surrogate_or_strict') -> _t.Any:
	def container_to_bytes(d: str | dict[bytes | str, bytes | str] | list[bytes | str] | tuple[bytes | str] | object, encoding: str = 'utf-8', errors: _ErrorHandlers = 'surrogate_or_strict') -> dict[bytes, bytes] | list[bytes] | tuple[bytes] | object:

[felixfontein]

Why? It also accepts all other types (and simply passes them through). Maybe it should be object instead of _t.Any, but I don't think it should be anything else. (Unless we have some specific type for JSON values.)

I missed the object.

But then, still: why? These expressions can be both simplified to object. So you'd have def container_to_bytes(d: object, ...) -> object:.

[webknjaz]

object isn't equvivalent to a union of more accurate types as those allow more behaviors than just object. I'm not entirely sure that it'd work. But you could try. Not sure about the return type, though. Returning object is very broad. The arg types should be broad but return types should be narrow.

[webknjaz]

Suggested change

	def container_to_text(d: _t.Any, encoding: str = 'utf-8', errors: _ErrorHandlers = 'surrogate_or_strict') -> _t.Any:
	def container_to_text(d: str | dict[bytes | str, bytes | str] | list[bytes | str] | tuple[bytes | str] | object, encoding: str = 'utf-8', errors: _ErrorHandlers = 'surrogate_or_strict') -> dict[str, str] | list[str] | tuple[str] | object:

[felixfontein]

Same as above - why not simply use object instead of this more complex expression?

(Besides that, the types listed there are random subsets of what the function accepts and produces. For example {1: [2.0, "foo"]} is acceptable input and output as well.)

[webknjaz]

For example {1: [2.0, "foo"]} is acceptable input and output as well.

Yes, I think this would require a recursive type definition that isn't inline. Most of my suggestions would benefit from this.