From 9fb8ff26608dd5dd99b37454d616e3e39c69f47a Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Fri, 4 Jun 2021 13:29:51 +0200 Subject: [PATCH 1/4] Hide access to variable descriptor. --- reframe/core/meta.py | 25 ++++++++++++++++++++++++- reframe/core/variables.py | 4 ++++ unittests/test_variables.py | 4 ++-- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 0708f8d53e..c4512231f1 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -286,6 +286,30 @@ def __call__(cls, *args, **kwargs): obj.__init__(*args, **kwargs) return obj + def __getattribute__(cls, name): + '''Handle special attribute access for variables. + + If the variable descriptor has already been injected into the class, + do not return the descriptor object and return the variable value + instead. + + .. warning:: + .. versionchanged:: 3.7.0 + Prior versions exposed the variable descriptor object after the + first class instantiation, instead of the variable's value. + ''' + + try: + var_space = super().__getattribute__('_rfm_var_space') + except AttributeError: + var_space = None + + # If the variable is already injected, delegate lookup to __getattr__. + if var_space and name in var_space.injected_vars: + raise AttributeError('delegate variable lookup to __getattr__') + + return super().__getattribute__(name) + def __getattr__(cls, name): '''Attribute lookup method for the MetaNamespace. @@ -295,7 +319,6 @@ def __getattr__(cls, name): method will perform an attribute lookup on these sub-namespaces if a call to the default :func:`__getattribute__` method fails to retrieve the requested class attribute. - ''' try: diff --git a/reframe/core/variables.py b/reframe/core/variables.py index a57cadb9bf..7dc3c2719f 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -544,3 +544,7 @@ def inject(self, obj, cls): @property def vars(self): return self._namespace + + @property + def injected_vars(self): + return self._injected_vars diff --git a/unittests/test_variables.py b/unittests/test_variables.py index 04f25e4d24..dbf091da1e 100644 --- a/unittests/test_variables.py +++ b/unittests/test_variables.py @@ -36,7 +36,7 @@ def test_custom_variable(OneVarTest): assert not isinstance(OneVarTest.foo, Field) inst = OneVarTest() assert hasattr(OneVarTest, 'foo') - assert isinstance(OneVarTest.foo, Field) + assert not isinstance(OneVarTest.foo, Field) assert hasattr(inst, 'foo') assert inst.foo == 10 @@ -136,7 +136,7 @@ class MyTest(OneVarTest): assert hasattr(OneVarTest, 'foo') assert not isinstance(OneVarTest.foo, Field) assert hasattr(MyTest, 'foo') - assert isinstance(MyTest.foo, Field) + assert not isinstance(MyTest.foo, Field) assert hasattr(inst, 'foo') assert inst.foo == 4 From 44c7395e49a6fda9ed2b4680390e77d6d7a2869e Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Fri, 4 Jun 2021 13:31:23 +0200 Subject: [PATCH 2/4] Bugfix __getattr__ --- reframe/core/meta.py | 69 +++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 29 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index c4512231f1..607856588a 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -23,11 +23,11 @@ class RegressionTestMeta(type): class MetaNamespace(namespaces.LocalNamespace): '''Custom namespace to control the cls attribute assignment. - Regular Python class attributes can be overriden by either + Regular Python class attributes can be overridden by either parameters or variables respecting the order of execution. A variable or a parameter may not be declared more than once in the same class body. Overriding a variable with a parameter or the other - way around has an undefined behaviour. A variable's value may be + way around has an undefined behavior. A variable's value may be updated multiple times within the same class body. A parameter's value may not be updated more than once within the same class body. ''' @@ -178,7 +178,7 @@ def bind(fn, name=None): with the ``name`` argument. :param fn: external function to be bound to a class. - :param name: bind the function under a diferent name. + :param name: bind the function under a different name. ''' inst = metacls.WrappedFunction(fn, name) @@ -272,7 +272,7 @@ def __call__(cls, *args, **kwargs): to perform specific reframe-internal actions. This gives extra control over the class instantiation process, allowing reframe to instantiate the regression test class differently if this class was registered or - not (e.g. when deep-copying a regression test object). These interal + not (e.g. when deep-copying a regression test object). These internal arguments must be intercepted before the object initialization, since these would otherwise affect the __init__ method's signature, and these internal mechanisms must be fully transparent to the user. @@ -287,16 +287,18 @@ def __call__(cls, *args, **kwargs): return obj def __getattribute__(cls, name): - '''Handle special attribute access for variables. - - If the variable descriptor has already been injected into the class, - do not return the descriptor object and return the variable value - instead. + '''Attribute lookup method for custom class attributes. + + ReFrame test variables are descriptors injected at the class level. + If a variable descriptor has already been injected into the class, + do not return the descriptor object and return the default value + associated with that variable instead. .. warning:: .. versionchanged:: 3.7.0 - Prior versions exposed the variable descriptor object after the - first class instantiation, instead of the variable's value. + Prior versions exposed the variable descriptor object if this + was already present in the class, instead of returning the + variable's default value. ''' try: @@ -308,40 +310,49 @@ def __getattribute__(cls, name): if var_space and name in var_space.injected_vars: raise AttributeError('delegate variable lookup to __getattr__') + # Default back to the base method if no special treatment required. return super().__getattribute__(name) def __getattr__(cls, name): - '''Attribute lookup method for the MetaNamespace. - - This metaclass uses a custom namespace, where ``variable`` built-in - and ``parameter`` types are stored in their own sub-namespaces (see - :class:`reframe.core.meta.RegressionTestMeta.MetaNamespace`). This - method will perform an attribute lookup on these sub-namespaces if a - call to the default :func:`__getattribute__` method fails to retrieve - the requested class attribute. + '''Backup attribute lookup method into custom namespaces. + + Some ReFrame built-in types are stored under their own sub-namespaces. + This method will perform an attribute lookup on these sub-namespaces + if a call to the default :func:`__getattribute__` method fails to + retrieve the requested class attribute. ''' try: - return cls._rfm_var_space.vars[name] + var_space = super().__getattribute__('_rfm_var_space') + return var_space.vars[name] + except AttributeError: + '''Catch early access attempt to the variable space.''' except KeyError: - try: - return cls._rfm_param_space.params[name] - except KeyError: - raise AttributeError( - f'class {cls.__qualname__!r} has no attribute {name!r}' - ) from None + '''Requested name not in variable space.''' + + try: + param_space = super().__getattribute__('_rfm_param_space') + return param_space.params[name] + except AttributeError: + '''Catch early access attempt to the parameter space.''' + except KeyError: + '''Requested name not in parameter space.''' + + raise AttributeError( + f'class {cls.__qualname__!r} has no attribute {name!r}' + ) from None def __setattr__(cls, name, value): '''Handle the special treatment required for variables and parameters. A variable's default value can be updated when accessed as a regular - class attribute. This behaviour does not apply when the assigned value + class attribute. This behavior does not apply when the assigned value is a descriptor object. In that case, the task of setting the value is delegated to the base :func:`__setattr__` (this is to comply with - standard Python behaviour). However, since the variables are already + standard Python behavior). However, since the variables are already descriptors which are injected during class instantiation, we disallow any attempt to override this descriptor (since it would be silently - re-overriden in any case). + re-overridden in any case). Altering the value of a parameter when accessed as a class attribute is not allowed. This would break the parameter space internals. From fd6cc665a0c001e0fd7299df9f06377ebcba85f8 Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Fri, 4 Jun 2021 14:16:13 +0200 Subject: [PATCH 3/4] Add unit tests --- unittests/test_meta.py | 22 ++++++++++++++++++++-- unittests/test_variables.py | 9 ++++----- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/unittests/test_meta.py b/unittests/test_meta.py index 6f97587fcd..56a8566d44 100644 --- a/unittests/test_meta.py +++ b/unittests/test_meta.py @@ -3,17 +3,35 @@ # # SPDX-License-Identifier: BSD-3-Clause +import pytest + import reframe as rfm import reframe.core.meta as meta +def test_class_attr_access(): + '''Test that `__getattr__` avoids infinite recursion.''' + def my_test(key): + class MyMeta(meta.RegressionTestMeta): + def __init__(cls, name, bases, namespace, **kwargs): + getattr(cls, f'{key}') + + msg = f'has no attribute {key!r}' + with pytest.raises(AttributeError, match=msg): + class Foo(metaclass=MyMeta): + pass + + my_test('_rfm_var_space') + my_test('_rfm_param_space') + + def test_directives(): '''Test that directives are not available as instance attributes.''' def ext_fn(x): pass - class MyTest(rfm.RegressionTest): + class MyTest(metaclass=meta.RegressionTestMeta): p = parameter() v = variable(int) bind(ext_fn, name='ext') @@ -38,7 +56,7 @@ def ext_fn(x): ext_fn._rfm_foo = True - class MyTest(rfm.RegressionTest): + class MyTest(metaclass=meta.RegressionTestMeta): bind(ext_fn) bind(ext_fn, name='ext') diff --git a/unittests/test_variables.py b/unittests/test_variables.py index dbf091da1e..3999546dde 100644 --- a/unittests/test_variables.py +++ b/unittests/test_variables.py @@ -8,7 +8,6 @@ import math import reframe as rfm -from reframe.core.fields import Field @pytest.fixture @@ -33,10 +32,10 @@ class OneVarTest(NoVarsTest): def test_custom_variable(OneVarTest): assert hasattr(OneVarTest, 'foo') - assert not isinstance(OneVarTest.foo, Field) + assert OneVarTest.foo == 10 inst = OneVarTest() assert hasattr(OneVarTest, 'foo') - assert not isinstance(OneVarTest.foo, Field) + assert OneVarTest.foo == 10 assert hasattr(inst, 'foo') assert inst.foo == 10 @@ -134,9 +133,9 @@ class MyTest(OneVarTest): inst = MyTest() assert hasattr(OneVarTest, 'foo') - assert not isinstance(OneVarTest.foo, Field) + assert OneVarTest.foo == 10 assert hasattr(MyTest, 'foo') - assert not isinstance(MyTest.foo, Field) + assert MyTest.foo == 4 assert hasattr(inst, 'foo') assert inst.foo == 4 From 0f4c3b9fd7b7fef0317371bb35e70afa50c5aa4f Mon Sep 17 00:00:00 2001 From: Javier Otero <71280927+jjotero@users.noreply.github.com> Date: Fri, 4 Jun 2021 14:33:22 +0200 Subject: [PATCH 4/4] Fix typo in docstring --- reframe/core/meta.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 607856588a..7752940dd4 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -396,7 +396,7 @@ def is_abstract(cls): This is the case when some parameters are undefined, which results in the length of the parameter space being 0. - :return: bool indicating wheteher the test has undefined parameters. + :return: bool indicating whether the test has undefined parameters. :meta private: '''