Refactor so builder context only stores Blocks (#432)

Having context be any Refable was overkill and didn't do anything except
for one clone check, This changes builder context to only store Blocks
(including Links), and has the context push happen in the init
metaclass.

Other changes:
- Renames ConstraintExpr.parent to ConstraintExpr._context, since parent
in Block and Port refer to a binding, whereas the Constraint binding is
stored in the Binding object.
- Changes the `__init__` hook to unconditionally pass through args and
kwargs, instead of looking for *args, **kwargs.
- Prevents builder double-pushing the same context.
- Ensure the dummy instantiation of part libraries are in lambdas,
otherwise they were being created on program start

Author: ducky64

edg/core/Blocks.py

@@ -407,35 +407,32 @@ def _bind_in_place(self, parent: Union[BaseBlock, Port]):
   def _bind(self: SelfType, parent: Union[BaseBlock, Port]) -> SelfType:
     """Returns a clone of this object with the specified binding. This object must be unbound."""
     assert self._parent is None, "can't clone bound block"
-    assert builder.get_curr_context() is self._lexical_parent, "can't clone to different context"
+    assert builder.get_enclosing_block() is self._block_context, "can't clone to different context"
     clone = type(self)(*self._initializer_args[0], **self._initializer_args[1])  # type: ignore
     clone._bind_in_place(parent)
     return clone
 
   def _check_constraint(self, constraint: ConstraintExpr) -> None:
     def check_subexpr(expr: Union[ConstraintExpr, BasePort]) -> None:  # TODO rewrite this whole method
       if isinstance(expr, ConstraintExpr) and isinstance(expr.binding, ParamBinding):
-        if isinstance(expr.parent, BaseBlock):
-          block_parent = expr.parent
-        elif isinstance(expr.parent, BasePort):
-          block_parent = cast(BaseBlock, expr.parent._block_parent())  # TODO make less ugly
-          assert block_parent is not None
-        else:
-          raise ValueError(f"unknown parent {expr.parent} of {expr}")
-
-        if isinstance(block_parent._parent, BasePort):
-          block_parent_parent: Any = block_parent._parent._block_parent()
-        else:
-          block_parent_parent = block_parent._parent
-
-        if not (block_parent is self or block_parent_parent is self):
-          raise UnreachableParameterError(f"In {type(self)}, constraint references unreachable parameter {expr}. "
+        expr_parent = expr.binding.parent
+        if isinstance(expr_parent, BasePort):
+          expr_block_parent = expr_parent._block_parent()
+          assert expr_block_parent is not None
+          expr_parent = expr_block_parent
+
+        expr_parent_parent = expr_parent._parent  # may be None for top-level
+        if isinstance(expr_parent_parent, BasePort):  # resolve Link parent port to parent block
+          expr_parent_parent = expr_parent_parent._block_parent()
+
+        if not (expr_parent is self or expr_parent_parent is self):
+          raise UnreachableParameterError(f"In {self}, constraint references unreachable parameter {expr}. "
                                           "Only own parameters, or immediate contained blocks' parameters can be accessed.")
       elif isinstance(expr, BasePort):
         block_parent = cast(BaseBlock, expr._block_parent())
         assert block_parent is not None
         if not block_parent is self or block_parent._parent is self:
-          raise UnreachableParameterError(f"In {type(self)}, constraint references unreachable port {expr}. "
+          raise UnreachableParameterError(f"In {self}, constraint references unreachable port {expr}. "
                                           "Only own ports, or immediate contained blocks' ports can be accessed.")
 
     for subexpr in constraint._get_exprs():

edg/core/Builder.py

@@ -2,42 +2,44 @@
 
 from typing import *
 
+from deprecated import deprecated
+
 from .. import edgir
 
 if TYPE_CHECKING:
-  from .Core import Refable
   from .Blocks import BaseBlock
 
 
 class Builder:
   def __init__(self) -> None:
-    self.stack: List[Refable] = []
+    self.stack: List[BaseBlock] = []
 
-  def push_element(self, elt: Refable) -> None:
-    self.stack.append(elt)
+  def push_element(self, elt: BaseBlock) -> Optional[BaseBlock]:
+    """Pushes a new element onto the context stack, returning the previous top element.
+    Ignores if the element is already on top of the stack."""
+    prev_elt = self.get_enclosing_block()
+    if not self.stack or self.stack[-1] is not elt:  # prevent double-pushing
+      self.stack.append(elt)
+    return prev_elt
 
-  def pop_to(self, elt: Optional[Refable]) -> None:
-    self.stack.pop()
-    assert self.get_curr_context() is elt
+  def pop_to(self, elt: Optional[BaseBlock]) -> None:
+    while (elt is None and self.stack) or (elt is not None and self.stack[-1] is not elt):
+      self.stack.pop()
 
   def get_enclosing_block(self) -> Optional[BaseBlock]:
-    from .Blocks import BaseBlock
-    # traverse down the stack and get the first BaseBlock, ignoring things like ports
-    for elt in reversed(self.stack):
-      if isinstance(elt, BaseBlock):
-        return elt
-    return None
-
-  def get_curr_context(self) -> Optional[Refable]:
     if not self.stack:
       return None
     else:
       return self.stack[-1]
 
+  @deprecated("use get_enclosing_block() instead, context frames can only be blocks now")
+  def get_curr_context(self) -> Optional[BaseBlock]:
+    return self.get_enclosing_block()
+
   def elaborate_toplevel(self, block: BaseBlock, *,
                          is_generator: bool = False,
                          generate_values: Iterable[Tuple[edgir.LocalPath, edgir.ValueLit]] = []) -> edgir.HierarchyBlock:
-    assert self.get_curr_context() is None
+    assert self.get_enclosing_block() is None
     self.push_element(block)
     try:
       if is_generator:  # TODO this is kind of nasty =(

edg/core/ConstraintExpr.py

@@ -17,6 +17,7 @@
 
 if TYPE_CHECKING:
   from .Ports import BasePort
+  from .Blocks import BaseBlock
 
 
 SelfType = TypeVar('SelfType', bound='ConstraintExpr')
@@ -64,7 +65,7 @@ def __init__(self: SelfType, initializer: Optional[Union[SelfType, WrappedType]]
       self.initializer = None
     else:
       self.initializer = self._to_expr_type(initializer)
-    self.parent = builder.get_curr_context()
+    self._context: Optional[BaseBlock] = builder.get_enclosing_block()
 
   def _get_exprs(self) -> Iterable[Union[ConstraintExpr, BasePort]]:
     assert self.binding is not None
@@ -80,7 +81,7 @@ def _new_bind(cls: Type[SelfType], binding: Binding) -> SelfType:
   def _bind(self: SelfType, binding: Binding) -> SelfType:
     """Returns a clone of this object with the specified binding. This object must be unbound."""
     assert not self._is_bound()
-    assert builder.get_curr_context() is self.parent, f"can't clone in original context {self.parent} to different new context {builder.get_curr_context()}"
+    assert builder.get_enclosing_block() is self._context, f"can't clone in original context {self._context} to different new context {builder.get_enclosing_block()}"
     if not isinstance(binding, ParamBinding):
       assert self.initializer is None, "Only Parameters may have initializers"
     clone: SelfType = type(self)(self.initializer)

edg/core/Core.py

@@ -146,17 +146,12 @@ class ElementMeta(type):
   """Hook on construction to store some metadata about its creation.
   This hooks the top-level __init__ only."""
   def __call__(cls, *args, **kwargs):
-    parent = builder.get_curr_context()
     block_context = builder.get_enclosing_block()
-    try:
-      obj = type.__call__(cls, *args, **kwargs)
-      obj._initializer_args = (args, kwargs)  # stores args so it is clone-able
-      obj._lexical_parent = parent  # stores context for error checking
-      obj._block_context = block_context
-      obj._post_init()
-    finally:
-      if builder.get_curr_context() is not parent:  # in case the constructor skipped internal element init
-        builder.pop_to(parent)
+
+    obj = type.__call__(cls, *args, **kwargs)
+    obj._initializer_args = (args, kwargs)  # stores args so it is clone-able
+    obj._block_context = block_context
+    obj._post_init()
 
     return obj
 
@@ -198,12 +193,10 @@ def __repr__(self) -> str:
     return "%s@%02x" % (self._get_def_name(), (id(self) // 4) & 0xff)
 
   def __init__(self) -> None:
-    self._lexical_parent: Optional[LibraryElement]  # set by metaclass
+    self._block_context: Optional["Refable"]  # set by metaclass, as lexical scope available pre-binding
     self._parent: Optional[LibraryElement] = None  # set by binding, None means not bound
     self._initializer_args: Tuple[Tuple[Any, ...], Dict[str, Any]]  # set by metaclass
 
-    builder.push_element(self)
-
     self.manager = SubElementManager()
     self.manager_ignored: Set[str] = set(['_parent'])
 

edg/core/HierarchyBlock.py

@@ -147,12 +147,11 @@ def __new__(cls, *args: Any, **kwargs: Any) -> Any:
       arg_data: List[Tuple[str, inspect.Parameter, Type[ConstraintExpr]]] = []
       # discard param 0 (self)
       for arg_name, arg_param in list(inspect.signature(orig_init).parameters.items())[1:]:
-        if arg_param.kind not in (inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.VAR_KEYWORD):
-          param_expr_type = BlockMeta._ANNOTATION_EXPR_MAP.get(arg_param.annotation, None)
-          if param_expr_type is None:
-            raise BlockDefinitionError(new_cls, f"in {new_cls}.__init__, unknown annotation type for {arg_name}: {arg_param.annotation}")
-        else:
-          param_expr_type = ConstraintExpr  # dummy placeholder
+        if arg_param.kind in (inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.VAR_KEYWORD):
+          continue  # pass through of *args, **kwargs handled later
+        param_expr_type = BlockMeta._ANNOTATION_EXPR_MAP.get(arg_param.annotation, None)
+        if param_expr_type is None:
+          raise BlockDefinitionError(new_cls, f"in {new_cls}.__init__, unknown annotation type for {arg_name}: {arg_param.annotation}")
 
         arg_data.append((arg_name, arg_param, param_expr_type))
 
@@ -182,21 +181,13 @@ def remap_arg(arg_name: str, arg_type: Type[ConstraintExpr], arg_value: Any) ->
 
           return arg_type()._bind(InitParamBinding(self, typed_arg_value))
 
-        # create wrapper ConstraintExpr in new object scope
-        builder_prev = builder.get_curr_context()
-        builder.push_element(self)
+        builder_prev = builder.push_element(self)
         try:
           # rebuild args and kwargs by traversing the args list
           new_args: List[Any] = []
           new_kwargs: Dict[str, Any] = {}
           for arg_pos, (arg_name, arg_param, param_expr_type) in enumerate(arg_data):
-            if arg_param.kind == inspect.Parameter.VAR_POSITIONAL:  # pass-through *args, handled at lower level
-              new_args.extend(args[arg_pos:])
-            elif arg_param.kind == inspect.Parameter.VAR_KEYWORD:  # pass-through *kwargs, handled at lower level
-              for arg_name in kwargs:
-                if arg_name not in new_kwargs:
-                  new_kwargs[arg_name] = kwargs[arg_name]
-            elif arg_pos < len(args) and arg_param.kind in (inspect.Parameter.POSITIONAL_ONLY,
+            if arg_pos < len(args) and arg_param.kind in (inspect.Parameter.POSITIONAL_ONLY,
                                                             inspect.Parameter.POSITIONAL_OR_KEYWORD):  # present positional arg
               new_arg = remap_arg(arg_name, param_expr_type, args[arg_pos])
               new_args.append(new_arg)
@@ -221,11 +212,17 @@ def remap_arg(arg_name: str, arg_type: Type[ConstraintExpr], arg_value: Any) ->
                 new_arg = remap_arg(arg_name, param_expr_type, None)
                 new_kwargs[arg_name] = new_arg
                 self._init_params[arg_name] = new_arg
+
+          # unconditionally pass through all args and kwargs
+          new_args.extend(args[len(new_args):])
+          for arg_name in kwargs:
+            if arg_name not in new_kwargs:
+              new_kwargs[arg_name] = kwargs[arg_name]
+
+          orig_init(self, *new_args, **new_kwargs)
         finally:
           builder.pop_to(builder_prev)
 
-        orig_init(self, *new_args, **new_kwargs)
-
       new_cls.__init__ = functools.update_wrapper(wrapped_init, orig_init)
 
     return new_cls

edg/core/Link.py

@@ -1,18 +1,38 @@
 from __future__ import annotations
 
+import functools
 from typing import *
 
 from .. import edgir
 from .Array import BaseVector, DerivedVector
 from .Blocks import BaseBlock, Connection
-from .Core import Refable, non_library
+from .Builder import builder
+from .Core import Refable, non_library, ElementMeta
 from .HdlUserExceptions import UnconnectableError
 from .IdentityDict import IdentityDict
 from .Ports import Port
 
 
+class LinkMeta(ElementMeta):
+  def __new__(cls, *args: Any, **kwargs: Any) -> Any:
+    new_cls = super().__new__(cls, *args, **kwargs)
+
+    if '__init__' in new_cls.__dict__:
+      orig_init = new_cls.__dict__['__init__']
+      def wrapped_init(self, *args, **kwargs) -> None:
+        builder_prev = builder.push_element(self)
+        try:
+          orig_init(self, *args, **kwargs)
+        finally:
+          builder.pop_to(builder_prev)
+
+      new_cls.__init__ = functools.update_wrapper(wrapped_init, orig_init)
+
+    return new_cls
+
+
 @non_library
-class Link(BaseBlock[edgir.Link]):
+class Link(BaseBlock[edgir.Link], metaclass=LinkMeta):
   def __init__(self) -> None:
     super().__init__()
     self.parent: Optional[Port] = None

edg/core/MultipackBlock.py

@@ -188,7 +188,7 @@ def packed_assign(self, self_param: ConstraintExpr, packed_param: PackedParamTyp
       raise BlockDefinitionError(self, "can only define multipack in init")
     if isinstance(packed_param, ConstraintExpr):
       assert type(self_param) == type(packed_param), "packed_assign parameters must be of the same type"
-      block_parent = packed_param.parent
+      block_parent = packed_param._context
       assert isinstance(block_parent, Block)
       self._packed_assigns_by_packed_block[block_parent][self_param] = packed_param
     elif isinstance(packed_param, PackedBlockParamArray):
@@ -234,7 +234,7 @@ def unpacked_assign(self, packed_param: UnpackedParamTypes, self_param: Constrai
       raise BlockDefinitionError(self, "can only define multipack in init")
     if isinstance(packed_param, ConstraintExpr):
       assert type(packed_param) == type(self_param), "unpacked_assign parameters must be of the same type"
-      block_parent = packed_param.parent
+      block_parent = packed_param._context
       assert isinstance(block_parent, Block)
       self._unpacked_assigns_by_packed_block[block_parent][self_param] = packed_param
     elif isinstance(packed_param, PackedBlockParam):

edg/core/Ports.py

@@ -26,7 +26,6 @@ class BasePort(HasMetadata):
   def __init__(self) -> None:
     """Abstract Base Class for ports"""
     self._parent: Optional[PortParentTypes]  # refined from Optional[Refable] in base LibraryElement
-    self._block_context: Optional[BaseBlock]  # set by metaclass, as lexical scope available pre-binding
     self._initializer_args: Tuple[Tuple[Any, ...], Dict[str, Any]]  # set by metaclass
 
     super().__init__()

edg/jlcparts/JlcPartsDiode.py

@@ -70,4 +70,4 @@ def _entry_to_table_row(cls, row_dict: Dict[PartsTableColumn, Any], filename: st
             return None
 
 
-lambda: JlcPartsDiode(), JlcPartsZenerDiode()  # ensure class is instantiable (non-abstract)
+lambda: (JlcPartsDiode(), JlcPartsZenerDiode())  # ensure class is instantiable (non-abstract)

edg/jlcparts/JlcPartsFet.py

@@ -68,4 +68,4 @@ class JlcPartsSwitchFet(PartsTableSelectorFootprint, JlcPartsBaseFet, TableSwitc
     pass
 
 
-lambda: JlcPartsFet(), JlcPartsSwitchFet()  # ensure class is instantiable (non-abstract)
+lambda: (JlcPartsFet(), JlcPartsSwitchFet())  # ensure class is instantiable (non-abstract)