Skip to content

Commit

Permalink
convert usages of TypeConstraint to TypeId for rule products in the e…
Browse files Browse the repository at this point in the history 
…ngine (#7114)

### Problem

See #4535 and #4005, in particular [this comment on #4535](#4535 (comment)). `TypeConstraint` is a pretty general construct that we would like to do more with, for example #6936, and as of [the current discussion in #4535](#4535 (comment)) we realize we can emulate union types in `@rule`s without it, matching just against a specific type.

### Solution

- Convert `output_constraint` in the `Rule` subclasses in `rules.py` into `output_type`, and add a `SubclassesOf(type)` type check in `datatype` fields in those classes to ensure this.
- Remove `satisfied_by()` and `satisfied_by_type()` externs, and add a `product_type()` extern used to intern a python `type` as a `TypeId`.
- Convert all `TypeConstraint`s passed to the engine for intrinsics (e.g. `Snapshot`) into `TypeId`s.
- Match whether a rule's result matches its declared output type by simply using `==`!
- Manually implement `fmt::Debug` for `TypeId` to be the same as `Display` (we may want to do these differently in the future, but it is very useful to see the actual type name when debugging).

### Result

Everything is the same, but now we don't have the additional complexity of `TypeConstraint` down in the engine when we can do simple `TypeId` equality. This lets us get more creative with `TypeConstraint` on the python side, while type checking on the rust side is a little less complex (and probably more efficient to some degree).
  • Loading branch information
@cosmicexplorer
cosmicexplorer authored and Stu Hood committed Mar 6, 2019
1 parent 304691b commit 27a13d4
Show file tree
Hide file tree
Showing 19 changed files with 428 additions and 541 deletions.
123 changes: 64 additions & 59 deletions src/python/pants/engine/native.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from future.utils import PY2, binary_type, text_type
from twitter.common.collections.orderedset import OrderedSet

from pants.engine.selectors import Get, constraint_for
from pants.engine.selectors import Get
from pants.util.contextutil import temporary_dir
from pants.util.dirutil import read_file, safe_mkdir, safe_mkdtemp
from pants.util.memo import memoized_classproperty, memoized_property
Expand Down Expand Up @@ -276,9 +276,33 @@ def extern_log(self, context_handle, level, msg_ptr, msg_len):
msg = bytes(self._ffi.buffer(msg_ptr, msg_len)).decode('utf-8')
logger.log(level, msg)

@_extern_decl('TypeId', ['ExternContext*', 'Handle*'])
def extern_product_type(self, context_handle, val):
"""Return a TypeId for the given Handle, which must be a `type`."""
c = self._ffi.from_handle(context_handle)
obj = self._ffi.from_handle(val[0])
# TODO: determine if this assertion has any performance implications.
assert isinstance(obj, type)
tid = c.to_id(obj)
return TypeId(tid)

@_extern_decl('TypeId', ['ExternContext*', 'Handle*'])
def extern_get_type_for(self, context_handle, val):
"""Return a representation of the object's type."""
c = self._ffi.from_handle(context_handle)
obj = self._ffi.from_handle(val[0])
type_id = c.to_id(type(obj))
return TypeId(type_id)

@_extern_decl('Ident', ['ExternContext*', 'Handle*'])
def extern_identify(self, context_handle, val):
"""Return an Ident containing the __hash__ and TypeId for the given Handle."""
"""Return a representation of the object's identity, including a hash and TypeId.
`extern_get_type_for()` also returns a TypeId, but doesn't hash the object -- this allows that
method to be used on unhashable objects. `extern_identify()` returns a TypeId as well to avoid
having to make two separate Python calls when interning a Python object in interning.rs, which
requires both the hash and type.
"""
c = self._ffi.from_handle(context_handle)
obj = self._ffi.from_handle(val[0])
hash_ = hash(obj)
Expand Down Expand Up @@ -318,19 +342,6 @@ def extern_val_to_str(self, context_handle, val):
v_str = '' if v is None else text_type(v)
return c.utf8_buf(v_str)

@_extern_decl('_Bool', ['ExternContext*', 'Handle*', 'Handle*'])
def extern_satisfied_by(self, context_handle, constraint_val, val):
"""Given a TypeConstraint and a Handle return constraint.satisfied_by(value)."""
constraint = self._ffi.from_handle(constraint_val[0])
return constraint.satisfied_by(self._ffi.from_handle(val[0]))

@_extern_decl('_Bool', ['ExternContext*', 'Handle*', 'TypeId*'])
def extern_satisfied_by_type(self, context_handle, constraint_val, cls_id):
"""Given a TypeConstraint and a TypeId, return constraint.satisfied_by_type(type_id)."""
c = self._ffi.from_handle(context_handle)
constraint = self._ffi.from_handle(constraint_val[0])
return constraint.satisfied_by_type(c.from_id(cls_id.tup_0))

@_extern_decl('Handle', ['ExternContext*', 'Handle**', 'uint64_t'])
def extern_store_tuple(self, context_handle, vals_ptr, vals_len):
"""Given storage and an array of Handles, return a new Handle to represent the list."""
Expand Down Expand Up @@ -423,12 +434,12 @@ def extern_generator_send(self, context_handle, func, arg):
if isinstance(res, Get):
# Get.
values = [res.subject]
products = [constraint_for(res.product)]
products = [res.product]
tag = 2
elif type(res) in (tuple, list):
# GetMulti.
values = [g.subject for g in res]
products = [constraint_for(g.product) for g in res]
products = [g.product for g in res]
tag = 3
else:
# Break.
Expand All @@ -446,7 +457,7 @@ def extern_generator_send(self, context_handle, func, arg):
return (
tag,
c.vals_buf([c.to_value(v) for v in values]),
c.vals_buf([c.to_value(v) for v in products]),
c.vals_buf([c.to_value(v) for v in products])
)

@_extern_decl('PyResult', ['ExternContext*', 'Handle*', 'Handle**', 'uint64_t'])
Expand All @@ -472,10 +483,6 @@ class Function(datatype(['key'])):
"""Corresponds to the native object of the same name."""


class TypeConstraint(datatype(['key'])):
"""Corresponds to the native object of the same name."""


class TypeId(datatype(['tup_0'])):
"""Corresponds to the native object of the same name."""

Expand Down Expand Up @@ -626,14 +633,14 @@ def init_externs():
self.ffi_lib.extern_call,
self.ffi_lib.extern_generator_send,
self.ffi_lib.extern_eval,
self.ffi_lib.extern_product_type,
self.ffi_lib.extern_get_type_for,
self.ffi_lib.extern_identify,
self.ffi_lib.extern_equals,
self.ffi_lib.extern_clone_val,
self.ffi_lib.extern_drop_handles,
self.ffi_lib.extern_type_to_str,
self.ffi_lib.extern_val_to_str,
self.ffi_lib.extern_satisfied_by,
self.ffi_lib.extern_satisfied_by_type,
self.ffi_lib.extern_store_tuple,
self.ffi_lib.extern_store_set,
self.ffi_lib.extern_store_dict,
Expand All @@ -644,8 +651,7 @@ def init_externs():
self.ffi_lib.extern_store_bool,
self.ffi_lib.extern_project_ignoring_type,
self.ffi_lib.extern_project_multi,
self.ffi_lib.extern_create_exception,
TypeId(context.to_id(str)))
self.ffi_lib.extern_create_exception)
return context

