Skip to content
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

Native python types #8559

Closed
wants to merge 15 commits into from
8 changes: 8 additions & 0 deletions src/inmanta/ast/type.py
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,9 @@ def get_location(self) -> None:
return None


def __eq__(self, other):
return type(self) == type(other)

@stable_api
class List(Type):
"""
Expand Down Expand Up @@ -544,6 +547,11 @@ def type_string_internal(self) -> str:
def get_location(self) -> None:
return None

def __eq__(self, other: object) -> bool:
if not isinstance(other, TypedDict):
return NotImplemented
return self.element_type == other.element_type


@stable_api
class LiteralDict(TypedDict):
Expand Down
94 changes: 87 additions & 7 deletions src/inmanta/plugins.py
sanderr marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,16 @@
import asyncio
import collections.abc
import inspect
import numbers
import os
import subprocess
import typing
import warnings
from collections import abc
from typing import TYPE_CHECKING, Any, Callable, Literal, Mapping, Optional, Sequence, Type, TypeVar
from typing import TYPE_CHECKING, Any, Callable, Literal, Mapping, Optional, Sequence, Type, TypeVar, Dict

import inmanta.ast.type as inmanta_type
import typing_inspect
from inmanta import const, protocol, util
from inmanta.ast import ( # noqa: F401 Plugin exception is part of the stable api
LocatableString,
Expand All @@ -35,7 +38,7 @@
Range,
RuntimeException,
TypeNotFoundException,
WithComment,
WithComment, TypingException,
)
from inmanta.ast.type import NamedType
from inmanta.config import Config
Expand Down Expand Up @@ -210,6 +213,9 @@ def type_string(self) -> str:
def type_string_internal(self) -> str:
return self.type_string()

def __eq__(self, other):
return type(self) == type(other)


# Define some types which are used in the context of plugins.
PLUGIN_TYPES = {
Expand All @@ -219,6 +225,78 @@ def type_string_internal(self) -> str:
None: Null(), # Only NoneValue will pass validation
}

python_to_model = {
str: inmanta_type.String(),
float: inmanta_type.Float(),
numbers.Number: inmanta_type.Number(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to support this, given that the number type has been deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecated, but not gone.

I have no strong opinion on this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After giving it some more thought, me neither. At some level I'd prefer not to integrate deprecated types into new features, but if it's as simple as this I guess it doesn't hurt too much either. The only thing is perhaps that we have the opportunity to make a clean cut here, to be more visible than the current deprecation warning.

int: inmanta_type.Integer(),
bool: inmanta_type.Bool(),
dict: inmanta_type.LiteralDict(),
list: inmanta_type.LiteralList(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure about hardcoding this mapping to the type objects. I think I'd prefer to use the existing inmanta.ast.type.TYPES mapping, so that if we ever change something there it is reflected in both flows. Then this mapping here would simply be a mapping to the model types as represented in the model, e.g. str -> "string".

Copy link
Contributor Author

@wouterdb wouterdb Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon closer consideration, what I had here was even wrong.

inmanta.ast.type.TYPES is the internal attribute types, which are subtly different from what is here, mapping it twice won't be correct / consistent with what existed already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following with this. From your comment I understood that you agreed we should use inmanta.ast.type.TYPES, but from the implementation I think I misunderstood? Why should we not use it?

Copy link
Contributor Author

@wouterdb wouterdb Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should use inmanta.ast.type.TYPES because:

  1. I consider the type object the interface, so we should decouple on that
  2. inmanta.ast.type.TYPES maps attribute type string to inmanta types, but this is subtly different from the type mapping at the plugin boundary. E.g. "dict": LiteralDict(), Vs dict: inmanta_type.TypedDict(inmanta_type.Type()), And this is a pre-existing condition

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, clear!

}


def to_dsl_type(python_type: type[object]) -> inmanta_type.Type:
sanderr marked this conversation as resolved.
Show resolved Hide resolved
sanderr marked this conversation as resolved.
Show resolved Hide resolved
"""
:param python_type: The evaluated python type as provided in the Python type annotation.
wouterdb marked this conversation as resolved.
Show resolved Hide resolved
"""
# Any to any
if python_type is typing.Any:
return inmanta_type.AnyType()
sanderr marked this conversation as resolved.
Show resolved Hide resolved

if python_type is type(None):
sanderr marked this conversation as resolved.
Show resolved Hide resolved
return Null()

if typing_inspect.is_union_type(python_type):
if any(typing_inspect.is_optional_type(tt) for tt in typing.get_args(python_type)):
# Optional type
other_types = [tt for tt in typing.get_args(python_type) if not typing_inspect.is_optional_type(tt)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>>> typing_inspect.is_optional_type(int | None)
True

So I think we have to be more careful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? That seems entirely as expected?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you call it on tt (not python_type), and filter them out if they are optional. So suppose you'd have python_type=Union[int | Optional[str]] (admittedly a convoluted expression but still), wouldn't this evolve to Union[int, None], dropping the str because it is "optional"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you write it all without quotes, Python probably collapses them together so you can't get in this scenario. But then we have to be very careful about this case when we add support for quoted types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even python_type=typing.Union[int, typing.Annotated[typing.Union[str, None], "myannotation"]]. Python can not collapse it there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then again, typing_inspect.is_optional_type doesn't seem to work at all on typing.Annotated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Union[int | Optional[str]] == Union[int | str | None]

Optionals always flatten

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plain ones do, but not if something non-trivial pops in there. If we plan to never support strings, that's one thing covered. That only leaves Annotated then, but that might not be a concern right now.

if len(other_types) == 0:
# Probably not possible
return Null()
if len(other_types) == 1:
return inmanta_type.NullableType(to_dsl_type(other_types[0]))
# TODO: optional unions
return inmanta_type.AnyType()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we create a follow-up ticket to fix all these any types by 8.1? They could cause some very unintuitive behavior I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then again, aren't these relatively easy to handle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will ticketize and we'll see when we get them.

(e.g. the unions could be easy, but then again, we don't use unions anywhere yet, so they could also be very difficult)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you mark the tickets for the 8.1 release? I don't want to keep all these Any's for longer than required, so I'd like to address it (on iso8) while we can still consider it a non-breaking change (because it's in there now but not documented).

else:
return inmanta_type.AnyType()

# TODO: unions
# bases: Sequence[inmanta.ast.type.Type] = [to_dsl_type(arg) for arg in typing.get_args(python_type)]
# return inmanta.ast.type.Union(bases)

if typing_inspect.is_generic_type(python_type):
origin = typing.get_origin(python_type)
if origin is Sequence:
sanderr marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the design document: "Collection and Sequence as return types will require minor changes to DynamicProxy.unwrap, which now contains isinstance(item, list)" The same might go for Mapping, I haven't checked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still missing.

# Can this fail?
wouterdb marked this conversation as resolved.
Show resolved Hide resolved
base: inmanta.ast.type.Type = typing.get_args(python_type)[0]
return inmanta.ast.type.TypedList(to_dsl_type(base))

if issubclass(origin, dict) or origin is collections.abc.Mapping:
args = typing_inspect.get_args(python_type)
assert len(args) == 2
sanderr marked this conversation as resolved.
Show resolved Hide resolved
if not issubclass(args[0], str):
raise TypingException(None, f"invalid type {python_type}, the keys of any dict should be 'str', got {args[0]} instead")
return inmanta_type.TypedDict(to_dsl_type(args[1]))


# TODO annotated types
# if typing.get_origin(t) is typing.Annotated:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one should also be easy enough to support for 7.4

# args: Sequence[object] = typing.get_args(python_type)
# inmanta_types: Sequence[plugin_typing.InmantaType] = [arg if isinstance(arg, plugin_typing.InmantaType) for arg in args]
# if inmanta_types:
# if len(inmanta_types) > 1:
# # TODO
# raise Exception()
# # TODO
# return parse_dsl_type(inmanta_types[0].dsl_type)
# # the annotation doesn't concern us => use base type
# return to_dsl_type(args[0])
if python_type in python_to_model:
return python_to_model[python_type]

return inmanta_type.AnyType()


class PluginValue:
"""
Expand Down Expand Up @@ -267,11 +345,13 @@ def resolve_type(self, plugin: "Plugin", resolver: Namespace) -> inmanta_type.Ty
return self._resolved_type

