id: make Tokens objects immutable

* Having Tokens objects mutable seemed like a good idea at the time.
* But in practice, Tokens are rarely modified.
* And their mutability is a barrier to use as dictionary keys and
  caching.
* Implement the `__hash__` interface and block the `__setitem__`
  and `update` interfaces.

Author: oliver-sanders

cylc/flow/id.py

@@ -74,7 +74,7 @@ class Tokens(dict):
         <id: w//c>
         >>> Tokens(workflow='w', cycle='c')['job']
 
-        # Make a copy (note Tokens are mutable):
+        # Make a copy (note Tokens are immutable):
         >>> tokens.duplicate()
         <id: ~u/w//c/t/01>
         >>> tokens.duplicate(job='02')  # make changes at the same time
@@ -118,9 +118,10 @@ def __init__(
         dict.__init__(self, **kwargs)
 
     def __setitem__(self, key, value):
-        if key not in self._KEYS:
-            raise ValueError(f'Invalid token: {key}')
-        dict.__setitem__(self, key, value)
+        raise Exception('Tokens objects are not mutable')
+
+    def update(self, other):
+        raise Exception('Tokens objects are not mutable')
 
     def __getitem__(self, key):
         try:
@@ -151,6 +152,9 @@ def __repr__(self):
             id_ = self.id
         return f'<id: {id_}>'
 
+    def __hash__(self):
+        return hash(tuple(self.values()))
+
     def __eq__(self, other):
         if not isinstance(other, self.__class__):
             return False
@@ -336,62 +340,20 @@ def is_null(self) -> bool:
             >>> tokens = Tokens()
             >>> tokens.is_null
             True
-            >>> tokens['job_sel'] = 'x'
-            >>> tokens.is_null
+            >>> tokens.duplicate(job_sel='x').is_null
             True
-            >>> tokens['job'] = '01'
-            >>> tokens.is_null
+            >>> tokens.duplicate(job='01').is_null
             False
 
         """
         return not any(
             self[key] for key in self._REGULAR_KEYS
         )
 
-    def update_tokens(
-        self,
-        tokens: 'Optional[Tokens]' = None,
-        **kwargs
-    ) -> None:
-        """Update the tokens dictionary.
-
-        Similar to dict.update but with an optional Tokens argument.
-
-        Examples:
-            >>> tokens = Tokens('x')
-            >>> tokens.update_tokens(workflow='y')
-            >>> tokens
-            <id: y>
-            >>> tokens.update_tokens(Tokens('z'))
-            >>> tokens
-            <id: z>
-            >>> tokens.update_tokens(Tokens('a'), cycle='b')
-            >>> tokens
-            <id: a//b>
-
-        """
-        if tokens:
-            for key, value in tokens.items():
-                self[key] = value
-        for key, value in kwargs.items():
-            self[key] = value
-
-    def update(self, other):
-        """dict.update.
-
-        Example:
-            >>> tokens = Tokens(workflow='w')
-            >>> tokens.update({'cycle': 'c'})
-            >>> tokens.id
-            'w//c'
-
-        """
-        return self.update_tokens(**other)
-
     def duplicate(
         self,
-        tokens: 'Optional[Tokens]' = None,
-        **kwargs
+        *tokens_list,
+        **kwargs,
     ) -> 'Tokens':
         """Duplicate a tokens object.
 
@@ -408,17 +370,28 @@ def duplicate(
             >>> id(tokens1) == id(tokens2)
             False
 
-            Make a copy and modify it:
+            Make a copy with a modification:
             >>> tokens1.duplicate(cycle='1').id
             '~u/w//1'
 
-            Original not changed
+            The Original is not changed:
             >>> tokens1.id
             '~u/w'
+
+            Arguments override in definition order:
+            >>> Tokens.duplicate(
+            ...     tokens1,
+            ...     Tokens(cycle='c', task='a', job='01'),
+            ...     task='b'
+            ... ).id
+            '~u/w//c/b/01'
+
         """
-        ret = Tokens(self)
-        ret.update_tokens(tokens, **kwargs)
-        return ret
+        _kwargs = {}
+        for tokens in (self, *tokens_list):
+            _kwargs.update(tokens)
+        _kwargs.update(kwargs)
+        return Tokens(**_kwargs)
 
 
 # //cycle[:sel][/task[:sel][/job[:sel]]]

cylc/flow/id_cli.py

@@ -294,7 +294,7 @@ async def parse_ids_async(
 
     # infer the run number if not specified the ID (and if possible)
     if infer_latest_runs:
-        _infer_latest_runs(*tokens_list, src_path=src_path)
+        _infer_latest_runs(tokens_list, src_path=src_path)
 
     _validate_number(
         *tokens_list,
@@ -409,12 +409,14 @@ def _validate_workflow_ids(*tokens_list, src_path):
         detect_both_flow_and_suite(src_path)
 
 
-def _infer_latest_runs(*tokens_list, src_path):
+def _infer_latest_runs(tokens_list, src_path):
     for ind, tokens in enumerate(tokens_list):
         if ind == 0 and src_path:
             # source workflow passed in as a path
             continue
-        tokens['workflow'] = infer_latest_run_from_id(tokens['workflow'])
+        tokens_list[ind] = tokens.duplicate(
+            workflow=infer_latest_run_from_id(tokens['workflow'])
+        )
         pass
 
 

cylc/flow/scripts/completion_server.py

@@ -390,7 +390,6 @@ async def list_in_workflow(tokens: Tokens, infer_run=True) -> t.List[str]:
             # list possible IDs
             cli_detokenise(
                 tokens.duplicate(
-                    tokens=None,
                     # use the workflow ID provided on the CLI to allow
                     # run name inference
                     workflow=input_workflow,

cylc/flow/task_id.py

@@ -107,5 +107,6 @@ def get_standardised_point(
     def get_standardised_taskid(cls, task_id):
         """Return task ID with standardised cycle point."""
         tokens = Tokens(task_id, relative=True)
-        tokens['cycle'] = cls.get_standardised_point_string(tokens['cycle'])
-        return tokens.relative_id
+        return tokens.duplicate(
+            cycle=cls.get_standardised_point_string(tokens['cycle'])
+        ).relative_id

cylc/flow/task_pool.py

@@ -1917,7 +1917,7 @@ def match_future_tasks(
                 # Glob or task state was not matched by active tasks
                 if not tokens['task']:
                     # make task globs explicit to make warnings clearer
-                    tokens['task'] = '*'
+                    tokens = tokens.duplicate(task='*')
                 LOG.warning(
                     'No active tasks matching:'
                     # preserve :selectors when logging the id
@@ -1985,7 +1985,7 @@ def match_taskdefs(
             point_str = tokens['cycle']
             if not tokens['task']:
                 # make task globs explicit to make warnings clearer
-                tokens['task'] = '*'
+                tokens = tokens.duplicate(task='*')
             name_str = tokens['task']
             try:
                 point_str = standardise_point_string(point_str)

tests/integration/test_task_pool.py

@@ -216,7 +216,8 @@ async def test_filter_task_proxies(
     expected_bad_items: List[str],
     expected_warnings: List[str],
     mod_example_flow: Scheduler,
-    caplog: pytest.LogCaptureFixture
+    caplog: pytest.LogCaptureFixture,
+    monkeypatch,
 ) -> None:
     """Test TaskPool.filter_task_proxies().
 
@@ -256,7 +257,8 @@ async def test_filter_task_proxies_hidden(
     expected_bad_items: List[str],
     expected_warnings: List[str],
     mod_example_flow: Scheduler,
-    caplog: pytest.LogCaptureFixture
+    caplog: pytest.LogCaptureFixture,
+    monkeypatch,
 ) -> None:
     """Test TaskPool.filter_task_proxies().
 
@@ -277,6 +279,13 @@ async def test_filter_task_proxies_hidden(
         expected_bad_items: Expected to be returned.
         expected_warnings: Expected to be logged.
     """
+    monkeypatch.setattr(
+        # make Tokens objects mutable to allow deepcopy to work on TaskProxy
+        # objects
+        'cylc.flow.id.Tokens.__setitem__',
+        lambda self, key, value: dict.__setitem__(self, key, value),
+    )
+
     caplog.set_level(logging.WARNING, CYLC_LOG)
     task_pool = mod_example_flow.pool
 

tests/unit/test_id.py

@@ -316,9 +316,9 @@ def test_tokens():
     with pytest.raises(ValueError):
         Tokens(foo='a')
 
-    Tokens()['cycle'] = 'a'
+    Tokens().duplicate(cycle='a')
     with pytest.raises(ValueError):
-        Tokens()['foo'] = 'a'
+        Tokens(foo='a')
 
     # test equality
     assert Tokens('a') == Tokens('a')
@@ -335,10 +335,24 @@ def test_tokens():
     assert not Tokens('a') == 1
 
     tokens = Tokens('a//b')
-    tokens.update({'cycle': 'c', 'task': 'd'})
-    assert tokens == Tokens('a//c/d')
-    with pytest.raises(ValueError):
+    new_tokens = tokens.duplicate(cycle='c', task='d')
+    assert new_tokens == Tokens('a//c/d')
+    with pytest.raises(Exception):
         tokens.update({'foo': 'c'})
+    with pytest.raises(Exception):
+        tokens['cycle'] = 'a'
+
+    # test gt/lt
+    assert sorted(
+        tokens.id
+        for tokens in [
+            Tokens('~u/c'),
+            Tokens('~u/b//1'),
+            Tokens('~u/a'),
+            Tokens('~u/b'),
+            Tokens('~u/b//2'),
+        ]
+    ) == ['~u/a', '~u/b', '~u/b//1', '~u/b//2', '~u/c']
 
 
 def test_no_look_behind():

tests/unit/test_id_cli.py

@@ -576,6 +576,6 @@ async def test_expand_workflow_tokens_impl_selector(no_scan):
     """It should reject filters it can't handle."""
     tokens = tokenise('~user/*')
     await _expand_workflow_tokens([tokens])
-    tokens['workflow_sel'] = 'stopped'
+    tokens = tokens.duplicate(workflow_sel='stopped')
     with pytest.raises(InputError):
         await _expand_workflow_tokens([tokens])