return self.ffi.init_once(init_externs, 'ExternContext singleton')
Expand Down Expand Up @@ -704,25 +710,25 @@ def new_scheduler(self,
construct_file_content,
construct_files_content,
construct_process_result,
constraint_address,
constraint_path_globs,
constraint_directory_digest,
constraint_snapshot,
constraint_merge_snapshots_request,
constraint_files_content,
constraint_dir,
constraint_file,
constraint_link,
constraint_process_request,
constraint_process_result,
constraint_generator,
constraint_url_to_fetch):
type_address,
type_path_globs,
type_directory_digest,
type_snapshot,
type_merge_snapshots_request,
type_files_content,
type_dir,
type_file,
type_link,
type_process_request,
type_process_result,
type_generator,
type_url_to_fetch):
"""Create and return an ExternContext and native Scheduler."""

def func(constraint):
return Function(self.context.to_key(constraint))
def tc(constraint):
return TypeConstraint(self.context.to_key(constraint))
def func(fn):
return Function(self.context.to_key(fn))
def ti(type_obj):
return TypeId(self.context.to_id(type_obj))

scheduler = self.lib.scheduler_create(
tasks,
Expand All @@ -732,23 +738,22 @@ def tc(constraint):
func(construct_file_content),
func(construct_files_content),
func(construct_process_result),
# TypeConstraints.
tc(constraint_address),
tc(constraint_path_globs),
tc(constraint_directory_digest),
tc(constraint_snapshot),
tc(constraint_merge_snapshots_request),
tc(constraint_files_content),
tc(constraint_dir),
tc(constraint_file),
tc(constraint_link),
tc(constraint_process_request),
tc(constraint_process_result),
tc(constraint_generator),
tc(constraint_url_to_fetch),
# Types.
TypeId(self.context.to_id(text_type)),
TypeId(self.context.to_id(binary_type)),
ti(type_address),
ti(type_path_globs),
ti(type_directory_digest),
ti(type_snapshot),
ti(type_merge_snapshots_request),
ti(type_files_content),
ti(type_dir),
ti(type_file),
ti(type_link),
ti(type_process_request),
ti(type_process_result),
ti(type_generator),
ti(type_url_to_fetch),
ti(text_type),
ti(binary_type),
# Project tree.
self.context.utf8_buf(build_root),
self.context.utf8_buf(work_dir),
Expand Down
75 changes: 21 additions & 54 deletions src/python/pants/engine/rules.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,20 @@
from future.utils import PY2
from twitter.common.collections import OrderedSet

from pants.engine.selectors import Get, type_or_constraint_repr
from pants.engine.selectors import Get
from pants.util.collections import assert_single_element
from pants.util.collections_abc_backport import Iterable, OrderedDict
from pants.util.memo import memoized
from pants.util.meta import AbstractClass
from pants.util.objects import Exactly, datatype
from pants.util.objects import SubclassesOf, datatype


logger = logging.getLogger(__name__)


_type_field = SubclassesOf(type)


class _RuleVisitor(ast.NodeVisitor):
"""Pull `Get` calls out of an @rule body and validate `yield` statements."""

Expand Down Expand Up @@ -249,17 +252,14 @@ def wrapper(func):

def resolve_type(name):
resolved = caller_frame.f_globals.get(name) or caller_frame.f_builtins.get(name)
if not isinstance(resolved, (type, Exactly)):
# TODO: should this say "...or Exactly instance;"?
raise ValueError('Expected either a `type` constructor or TypeConstraint instance; '
'got: {}'.format(name))
if not isinstance(resolved, type):
raise ValueError('Expected a `type` constructor, but got: {}'.format(name))
return resolved

gets = OrderedSet()
rule_func_node = assert_single_element(
node for node in ast.iter_child_nodes(module_ast)
if isinstance(node, ast.FunctionDef) and node.name == func.__name__
)
if isinstance(node, ast.FunctionDef) and node.name == func.__name__)