if not isinstance(self.type_expression, str):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so for iso 8.1 we also aim to evaluate strings here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had understood that every string is considered to be an inmanta type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer to also support python type strings, because that's what you would expect when writing Python. But that doesn't have to be now necessarily. What I had in mind was to

  1. try to evaluate the annotation as a python type expression (converting any strings, even nested ones, to types)
  2. convert python type expression to dsl type expression

If 1 fails -> fall back to dsl type parsing.

But I don't feel too strongly about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want that. That would create the same type of problem that yaml has (is 00:00:00:00 a number?), that parsing outcome depends on context you (often) can't know. if a type 'string' is somehow in scope, the typing changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a key element in my reasoning is that I approach it from the idea that the pure dsl annotations would eventually disappear, leaving only the typing.Annotated when you really need them. With that reasoning, the Python parsing would always be the primary, and the DSL parsing would become only a backwards compatibility fallback.

But if that's not how you see it evolve, then perhaps I agree with you, given your argument above. I still think it can have unintuitive aspects of its own, but the typing.Annotated should give us a way to work around any we encounter to give ourselves time to discover where we really want to go.

raise RuntimeException(
stmt=None,
msg="Bad annotation in plugin %s for %s, expected str but got %s (%s)"
% (plugin.get_full_name(), self.VALUE_NAME, type(self.type_expression).__name__, self.type_expression),
)
if not isinstance(python_type, type) and typing.get_origin(python_type) is None:
raise RuntimeException(
stmt=None,
msg="Bad annotation in plugin %s for %s, expected str or python type but got %s (%s)"
% (plugin.get_full_name(), self.VALUE_NAME, type(self.type_expression).__name__, self.type_expression),
)
self._resolved_type = to_dsl_type(self.type_expression)

plugin_line: Range = Range(plugin.location.file, plugin.location.lnr, 1, plugin.location.lnr + 1, 1)
locatable_type: LocatableString = LocatableString(self.type_expression, plugin_line, 0, resolver)
Expand Down
25 changes: 25 additions & 0 deletions tests/compiler/test_plugin_types.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from typing import Mapping, Any, Union

import pytest
from inmanta.ast import RuntimeException
from inmanta.plugins import to_dsl_type, Null
import inmanta.ast.type as inmanta_type
import collections.abc

def test_conversion():
assert inmanta_type.Integer() == to_dsl_type(int)
assert inmanta_type.Float() == to_dsl_type(float)
assert inmanta_type.NullableType(inmanta_type.Float()) == to_dsl_type(float | None)
assert inmanta_type.LiteralList() == to_dsl_type(list)
assert inmanta_type.TypedDict(inmanta_type.String()) == to_dsl_type(dict[str, str])
assert inmanta_type.TypedDict(inmanta_type.String()) == to_dsl_type(Mapping[str, str])
assert inmanta_type.TypedDict(inmanta_type.String()) == to_dsl_type(collections.abc.Mapping[str, str])

assert Null() == to_dsl_type(Union[None] )

assert isinstance(to_dsl_type(Any), inmanta_type.AnyType)


with pytest.raises(RuntimeException):
to_dsl_type(dict[int, int])