parents_table = {}
for parent in ast.walk(rule_func_node):
Expand Down Expand Up @@ -300,7 +300,7 @@ def goal_and_return(*args, **kwargs):
wrapped_func,
input_gets=tuple(gets),
goal=for_goal,
cacheable=cacheable
cacheable=cacheable,
)

return wrapped_func
Expand Down Expand Up @@ -368,16 +368,17 @@ class Rule(AbstractClass):
"""

@abstractproperty
def output_constraint(self):
"""An output Constraint type for the rule."""
def output_type(self):
"""An output `type` for the rule."""

@abstractproperty
@property
def dependency_optionables(self):
"""A tuple of Optionable classes that are known to be necessary to run this rule."""
return ()


class TaskRule(datatype([
'output_constraint',
('output_type', _type_field),
('input_selectors', tuple),
('input_gets', tuple),
'func',
Expand All @@ -400,18 +401,10 @@ def __new__(cls,
goal=None,
dependency_optionables=None,
cacheable=True):
# Validate result type.
if isinstance(output_type, Exactly):
constraint = output_type
elif isinstance(output_type, type):
constraint = Exactly(output_type)
else:
raise TypeError("Expected an output_type for rule `{}`, got: {}".format(
func.__name__, output_type))

return super(TaskRule, cls).__new__(
cls,
constraint,
output_type,
input_selectors,
input_gets,
func,
Expand All @@ -422,53 +415,31 @@ def __new__(cls,

def __str__(self):
return ('({}, {!r}, {}, gets={}, opts={})'
.format(type_or_constraint_repr(self.output_constraint),
.format(self.output_type.__name__,
self.input_selectors,
self.func.__name__,
self.input_gets,
self.dependency_optionables))


class SingletonRule(datatype(['output_constraint', 'value']), Rule):
class SingletonRule(datatype([('output_type', _type_field), 'value']), Rule):
"""A default rule for a product, which is thus a singleton for that product."""

@classmethod
def from_instance(cls, obj):
return cls(type(obj), obj)

def __new__(cls, output_type, value):
# Validate result type.
if isinstance(output_type, Exactly):
constraint = output_type
elif isinstance(output_type, type):
constraint = Exactly(output_type)
else:
raise TypeError("Expected an output_type for rule; got: {}".format(output_type))

# Create.
return super(SingletonRule, cls).__new__(cls, constraint, value)

@property
def dependency_optionables(self):
return tuple()

def __repr__(self):
return '{}({}, {})'.format(type(self).__name__, type_or_constraint_repr(self.output_constraint), self.value)


class RootRule(datatype(['output_constraint']), Rule):
class RootRule(datatype([('output_type', _type_field)]), Rule):
"""Represents a root input to an execution of a rule graph.
Roots act roughly like parameters, in that in some cases the only source of a
particular type might be when a value is provided as a root subject at the beginning
of an execution.
"""

@property
def dependency_optionables(self):
return tuple()


# TODO: add typechecking here -- would need to have a TypedCollection for dicts for `union_rules`.
class RuleIndex(datatype(['rules', 'roots', 'union_rules'])):
"""Holds a normalized index of Rules used to instantiate Nodes."""

Expand All @@ -480,6 +451,7 @@ def create(cls, rule_entries, union_rules=None):
union_rules = OrderedDict(union_rules or ())

def add_task(product_type, rule):
# TODO(#7311): make a defaultdict-like wrapper for OrderedDict if more widely used.
if product_type not in serializable_rules:
serializable_rules[product_type] = OrderedSet()
serializable_rules[product_type].add(rule)
Expand All @@ -491,12 +463,7 @@ def add_rule(rule):
if isinstance(rule, RootRule):
add_root_rule(rule)
else:
# TODO: Ensure that interior types work by indexing on the list of types in
# the constraint. This heterogenity has some confusing implications:
# see https://github.com/pantsbuild/pants/issues/4005
for kind in rule.output_constraint.types:
add_task(kind, rule)
add_task(rule.output_constraint, rule)
add_task(rule.output_type, rule)

def add_type_transition_rule(union_rule):
# NB: This does not require that union bases be supplied to `def rules():`, as the union type
Expand Down
Loading

0 comments on commit 27a13d4

Please sign in to comment.