From f4eb4cee6527a248d9290810112c42117301d1db Mon Sep 17 00:00:00 2001
From: rocky <rb@dustyfeet.com>
Date: Sun, 13 Nov 2022 07:45:19 -0500
Subject: [PATCH 1/7] Add SetAttributes length check and ...

* More apply() -> eval() cutovers in the changed files
* Some <dt> indentation regularization.
* DRY redundant "argr" message definitions
---
 mathics/builtin/attributes.py          |   7 +
 mathics/builtin/files_io/filesystem.py | 196 +++++++++++++------------
 mathics/builtin/lists.py               |  80 +++++-----
 mathics/builtin/patterns.py            |   1 -
 test/builtin/test_attributes.py        |  24 ++-
 5 files changed, 172 insertions(+), 136 deletions(-)

diff --git a/mathics/builtin/attributes.py b/mathics/builtin/attributes.py
index 05a72523a..5c161ecae 100644
--- a/mathics/builtin/attributes.py
+++ b/mathics/builtin/attributes.py
@@ -745,6 +745,13 @@ def eval(self, symbols, attributes, evaluation):
                         evaluation.message("Attributes", "attnf", Symbol(value))
         return SymbolNull
 
+    def eval_arg_error(self, args, evaluation):
+        "SetAttributes[args___]"
+        # We should only come here when we don't have 2 args, because
+        # eval() should be called otherwise.
+        nargs = len(args.elements) if isinstance(args, Expression) else 1
+        evaluation.message("SetAttributes", "argrx", "SetAttributes", nargs, 2)
+
 
 class Unprotect(Builtin):
     """
diff --git a/mathics/builtin/files_io/filesystem.py b/mathics/builtin/files_io/filesystem.py
index a11299cb5..9cf5da9de 100644
--- a/mathics/builtin/files_io/filesystem.py
+++ b/mathics/builtin/files_io/filesystem.py
@@ -64,7 +64,7 @@
 class AbsoluteFileName(Builtin):
     """
     <dl>
-    <dt>'AbsoluteFileName["$name$"]'
+      <dt>'AbsoluteFileName["$name$"]'
       <dd>returns the absolute version of the given filename.
     </dl>
 
@@ -82,7 +82,7 @@ class AbsoluteFileName(Builtin):
     }
     summary_text = "absolute path"
 
-    def apply(self, name, evaluation):
+    def eval(self, name, evaluation):
         "AbsoluteFileName[name_]"
 
         py_name = name.to_python()
@@ -106,7 +106,7 @@ def apply(self, name, evaluation):
 class BaseDirectory(Predefined):
     """
     <dl>
-    <dt>'$UserBaseDirectory'
+      <dt>'$UserBaseDirectory'
       <dd>returns the folder where user configurations are stored.
     </dl>
 
@@ -125,13 +125,12 @@ def evaluate(self, evaluation):
 class CopyDirectory(Builtin):
     """
     <dl>
-    <dt>'CopyDirectory["$dir1$", "$dir2$"]'
+      <dt>'CopyDirectory["$dir1$", "$dir2$"]'
       <dd>copies directory $dir1$ to $dir2$.
     </dl>
     """
 
     messages = {
-        "argr": "called with `1` argument; 2 arguments are expected.",
         "fstr": (
             "File specification `1` is not a string of " "one or more characters."
         ),
@@ -140,12 +139,12 @@ class CopyDirectory(Builtin):
     }
     summary_text = "copy a directory into a new path"
 
-    def apply(self, dirs, evaluation):
+    def eval(self, dirs, evaluation):
         "CopyDirectory[dirs__]"
 
         seq = dirs.get_sequence()
         if len(seq) != 2:
-            evaluation.message("CopyDirectory", "argr", len(seq))
+            evaluation.message("CopyDirectory", "argr", "CopyDirectory", 2)
             return
         (dir1, dir2) = (s.to_python() for s in seq)
 
@@ -174,7 +173,7 @@ def apply(self, dirs, evaluation):
 class CopyFile(Builtin):
     """
     <dl>
-    <dt>'CopyFile["$file1$", "$file2$"]'
+      <dt>'CopyFile["$file1$", "$file2$"]'
       <dd>copies $file1$ to $file2$.
     </dl>
 
@@ -192,7 +191,7 @@ class CopyFile(Builtin):
     }
     summary_text = "copy a file into a new path"
 
-    def apply(self, source, dest, evaluation):
+    def eval(self, source, dest, evaluation):
         "CopyFile[source_, dest_]"
 
         py_source = source.to_python()
@@ -233,9 +232,10 @@ def apply(self, source, dest, evaluation):
 class CreateDirectory(Builtin):
     """
     <dl>
-    <dt>'CreateDirectory["$dir$"]'
+      <dt>'CreateDirectory["$dir$"]'
       <dd>creates a directory called $dir$.
-    <dt>'CreateDirectory[]'
+
+      <dt>'CreateDirectory[]'
       <dd>creates a temporary directory.
     </dl>
 
@@ -261,7 +261,7 @@ class CreateDirectory(Builtin):
     }
     summary_text = "create a directory"
 
-    def apply(self, dirname, evaluation, options):
+    def eval(self, dirname, evaluation, options):
         "CreateDirectory[dirname_, OptionsPattern[CreateDirectory]]"
 
         expr = to_expression("CreateDirectory", dirname)
@@ -285,7 +285,7 @@ def apply(self, dirname, evaluation, options):
 
         return String(osp.abspath(py_dirname))
 
-    def apply_empty(self, evaluation, options):
+    def eval_empty(self, evaluation, options):
         "CreateDirectory[OptionsPattern[CreateDirectory]]"
         dirname = tempfile.mkdtemp(prefix="m", dir=TMP_DIR)
         return String(dirname)
@@ -294,10 +294,11 @@ def apply_empty(self, evaluation, options):
 class CreateFile(Builtin):
     """
     <dl>
-    <dt>'CreateFile["filename"]'
-        <dd>Creates a file named "filename" temporary file, but do not open it.
-    <dt>'CreateFile[]'
-        <dd>Creates a temporary file, but do not open it.
+      <dt>'CreateFile["filename"]'
+      <dd>Creates a file named "filename" temporary file, but do not open it.
+
+      <dt>'CreateFile[]'
+      <dd>Creates a temporary file, but do not open it.
     </dl>
     """
 
@@ -311,7 +312,7 @@ class CreateFile(Builtin):
     }
     summary_text = "create a file"
 
-    def apply(self, filename, evaluation, **options):
+    def eval(self, filename, evaluation, **options):
         "CreateFile[filename_String, OptionsPattern[CreateFile]]"
         try:
             # TODO: Implement options
@@ -329,14 +330,14 @@ def apply(self, filename, evaluation, **options):
 class CreateTemporary(Builtin):
     """
     <dl>
-    <dt>'CreateTemporary[]'
-        <dd>Creates a temporary file, but do not open it.
+      <dt>'CreateTemporary[]'
+      <dd>Creates a temporary file, but do not open it.
     </dl>
     """
 
     summary_text = "create a temporary file"
 
-    def apply_0(self, evaluation):
+    def eval_0(self, evaluation):
         "CreateTemporary[]"
         try:
             res = create_temporary_file()
@@ -348,7 +349,7 @@ def apply_0(self, evaluation):
 class DeleteDirectory(Builtin):
     """
     <dl>
-    <dt>'DeleteDirectory["$dir$"]'
+      <dt>'DeleteDirectory["$dir$"]'
       <dd>deletes a directory called $dir$.
     </dl>
 
@@ -375,7 +376,7 @@ class DeleteDirectory(Builtin):
     }
     summary_text = "delete a directory"
 
-    def apply(self, dirname, evaluation, options):
+    def eval(self, dirname, evaluation, options):
         "DeleteDirectory[dirname_, OptionsPattern[DeleteDirectory]]"
 
         expr = to_expression("DeleteDirectory", dirname)
@@ -410,9 +411,10 @@ def apply(self, dirname, evaluation, options):
 class DeleteFile(Builtin):
     """
     <dl>
-    <dt>'Delete["$file$"]'
+      <dt>'Delete["$file$"]'
       <dd>deletes $file$.
-    <dt>'Delete[{"$file1$", "$file2$", ...}]'
+
+      <dt>'Delete[{"$file1$", "$file2$", ...}]'
       <dd>deletes a list of files.
     </dl>
 
@@ -433,7 +435,7 @@ class DeleteFile(Builtin):
     }
     summary_text = "delete a file"
 
-    def apply(self, filename, evaluation):
+    def eval(self, filename, evaluation):
         "DeleteFile[filename_]"
 
         py_path = filename.to_python()
@@ -474,7 +476,7 @@ def apply(self, filename, evaluation):
 class Directory(Builtin):
     """
     <dl>
-    <dt>'Directory[]'
+      <dt>'Directory[]'
       <dd>returns the current working directory.
     </dl>
 
@@ -484,7 +486,7 @@ class Directory(Builtin):
 
     summary_text = "current working directory"
 
-    def apply(self, evaluation):
+    def eval(self, evaluation):
         "Directory[]"
         result = os.getcwd()
         return String(result)
@@ -493,7 +495,7 @@ def apply(self, evaluation):
 class DirectoryName(Builtin):
     """
     <dl>
-    <dt>'DirectoryName["$name$"]'
+      <dt>'DirectoryName["$name$"]'
       <dd>extracts the directory name from a filename.
     </dl>
 
@@ -531,7 +533,7 @@ class DirectoryName(Builtin):
     }
     summary_text = "directory part of a filename"
 
-    def apply_with_n(self, name, n, evaluation, options):
+    def eval_with_n(self, name, n, evaluation, options):
         "DirectoryName[name_, n_, OptionsPattern[DirectoryName]]"
 
         if n is None:
@@ -557,15 +559,15 @@ def apply_with_n(self, name, n, evaluation, options):
 
         return String(result)
 
-    def apply(self, name, evaluation, options):
+    def eval(self, name, evaluation, options):
         "DirectoryName[name_, OptionsPattern[DirectoryName]]"
-        return self.apply_with_n(name, None, evaluation, options)
+        return self.eval_with_n(name, None, evaluation, options)
 
 
 class DirectoryStack(Builtin):
     """
     <dl>
-    <dt>'DirectoryStack[]'
+      <dt>'DirectoryStack[]'
       <dd>returns the directory stack.
     </dl>
 
@@ -575,7 +577,7 @@ class DirectoryStack(Builtin):
 
     summary_text = "list the sequence of current directories in use"
 
-    def apply(self, evaluation):
+    def eval(self, evaluation):
         "DirectoryStack[]"
         global DIRECTORY_STACK
         return from_python(DIRECTORY_STACK)
@@ -584,7 +586,7 @@ def apply(self, evaluation):
 class DirectoryQ(Builtin):
     """
     <dl>
-    <dt>'DirectoryQ["$name$"]'
+      <dt>'DirectoryQ["$name$"]'
       <dd>returns 'True' if the directory called $name$ exists and 'False' otherwise.
     </dl>
 
@@ -607,7 +609,7 @@ class DirectoryQ(Builtin):
     }
     summary_text = "test whether a path exists and is a directory"
 
-    def apply(self, pathname, evaluation):
+    def eval(self, pathname, evaluation):
         "DirectoryQ[pathname_]"
         path = pathname.to_python()
 
@@ -626,7 +628,7 @@ def apply(self, pathname, evaluation):
 class ExpandFileName(Builtin):
     """
     <dl>
-    <dt>'ExpandFileName["$name$"]'
+      <dt>'ExpandFileName["$name$"]'
       <dd>expands $name$ to an absolute filename for your system.
     </dl>
 
@@ -639,7 +641,7 @@ class ExpandFileName(Builtin):
     }
     summary_text = "absolute path"
 
-    def apply(self, name, evaluation):
+    def eval(self, name, evaluation):
         "ExpandFileName[name_]"
 
         py_name = name.to_python()
@@ -657,7 +659,7 @@ def apply(self, name, evaluation):
 class File(Builtin):
     """
     <dl>
-    <dt>'File["$file$"]'
+      <dt>'File["$file$"]'
       <dd>is a symbolic representation of an element in the local file system.
     </dl>
     """
@@ -668,7 +670,7 @@ class File(Builtin):
 class FileBaseName(Builtin):
     """
     <dl>
-    <dt>'FileBaseName["$file$"]'
+      <dt>'FileBaseName["$file$"]'
       <dd>gives the base name for the specified file name.
     </dl>
 
@@ -690,7 +692,7 @@ class FileBaseName(Builtin):
     }
     summary_text = "base name of the file"
 
-    def apply(self, filename, evaluation, options):
+    def eval(self, filename, evaluation, options):
         "FileBaseName[filename_String, OptionsPattern[FileBaseName]]"
         path = filename.to_python()[1:-1]
 
@@ -701,7 +703,7 @@ def apply(self, filename, evaluation, options):
 class FileByteCount(Builtin):
     """
     <dl>
-    <dt>'FileByteCount[$file$]'
+      <dt>'FileByteCount[$file$]'
       <dd>returns the number of bytes in $file$.
     </dl>
 
@@ -714,7 +716,7 @@ class FileByteCount(Builtin):
     }
     summary_text = "length of the file"
 
-    def apply(self, filename, evaluation):
+    def eval(self, filename, evaluation):
         "FileByteCount[filename_]"
         py_filename = filename.to_python()
         if not (
@@ -745,7 +747,7 @@ def apply(self, filename, evaluation):
 class FileDate(Builtin):
     """
     <dl>
-    <dt>'FileDate[$file$, $types$]'
+      <dt>'FileDate[$file$, $types$]'
       <dd>returns the time and date at which the file was last modified.
     </dl>
 
@@ -796,7 +798,7 @@ class FileDate(Builtin):
     }
     summary_text = "date and time of the last change in a file"
 
-    def apply(self, path, timetype, evaluation):
+    def eval(self, path, timetype, evaluation):
         "FileDate[path_, timetype_]"
         py_path, is_temparary_file = path_search(path.to_python()[1:-1])
 
@@ -839,15 +841,15 @@ def apply(self, path, timetype, evaluation):
 
         return to_expression("DateList", Real(result))
 
-    def apply_default(self, path, evaluation):
+    def eval_default(self, path, evaluation):
         "FileDate[path_]"
-        return self.apply(path, None, evaluation)
+        return self.eval(path, None, evaluation)
 
 
 class FileExistsQ(Builtin):
     """
     <dl>
-    <dt>'FileExistsQ["$file$"]'
+      <dt>'FileExistsQ["$file$"]'
       <dd>returns 'True' if $file$ exists and 'False' otherwise.
     </dl>
 
@@ -864,7 +866,7 @@ class FileExistsQ(Builtin):
     }
     summary_text = "test whether a file exists"
 
-    def apply(self, filename, evaluation):
+    def eval(self, filename, evaluation):
         "FileExistsQ[filename_]"
         path = filename.to_python()
         if not (isinstance(path, str) and path[0] == path[-1] == '"'):
@@ -882,7 +884,7 @@ def apply(self, filename, evaluation):
 class FileExtension(Builtin):
     """
     <dl>
-    <dt>'FileExtension["$file$"]'
+      <dt>'FileExtension["$file$"]'
       <dd>gives the extension for the specified file name.
     </dl>
 
@@ -903,7 +905,7 @@ class FileExtension(Builtin):
     }
     summary_text = "file extension"
 
-    def apply(self, filename, evaluation, options):
+    def eval(self, filename, evaluation, options):
         "FileExtension[filename_String, OptionsPattern[FileExtension]]"
         path = filename.to_python()[1:-1]
         filename_base, filename_ext = osp.splitext(path)
@@ -914,12 +916,14 @@ def apply(self, filename, evaluation, options):
 class FileHash(Builtin):
     """
     <dl>
-    <dt>'FileHash[$file$]'
+      <dt>'FileHash[$file$]'
       <dd>returns an integer hash for the given $file$.
-    <dt>'FileHash[$file$, $type$]'
+
+      <dt>'FileHash[$file$, $type$]'
       <dd>returns an integer hash of the specified $type$ for the given $file$.
       <dd>The types supported are "MD5", "Adler32", "CRC32", "SHA", "SHA224", "SHA256", "SHA384", and "SHA512".
-    <dt>'FileHash[$file$, $type$, $format$]'
+
+      <dt>'FileHash[$file$, $type$, $format$]'
       <dd>gives a hash code in the specified format.
     </dl>
 
@@ -961,7 +965,7 @@ class FileHash(Builtin):
     }
     summary_text = "compute a hash from the content of a file"
 
-    def apply(self, filename, hashtype, format, evaluation):
+    def eval(self, filename, hashtype, format, evaluation):
         "FileHash[filename_String, hashtype_String, format_String]"
         py_filename = filename.get_string_value()
 
@@ -985,7 +989,7 @@ def apply(self, filename, hashtype, format, evaluation):
 class FileInformation(Builtin):
     """
     <dl>
-    <dt>'FileInformation["$file$"]'
+      <dt>'FileInformation["$file$"]'
       <dd>returns information about $file$.
     </dl>
 
@@ -1007,7 +1011,7 @@ class FileInformation(Builtin):
 class FileNameDepth(Builtin):
     """
     <dl>
-    <dt>'FileNameDepth["$name$"]'
+      <dt>'FileNameDepth["$name$"]'
       <dd>gives the number of path parts in the given filename.
     </dl>
 
@@ -1065,7 +1069,7 @@ class FileNameJoin(Builtin):
     }
     summary_text = "join parts into a path"
 
-    def apply(self, pathlist, evaluation, options):
+    def eval(self, pathlist, evaluation, options):
         "FileNameJoin[pathlist_List, OptionsPattern[FileNameJoin]]"
 
         py_pathlist = pathlist.to_python()
@@ -1107,7 +1111,7 @@ def apply(self, pathlist, evaluation, options):
 class FileType(Builtin):
     """
     <dl>
-    <dt>'FileType["$file$"]'
+      <dt>'FileType["$file$"]'
       <dd>gives the type of a file, a string. This is typically 'File', 'Directory' or 'None'.
     </dl>
 
@@ -1130,7 +1134,7 @@ class FileType(Builtin):
     }
     summary_text = "type of a file"
 
-    def apply(self, filename, evaluation):
+    def eval(self, filename, evaluation):
         "FileType[filename_]"
         if not isinstance(filename, String):
             evaluation.message("FileType", "fstr", filename)
@@ -1151,7 +1155,7 @@ def apply(self, filename, evaluation):
 class FindFile(Builtin):
     """
     <dl>
-    <dt>'FindFile[$name$]'
+      <dt>'FindFile[$name$]'
       <dd>searches '$Path' for the given filename.
     </dl>
 
@@ -1175,7 +1179,7 @@ class FindFile(Builtin):
         "search the path of of a file in the current directory and its subdirectories"
     )
 
-    def apply(self, name, evaluation):
+    def eval(self, name, evaluation):
         "FindFile[name_]"
 
         py_name = name.to_python()
@@ -1231,21 +1235,21 @@ class FileNames(Builtin):
     }
     summary_text = "list file names in the current directory"
 
-    def apply_0(self, evaluation, **options):
+    def eval_0(self, evaluation, **options):
         """FileNames[OptionsPattern[FileNames]]"""
-        return self.apply_3(
+        return self.eval_3(
             String("*"), String(os.getcwd()), None, evaluation, **options
         )
 
-    def apply_1(self, forms, evaluation, **options):
+    def eval_1(self, forms, evaluation, **options):
         """FileNames[forms_, OptionsPattern[FileNames]]"""
-        return self.apply_3(forms, String(os.getcwd()), None, evaluation, **options)
+        return self.eval_3(forms, String(os.getcwd()), None, evaluation, **options)
 
-    def apply_2(self, forms, paths, evaluation, **options):
+    def eval_2(self, forms, paths, evaluation, **options):
         """FileNames[forms_, paths_, OptionsPattern[FileNames]]"""
-        return self.apply_3(forms, paths, None, evaluation, **options)
+        return self.eval_3(forms, paths, None, evaluation, **options)
 
-    def apply_3(self, forms, paths, n, evaluation, **options):
+    def eval_3(self, forms, paths, n, evaluation, **options):
         """FileNames[forms_, paths_, n_, OptionsPattern[FileNames]]"""
         filenames = set()
         # Building a list of forms
@@ -1358,7 +1362,7 @@ class FileNameSplit(Builtin):
 
     summary_text = "split the file name in a list of parts"
 
-    def apply(self, filename, evaluation, options):
+    def eval(self, filename, evaluation, options):
         "FileNameSplit[filename_String, OptionsPattern[FileNameSplit]]"
 
         path = filename.to_python()[1:-1]
@@ -1419,12 +1423,12 @@ class FileNameTake(Builtin):
     }
     summary_text = "take a part of the filename"
 
-    def apply(self, filename, evaluation, options):
+    def eval(self, filename, evaluation, options):
         "FileNameTake[filename_String, OptionsPattern[FileBaseName]]"
         path = pathlib.Path(filename.to_python()[1:-1])
         return String(path.name)
 
-    def apply_n(self, filename, n, evaluation, options):
+    def eval_n(self, filename, n, evaluation, options):
         "FileNameTake[filename_String, n_Integer, OptionsPattern[FileBaseName]]"
         n_int = n.get_int_value()
         parts = pathlib.Path(filename.to_python()[1:-1]).parts
@@ -1479,11 +1483,11 @@ class FindList(Builtin):
     # TODO: Extra options AnchoredSearch, IgnoreCase RecordSeparators,
     # WordSearch, WordSeparators this is probably best done with a regex
 
-    def apply_without_n(self, filename, text, evaluation, options):
+    def eval_without_n(self, filename, text, evaluation, options):
         "FindList[filename_, text_, OptionsPattern[FindList]]"
-        return self.apply(filename, text, None, evaluation, options)
+        return self.eval(filename, text, None, evaluation, options)
 
-    def apply(self, filename, text, n, evaluation, options):
+    def eval(self, filename, text, n, evaluation, options):
         "FindList[filename_, text_, n_, OptionsPattern[FindList]]"
         py_text = text.to_python()
         py_name = filename.to_python()
@@ -1713,7 +1717,7 @@ class Needs(Builtin):
     }
     summary_text = "load a package if it is not already loaded"
 
-    def apply(self, context, evaluation):
+    def eval(self, context, evaluation):
         "Needs[context_String]"
         contextstr = context.get_string_value()
         if contextstr == "":
@@ -1789,7 +1793,7 @@ class ParentDirectory(Builtin):
     }
     summary_text = "parent directory of the current working directory"
 
-    def apply(self, path, evaluation):
+    def eval(self, path, evaluation):
         "ParentDirectory[path_]"
 
         if not isinstance(path, String):
@@ -1848,7 +1852,6 @@ class RenameDirectory(Builtin):
     """
 
     messages = {
-        "argr": "called with `1` argument; 2 arguments are expected.",
         "fstr": (
             "File specification `1` is not a string of " "one or more characters."
         ),
@@ -1857,12 +1860,12 @@ class RenameDirectory(Builtin):
     }
     summary_text = "change the name of a directory"
 
-    def apply(self, dirs, evaluation):
+    def eval(self, dirs, evaluation):
         "RenameDirectory[dirs__]"
 
         seq = dirs.get_sequence()
         if len(seq) != 2:
-            evaluation.message("RenameDirectory", "argr", len(seq))
+            evaluation.message("RenameDirectory", "argr", "RenameDirectory", 2)
             return
         (dir1, dir2) = (s.to_python() for s in seq)
 
@@ -1911,7 +1914,7 @@ class RenameFile(Builtin):
     }
     summary_text = "change the name of a file"
 
-    def apply(self, source, dest, evaluation):
+    def eval(self, source, dest, evaluation):
         "RenameFile[source_, dest_]"
 
         py_source = source.to_python()
@@ -1963,7 +1966,7 @@ class ResetDirectory(Builtin):
     }
     summary_text = "return to the directory before the last SetDirectory call"
 
-    def apply(self, evaluation):
+    def eval(self, evaluation):
         "ResetDirectory[]"
         try:
             tmp = DIRECTORY_STACK.pop()
@@ -2020,7 +2023,7 @@ class SetDirectory(Builtin):
     }
     summary_text = "set the working directory"
 
-    def apply(self, path, evaluation):
+    def eval(self, path, evaluation):
         "SetDirectory[path_]"
 
         if not isinstance(path, String):
@@ -2097,7 +2100,7 @@ class SetFileDate(Builtin):
     }
     summary_text = "set the access/modification time of a file in the filesystem"
 
-    def apply(self, filename, datelist, attribute, evaluation):
+    def eval(self, filename, datelist, attribute, evaluation):
         "SetFileDate[filename_, datelist_, attribute_]"
 
         py_filename = filename.to_python()
@@ -2170,19 +2173,19 @@ def apply(self, filename, datelist, attribute, evaluation):
                 os.utime(py_filename, (osp.getatime(py_filename), stattime))
             if py_attr == "All":
                 os.utime(py_filename, (stattime, stattime))
-        except OSError as e:
+        except OSError:  #  as e:
             # evaluation.message(...)
             return SymbolFailed
 
         return SymbolNull
 
-    def apply_1arg(self, filename, evaluation):
+    def eval_1arg(self, filename, evaluation):
         "SetFileDate[filename_]"
-        return self.apply(filename, None, None, evaluation)
+        return self.eval(filename, None, None, evaluation)
 
-    def apply_2arg(self, filename, datelist, evaluation):
+    def eval_2arg(self, filename, datelist, evaluation):
         "SetFileDate[filename_, datelist_]"
-        return self.apply(filename, datelist, None, evaluation)
+        return self.eval(filename, datelist, None, evaluation)
 
 
 class TemporaryDirectory(Predefined):
@@ -2252,10 +2255,11 @@ def evaluate(self, evaluation):
 class URLSave(Builtin):
     """
     <dl>
-    <dt>'URLSave["url"]'
-        <dd>Save "url" in a temporary file.
-    <dt>'URLSave["url", $filename$]'
-        <dd>Save "url" in $filename$.
+      <dt>'URLSave["url"]'
+      <dd>Save "url" in a temporary file.
+
+      <dt>'URLSave["url", $filename$]'
+      <dd>Save "url" in $filename$.
     </dl>
     """
 
@@ -2265,11 +2269,11 @@ class URLSave(Builtin):
     }
     summary_text = "save the content of an URL"
 
-    def apply_1(self, url, evaluation, **options):
+    def eval_1(self, url, evaluation, **options):
         "URLSave[url_String, OptionsPattern[URLSave]]"
-        return self.apply_2(url, None, evaluation, **options)
+        return self.eval_2(url, None, evaluation, **options)
 
-    def apply_2(self, url, filename, evaluation, **options):
+    def eval_2(self, url, filename, evaluation, **options):
         "URLSave[url_String, filename_, OptionsPattern[URLSave]]"
         url = url.value
         if filename is None:
diff --git a/mathics/builtin/lists.py b/mathics/builtin/lists.py
index 004e90d6b..68785ff4b 100644
--- a/mathics/builtin/lists.py
+++ b/mathics/builtin/lists.py
@@ -172,7 +172,7 @@ def check_options(self, expr, evaluation, options):
                     return evaluation.message("ContainsOnly", "optx", Symbol(key), expr)
         return None
 
-    def apply(self, list1, list2, evaluation, options={}):
+    def eval(self, list1, list2, evaluation, options={}):
         "ContainsOnly[list1_List, list2_List, OptionsPattern[ContainsOnly]]"
 
         same_test = self.get_option(options, "SameTest", evaluation)
@@ -188,7 +188,7 @@ def sameQ(a, b) -> bool:
                 return SymbolFalse
         return SymbolTrue
 
-    def apply_msg(self, e1, e2, evaluation, options={}):
+    def eval_msg(self, e1, e2, evaluation, options={}):
         "ContainsOnly[e1_, e2_, OptionsPattern[ContainsOnly]]"
 
         opts = (
@@ -295,6 +295,8 @@ class Delete(Builtin):
     """
 
     messages = {
+        # FIXME: This message doesn't exist in more modern WMA, and
+        # Delete *can* take more than 2 arguments.
         "argr": "Delete called with 1 argument; 2 arguments are expected.",
         "argt": "Delete called with `1` arguments; 2 arguments are expected.",
         "psl": "Position specification `1` in `2` is not a machine-sized integer or a list of machine-sized integers.",
@@ -302,7 +304,7 @@ class Delete(Builtin):
     }
     summary_text = "delete elements from a list at given positions"
 
-    def apply_one(self, expr, position: Integer, evaluation):
+    def eval_one(self, expr, position: Integer, evaluation):
         "Delete[expr_, position_Integer]"
         pos = position.value
         try:
@@ -310,7 +312,7 @@ def apply_one(self, expr, position: Integer, evaluation):
         except PartRangeError:
             evaluation.message("Part", "partw", ListExpression(position), expr)
 
-    def apply(self, expr, positions, evaluation):
+    def eval(self, expr, positions, evaluation):
         "Delete[expr_, positions___]"
         positions = positions.get_sequence()
         if len(positions) > 1:
@@ -443,7 +445,7 @@ class Level(Builtin):
     }
     summary_text = "parts specified by a given number of indices"
 
-    def apply(self, expr, ls, evaluation, options={}):
+    def eval(self, expr, ls, evaluation, options={}):
         "Level[expr_, ls_, OptionsPattern[Level]]"
 
         try:
@@ -509,7 +511,7 @@ class List(Builtin):
     attributes = A_LOCKED | A_PROTECTED
     summary_text = "specify a list explicitly"
 
-    def apply(self, elements, evaluation):
+    def eval(self, elements, evaluation):
         """List[elements___]"""
         # Pick out the elements part of the parameter elements;
         # we we will call that `elements_part_of_elements__`.
@@ -518,7 +520,7 @@ def apply(self, elements, evaluation):
         elements_part_of_elements__ = elements.get_sequence()
         return ListExpression(*elements_part_of_elements__)
 
-    def apply_makeboxes(self, items, f, evaluation):
+    def eval_makeboxes(self, items, f, evaluation):
         """MakeBoxes[{items___},
         f:StandardForm|TraditionalForm|OutputForm|InputForm|FullForm]"""
 
@@ -649,7 +651,7 @@ class Split(Builtin):
     }
     summary_text = "split into runs of identical elements"
 
-    def apply(self, mlist, test, evaluation):
+    def eval(self, mlist, test, evaluation):
         "Split[mlist_, test_]"
 
         expr = Expression(SymbolSplit, mlist, test)
@@ -702,7 +704,7 @@ class SplitBy(Builtin):
 
     summary_text = "split based on values of a function applied to elements"
 
-    def apply(self, mlist, func, evaluation):
+    def eval(self, mlist, func, evaluation):
         "SplitBy[mlist_, func_?NotListQ]"
 
         expr = Expression(SymbolSplit, mlist, func)
@@ -727,7 +729,7 @@ def apply(self, mlist, func, evaluation):
         outer = structure(mlist.head, inner, evaluation)
         return outer([inner(t) for t in result])
 
-    def apply_multiple(self, mlist, funcs, evaluation):
+    def eval_multiple(self, mlist, funcs, evaluation):
         "SplitBy[mlist_, funcs_List]"
         expr = Expression(SymbolSplit, mlist, funcs)
 
@@ -737,7 +739,7 @@ def apply_multiple(self, mlist, funcs, evaluation):
 
         result = mlist
         for f in funcs.elements[::-1]:
-            result = self.apply(result, f, evaluation)
+            result = self.eval(result, f, evaluation)
 
         return result
 
@@ -781,7 +783,7 @@ class LeafCount(Builtin):
     }
     summary_text = "the total number of atomic subexpressions"
 
-    def apply(self, expr, evaluation):
+    def eval(self, expr, evaluation):
         "LeafCount[expr___]"
 
         from mathics.core.atoms import Rational, Complex
@@ -820,7 +822,7 @@ class _IterationFunction(Builtin):
     def get_result(self, items):
         pass
 
-    def apply_symbol(self, expr, iterator, evaluation):
+    def eval_symbol(self, expr, iterator, evaluation):
         "%(name)s[expr_, iterator_Symbol]"
         iterator = iterator.evaluate(evaluation)
         if iterator.has_form(["List", "Range", "Sequence"], None):
@@ -830,19 +832,19 @@ def apply_symbol(self, expr, iterator, evaluation):
             elif len(elements) == 2:
                 if elements[1].has_form(["List", "Sequence"], None):
                     seq = Expression(SymbolSequence, *(elements[1].elements))
-                    return self.apply_list(expr, elements[0], seq, evaluation)
+                    return self.eval_list(expr, elements[0], seq, evaluation)
                 else:
-                    return self.apply_range(expr, *elements, evaluation)
+                    return self.eval_range(expr, *elements, evaluation)
             elif len(elements) == 3:
-                return self.apply_iter_nostep(expr, *elements, evaluation)
+                return self.eval_iter_nostep(expr, *elements, evaluation)
             elif len(elements) == 4:
-                return self.apply_iter(expr, *elements, evaluation)
+                return self.eval_iter(expr, *elements, evaluation)
 
         if self.throw_iterb:
             evaluation.message(self.get_name(), "iterb")
         return
 
-    def apply_range(self, expr, i, imax, evaluation):
+    def eval_range(self, expr, i, imax, evaluation):
         "%(name)s[expr_, {i_Symbol, imax_}]"
         imax = imax.evaluate(evaluation)
         if imax.has_form("Range", None):
@@ -852,11 +854,11 @@ def apply_range(self, expr, i, imax, evaluation):
             return self.apply_list(expr, i, seq, evaluation)
         elif imax.has_form("List", None):
             seq = Expression(SymbolSequence, *(imax.elements))
-            return self.apply_list(expr, i, seq, evaluation)
+            return self.eval_list(expr, i, seq, evaluation)
         else:
-            return self.apply_iter(expr, i, Integer1, imax, Integer1, evaluation)
+            return self.eval_iter(expr, i, Integer1, imax, Integer1, evaluation)
 
-    def apply_max(self, expr, imax, evaluation):
+    def eval_max(self, expr, imax, evaluation):
         "%(name)s[expr_, {imax_}]"
 
         # Even though `imax` should be an integeral value, its type does not
@@ -912,11 +914,11 @@ def do_iteration():
 
         return self.get_result(result)
 
-    def apply_iter_nostep(self, expr, i, imin, imax, evaluation):
+    def eval_iter_nostep(self, expr, i, imin, imax, evaluation):
         "%(name)s[expr_, {i_Symbol, imin_, imax_}]"
-        return self.apply_iter(expr, i, imin, imax, Integer1, evaluation)
+        return self.eval_iter(expr, i, imin, imax, Integer1, evaluation)
 
-    def apply_iter(self, expr, i, imin, imax, di, evaluation):
+    def eval_iter(self, expr, i, imin, imax, di, evaluation):
         "%(name)s[expr_, {i_Symbol, imin_, imax_, di_}]"
 
         if isinstance(self, SympyFunction) and di.get_int_value() == 1:
@@ -977,7 +979,7 @@ def apply_iter(self, expr, i, imin, imax, di, evaluation):
             index = Expression(SymbolPlus, index, di).evaluate(evaluation)
         return self.get_result(result)
 
-    def apply_list(self, expr, i, items, evaluation):
+    def eval_list(self, expr, i, items, evaluation):
         "%(name)s[expr_, {i_Symbol, {items___}}]"
         items = items.evaluate(evaluation).get_sequence()
         result = []
@@ -1003,7 +1005,7 @@ def apply_list(self, expr, i, items, evaluation):
                     raise
         return self.get_result(result)
 
-    def apply_multi(self, expr, first, sequ, evaluation):
+    def eval_multi(self, expr, first, sequ, evaluation):
         "%(name)s[expr_, first_, sequ__]"
 
         sequ = sequ.get_sequence()
@@ -1027,7 +1029,7 @@ class Insert(Builtin):
 
     summary_text = "insert an element at a given position"
 
-    def apply(self, expr, elem, n: Integer, evaluation):
+    def eval(self, expr, elem, n: Integer, evaluation):
         "Insert[expr_List, elem_, n_Integer]"
 
         py_n = n.value
@@ -1232,7 +1234,7 @@ class TakeLargestBy(_RankedTakeLargest):
 
     summary_text = "sublist of n largest elements according to a given criteria"
 
-    def apply(self, element, f, n, evaluation, options):
+    def eval(self, element, f, n, evaluation, options):
         "TakeLargestBy[element_List, f_, n_, OptionsPattern[TakeLargestBy]]"
         return self._compute(element, n, evaluation, options, f=f)
 
@@ -1256,7 +1258,7 @@ class TakeSmallestBy(_RankedTakeSmallest):
 
     summary_text = "sublist of n largest elements according to a criteria"
 
-    def apply(self, element, f, n, evaluation, options):
+    def eval(self, element, f, n, evaluation, options):
         "TakeSmallestBy[element_List, f_, n_, OptionsPattern[TakeSmallestBy]]"
         return self._compute(element, n, evaluation, options, f=f)
 
@@ -1416,7 +1418,7 @@ def levels(k):
             )
             return None
 
-    def apply_zero(self, element, n, evaluation):
+    def eval_zero(self, element, n, evaluation):
         "%(name)s[element_, n_]"
         return self._pad(
             element,
@@ -1427,7 +1429,7 @@ def apply_zero(self, element, n, evaluation):
             lambda: Expression(self.get_name(), element, n),
         )
 
-    def apply(self, element, n, x, evaluation):
+    def eval(self, element, n, x, evaluation):
         "%(name)s[element_, n_, x_]"
         return self._pad(
             element,
@@ -1438,7 +1440,7 @@ def apply(self, element, n, x, evaluation):
             lambda: Expression(self.get_name(), element, n, x),
         )
 
-    def apply_margin(self, element, n, x, m, evaluation):
+    def eval_margin(self, element, n, x, m, evaluation):
         "%(name)s[element_, n_, x_, m_]"
         return self._pad(
             element,
@@ -1820,7 +1822,7 @@ class FindClusters(_Cluster):
 
     summary_text = "divide data into lists of similar elements"
 
-    def apply(self, p, evaluation, options):
+    def eval(self, p, evaluation, options):
         "FindClusters[p_, OptionsPattern[%(name)s]]"
         return self._cluster(
             p,
@@ -1831,7 +1833,7 @@ def apply(self, p, evaluation, options):
             Expression(SymbolFindClusters, p, *options_to_rules(options)),
         )
 
-    def apply_manual_k(self, p, k: Integer, evaluation, options):
+    def eval_manual_k(self, p, k: Integer, evaluation, options):
         "FindClusters[p_, k_Integer, OptionsPattern[%(name)s]]"
         return self._cluster(
             p,
@@ -1867,7 +1869,7 @@ class ClusteringComponents(_Cluster):
 
     summary_text = "label data with the index of the cluster it is in"
 
-    def apply(self, p, evaluation, options):
+    def eval(self, p, evaluation, options):
         "ClusteringComponents[p_, OptionsPattern[%(name)s]]"
         return self._cluster(
             p,
@@ -1878,7 +1880,7 @@ def apply(self, p, evaluation, options):
             Expression(SymbolClusteringComponents, p, *options_to_rules(options)),
         )
 
-    def apply_manual_k(self, p, k: Integer, evaluation, options):
+    def eval_manual_k(self, p, k: Integer, evaluation, options):
         "ClusteringComponents[p_, k_Integer, OptionsPattern[%(name)s]]"
         return self._cluster(
             p,
@@ -1941,7 +1943,7 @@ class Nearest(Builtin):
     }
     summary_text = "the nearest element from a list"
 
-    def apply(self, items, pivot, limit, expression, evaluation, options):
+    def eval(self, items, pivot, limit, expression, evaluation, options):
         "Nearest[items_, pivot_, limit_, OptionsPattern[%(name)s]]"
 
         method = self.get_option(options, "Method", evaluation)
@@ -2078,6 +2080,8 @@ class SubsetQ(Builtin):
     """
 
     messages = {
+        # FIXME: This message doesn't exist in more modern WMA, and
+        # Subset *can* take more than 2 arguments.
         "argr": "SubsetQ called with 1 argument; 2 arguments are expected.",
         "argrx": "SubsetQ called with `1` arguments; 2 arguments are expected.",
         "heads": "Heads `1` and `2` at positions 1 and 2 are expected to be the same.",
@@ -2085,7 +2089,7 @@ class SubsetQ(Builtin):
     }
     summary_text = "test if a list is a subset of another list"
 
-    def apply(self, expr, subset, evaluation):
+    def eval(self, expr, subset, evaluation):
         "SubsetQ[expr_, subset___]"
 
         if isinstance(expr, Atom):
diff --git a/mathics/builtin/patterns.py b/mathics/builtin/patterns.py
index 7faba7731..72fae8e61 100644
--- a/mathics/builtin/patterns.py
+++ b/mathics/builtin/patterns.py
@@ -975,7 +975,6 @@ class Pattern_(PatternObject):
         "nodef": (
             "No default setting found for `1` in " "position `2` when length is `3`."
         ),
-        "argr": "Pattern called with 1 argument; 2 arguments are expected.",
     }
 
     rules = {
diff --git a/test/builtin/test_attributes.py b/test/builtin/test_attributes.py
index 175deeaeb..41f76df66 100644
--- a/test/builtin/test_attributes.py
+++ b/test/builtin/test_attributes.py
@@ -121,7 +121,7 @@ def test_one_identity(str_expr, str_expected, msg):
     ],
 )
 @skip_or_fail
-def test_one_identity_stil_failing(str_expr, str_expected, msg):
+def test_one_identity_still_failing(str_expr, str_expected, msg):
     check_evaluation(
         str_expr,
         str_expected,
@@ -204,3 +204,25 @@ def test_one_identity_stil_failing(str_expr, str_expected, msg):
         hold_expected=True,
         failure_message=msg,
     )
+
+
+@pytest.mark.parametrize(
+    ("str_expr", "arg_count"),
+    [
+        ("SetAttributes[F]", 1),
+        ("SetAttributes[]", 0),
+        ("SetAttributes[F, F, F]", 3),
+    ],
+)
+def test_Attributes_wrong_args(str_expr, arg_count):
+    check_evaluation(
+        str_expr=str_expr,
+        str_expected=str_expr,
+        failure_message=f"Arg count mismatch test with {arg_count} args",
+        hold_expected=True,
+        to_string_expr=True,
+        to_string_expected=True,
+        expected_messages=(
+            f"SetAttributes called with {arg_count} arguments; 2 arguments are expected.",
+        ),
+    )

From c1b5fa3dda1167061e16024fccc43c0386188cc2 Mon Sep 17 00:00:00 2001
From: mmatera <matera@fisica.unlp.edu.ar>
Date: Sun, 13 Nov 2022 14:20:10 -0300
Subject: [PATCH 2/7] fix get_sort_key second argument and BoxExpression.sameQ.
 With this, SyntaxAnnotation.m does not kill the interpreter

---
 mathics/builtin/base.py     | 4 ++--
 mathics/builtin/patterns.py | 2 +-
 mathics/core/element.py     | 2 +-
 mathics/core/evaluation.py  | 2 +-
 mathics/core/rules.py       | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/mathics/builtin/base.py b/mathics/builtin/base.py
index b6faf3aac..2e1cbd942 100644
--- a/mathics/builtin/base.py
+++ b/mathics/builtin/base.py
@@ -818,7 +818,7 @@ def get_head_name(self):
     def get_lookup_name(self):
         return self.get_name()
 
-    def get_sort_key(self) -> tuple:
+    def get_sort_key(self, pattern_sort=False) -> tuple:
         return self.to_expression().get_sort_key()
 
     def get_string_value(self):
@@ -826,7 +826,7 @@ def get_string_value(self):
 
     def sameQ(self, expr) -> bool:
         """Mathics SameQ"""
-        return expr.sameQ(self)
+        return self.to_expression().sameQ(expr)
 
     def do_format(self, evaluation, format):
         return self
diff --git a/mathics/builtin/patterns.py b/mathics/builtin/patterns.py
index 7faba7731..0527c537e 100644
--- a/mathics/builtin/patterns.py
+++ b/mathics/builtin/patterns.py
@@ -1691,7 +1691,7 @@ def __init__(self, rulelist, evaluation):
         self._elements = None
         self._head = SymbolDispatch
 
-    def get_sort_key(self) -> tuple:
+    def get_sort_key(self, pattern_sort=False) -> tuple:
         return self.src.get_sort_key()
 
     def get_atom_name(self):
diff --git a/mathics/core/element.py b/mathics/core/element.py
index 26f525311..f061081b6 100644
--- a/mathics/core/element.py
+++ b/mathics/core/element.py
@@ -147,7 +147,7 @@ class KeyComparable:
     # FIXME: return type should be a specific kind of Tuple, not a list.
     # FIXME: Describe sensible, and easy to follow rules by which one
     #        can create the kind of tuple for some new kind of element.
-    def get_sort_key(self) -> list:
+    def get_sort_key(self, pattern_sort=False) -> list:
         """
         This returns a tuple in a way that
         it can be used to compare in expressions.
diff --git a/mathics/core/evaluation.py b/mathics/core/evaluation.py
index 3ac098309..212bb15b0 100644
--- a/mathics/core/evaluation.py
+++ b/mathics/core/evaluation.py
@@ -157,7 +157,7 @@ def __init__(self) -> None:
         self.is_print = False
         self.text = ""
 
-    def get_sort_key(self) -> Tuple[bool, bool, str]:
+    def get_sort_key(self, pattern_sort=False) -> Tuple[bool, bool, str]:
         return (self.is_message, self.is_print, self.text)
 
 
diff --git a/mathics/core/rules.py b/mathics/core/rules.py
index 717f9d64c..dac7426d0 100644
--- a/mathics/core/rules.py
+++ b/mathics/core/rules.py
@@ -107,7 +107,7 @@ def yield_match(vars, rest):
     def do_replace(self):
         raise NotImplementedError
 
-    def get_sort_key(self) -> tuple:
+    def get_sort_key(self, pattern_sort=False) -> tuple:
         # FIXME: check if this makes sense:
         return tuple((self.system, self.pattern.get_sort_key(True)))
 

From 475cb9d4076becf41d5237662a56c790b4244daa Mon Sep 17 00:00:00 2001
From: mmatera <matera@fisica.unlp.edu.ar>
Date: Sun, 13 Nov 2022 16:26:44 -0300
Subject: [PATCH 3/7] adding test

---
 test/builtin/test_assignment.py | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/test/builtin/test_assignment.py b/test/builtin/test_assignment.py
index 3af7020c4..9c6fc797c 100644
--- a/test/builtin/test_assignment.py
+++ b/test/builtin/test_assignment.py
@@ -289,6 +289,34 @@ def test_set_and_clear(str_expr, str_expected, msg):
             "{F[2.], 4.}",
             "Assign N rule",
         ),
+        (None, None, None),
+        # This test is inspirated in CellsToTeX
+        ("SetAttributes[testHoldAll, HoldAll]", "Null", None),
+        (
+            (
+                "addF[sym_Symbol] := ("
+                "        functionCall:sym[___] := "
+                "            holdallfunc[functionCall]"
+                "                    )"
+            ),
+            "Null",
+            None,
+        ),
+        ("addF[Q]", "Null", None),
+        # This fails because the previous line does not set the
+        # rule as it should.
+        #
+        # In WMA,
+        #
+        # In[...]:=Downvalues[Q]                                                        # Out[...]=  {HoldPattern[functionCall:Q[___]] :> testHoldAll[functionCall]}
+        #
+        #
+        # In Mathics,
+        #
+        # Out[...]={HoldPattern[Q[___]] :> testHoldAll[Q[___]]}
+        #
+        #
+        ("Q[1]", "holdallfunc[Q[1]]", None),
     ],
 )
 @skip_or_fail

From 1f87088757e03337f03a5a0f283795e4768b2079 Mon Sep 17 00:00:00 2001
From: mmatera <matera@fisica.unlp.edu.ar>
Date: Mon, 14 Nov 2022 14:13:00 -0300
Subject: [PATCH 4/7] simplifying assignment. Now all the tests pass.

---
 mathics/builtin/assignments/internals.py | 110 ++++++++---------------
 mathics/core/definitions.py              |  27 +++++-
 test/builtin/test_assignment.py          |  47 +---------
 3 files changed, 62 insertions(+), 122 deletions(-)

diff --git a/mathics/builtin/assignments/internals.py b/mathics/builtin/assignments/internals.py
index f00e708d9..d34c0e1fa 100644
--- a/mathics/builtin/assignments/internals.py
+++ b/mathics/builtin/assignments/internals.py
@@ -4,6 +4,7 @@
 
 from mathics.algorithm.parts import walk_parts
 from mathics.core.atoms import Atom, Integer
+from mathics.core.definitions import get_tag_position
 from mathics.core.element import BaseElement
 from mathics.core.evaluation import MAX_RECURSION_DEPTH, set_python_recursion_limit
 from mathics.core.expression import Expression, SymbolDefault
@@ -112,19 +113,16 @@ def assign_store_rules_by_tag(self, lhs, rhs, evaluation, tags, upset=None):
     lhs and rhs are rewritten in a normal form, where
     conditions are associated to the lhs.
     """
-    lhs, condition = unroll_conditions(lhs)
-    lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
 
+    lhs, condition = unroll_conditions(lhs)
     defs = evaluation.definitions
     ignore_protection, tags = process_assign_other(
         self, lhs, rhs, evaluation, tags, upset
     )
-
     # In WMA, this does not happens. However, if we remove this,
     # some combinatorica tests fail.
     # Also, should not be at the begining?
     lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
-
     count = 0
     rule = Rule(lhs, rhs)
     position = "up" if upset else None
@@ -549,9 +547,7 @@ def process_assign_n(self, lhs, rhs, evaluation, tags, upset):
         nprec = lhs.elements[1]
     focus = lhs.elements[0]
     lhs = Expression(SymbolN, focus, nprec)
-    tags = process_tags_and_upset_dont_allow_custom(
-        tags, upset, self, lhs, focus, evaluation
-    )
+    tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation, focus)
     count = 0
     lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
     rule = Rule(lhs, rhs)
@@ -575,9 +571,7 @@ def process_assign_other(
     True, meaning that the `Protected` attribute should not be taken into accout.
     Otherwise, the value is False.
     """
-    tags, focus = process_tags_and_upset_allow_custom(
-        tags, upset, self, lhs, evaluation
-    )
+    tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation)
 
     lhs_name = lhs.get_name()
     if lhs_name == "System`$RecursionLimit":
@@ -652,9 +646,7 @@ def process_assign_default(self, lhs, rhs, evaluation, tags, upset):
         evaluation.message_args(SymbolDefault, len(lhs.elements), 1, 2, 3)
         raise AssignmentException(lhs, None)
     focus = lhs.elements[0]
-    tags = process_tags_and_upset_dont_allow_custom(
-        tags, upset, self, lhs, focus, evaluation
-    )
+    tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation, focus)
     lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
     rule = Rule(lhs, rhs)
     for tag in tags:
@@ -694,9 +686,7 @@ def process_assign_format(self, lhs, rhs, evaluation, tags, upset):
             "System`MathMLForm",
         ]
     lhs = focus = lhs.elements[0]
-    tags = process_tags_and_upset_dont_allow_custom(
-        tags, upset, self, lhs, focus, evaluation
-    )
+    tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation, focus)
     lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
     rule = Rule(lhs, rhs)
     for tag in tags:
@@ -771,9 +761,7 @@ def process_assign_messagename(self, lhs, rhs, evaluation, tags, upset):
         evaluation.message_args("MessageName", len(lhs.elements), 2)
         raise AssignmentException(lhs, None)
     focus = lhs.elements[0]
-    tags = process_tags_and_upset_dont_allow_custom(
-        tags, upset, self, lhs, focus, evaluation
-    )
+    tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation, focus)
     lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
     rule = Rule(lhs, rhs)
     for tag in tags:
@@ -823,7 +811,7 @@ def find_tag(focus):
         # the message
         # "HoldPattern::argx: HoldPattern called with 2 arguments; 1 argument is expected."
         # must be shown.
-        raise AssignmentException(lhs, None)
+        raise AssignmentException(focus, None)
     if name == "System`Optional":
         return None
     if name == "System`Pattern" and len(focus.elements) == 2:
@@ -842,7 +830,14 @@ def find_tag(focus):
     return name
 
 
-def process_tags_and_upset_dont_allow_custom(tags, upset, self, lhs, focus, evaluation):
+def process_tags_and_upset(tags, upset, self, lhs, evaluation, focus=None):
+    if focus is None:
+        allow_custom = True
+        focus = lhs
+    else:
+        allow_custom = False
+
+    name = lhs.get_head_name()
 
     if (
         isinstance(focus, Expression)
@@ -850,7 +845,6 @@ def process_tags_and_upset_dont_allow_custom(tags, upset, self, lhs, focus, eval
     ):
         focus = focus.evaluate_elements(evaluation)
 
-    name = lhs.get_head_name()
     if tags is None and not upset:
         name = find_tag(focus)
         if name == "":
@@ -858,59 +852,28 @@ def process_tags_and_upset_dont_allow_custom(tags, upset, self, lhs, focus, eval
             raise AssignmentException(lhs, None)
         tags = [] if name is None else [name]
     elif upset:
-        name = find_tag(focus)
-        tags = [] if name is None else [name]
-    else:
-        name = find_tag(focus)
-        allowed_names = [] if name is None else [name]
-        for name in tags:
-            if name not in allowed_names:
-                evaluation.message(self.get_name(), "tagnfd", Symbol(name))
+        if allow_custom:
+            tags = []
+            if isinstance(focus, Atom):
+                evaluation.message(self.get_name(), "normal")
                 raise AssignmentException(lhs, None)
-
-    if len(tags) == 0:
-        evaluation.message(self.get_name(), "nosym", focus)
-        raise AssignmentException(lhs, None)
-    return tags
-
-
-def process_tags_and_upset_allow_custom(tags, upset, self, lhs, evaluation):
-    name = lhs.get_head_name()
-    focus = lhs
-
-    if isinstance(focus, Expression) and focus.head not in (
-        SymbolSet,
-        SymbolSetDelayed,
-        SymbolUpSet,
-        SymbolUpSetDelayed,
-        SymbolTagSet,
-        SymbolTagSetDelayed,
-        SymbolList,
-        SymbolPart,
-    ):
-        focus = focus.evaluate_elements(evaluation)
-
-    if tags is None and not upset:
-        name = find_tag(focus)
-        if name == "":
-            evaluation.message(self.get_name(), "setraw", focus)
-            raise AssignmentException(lhs, None)
-        tags = [] if name is None else [name]
-    elif upset:
-        tags = []
-        if isinstance(focus, Atom):
-            evaluation.message(self.get_name(), "normal")
-            raise AssignmentException(lhs, None)
-        for element in focus.elements:
-            name = find_tag(element)
-            if name != "" and name is not None:
-                tags.append(name)
+            for element in focus.elements:
+                name = find_tag(element)
+                if name != "" and name is not None:
+                    tags.append(name)
+        else:
+            name = find_tag(focus)
+            tags = [] if name is None else [name]
     else:
-        allowed_names = [find_tag(focus)]
-        for element in focus.get_elements():
-            element_tag = find_tag(element)
-            if element_tag is not None and element_tag != "":
-                allowed_names.append(element_tag)
+        if allow_custom:
+            allowed_names = [find_tag(focus)]
+            for element in focus.get_elements():
+                element_tag = find_tag(element)
+                if element_tag is not None and element_tag != "":
+                    allowed_names.append(element_tag)
+        else:
+            name = find_tag(focus)
+            allowed_names = [] if name is None else [name]
         for name in tags:
             if name not in allowed_names:
                 evaluation.message(self.get_name(), "tagnfd", Symbol(name))
@@ -918,7 +881,6 @@ def process_tags_and_upset_allow_custom(tags, upset, self, lhs, evaluation):
     if len(tags) == 0:
         evaluation.message(self.get_name(), "nosym", focus)
         raise AssignmentException(lhs, None)
-
     return tags, focus
 
 
diff --git a/mathics/core/definitions.py b/mathics/core/definitions.py
index def6b1a93..b3c7df20a 100644
--- a/mathics/core/definitions.py
+++ b/mathics/core/definitions.py
@@ -16,6 +16,7 @@
 from mathics.core.convert.expression import to_mathics_list
 from mathics.core.element import fully_qualified_symbol_name
 from mathics.core.expression import Expression
+from mathics.core.pattern import Pattern
 from mathics.core.symbols import (
     Atom,
     Symbol,
@@ -780,6 +781,25 @@ def get_history_length(self):
 
 
 def get_tag_position(pattern, name) -> Optional[str]:
+    # Strip first the pattern from HoldPattern, Pattern
+    # and Condition wrappings
+    while True:
+        # TODO: Not Atom/Expression,
+        # pattern -> pattern.to_expression()
+        if isinstance(pattern, Pattern):
+            pattern = pattern.expr
+            continue
+        if pattern.has_form("System`HoldPattern", 1):
+            pattern = pattern.elements[0]
+            continue
+        if pattern.has_form("System`Pattern", 2):
+            pattern = pattern.elements[1]
+            continue
+        if pattern.has_form("System`Condition", 2):
+            pattern = pattern.elements[0]
+            continue
+        break
+
     if pattern.get_name() == name:
         return "own"
     elif isinstance(pattern, Atom):
@@ -788,10 +808,8 @@ def get_tag_position(pattern, name) -> Optional[str]:
         head_name = pattern.get_head_name()
         if head_name == name:
             return "down"
-        elif head_name == "System`N" and len(pattern.elements) == 2:
+        elif pattern.has_form("System`N", 2):
             return "n"
-        elif head_name == "System`Condition" and len(pattern.elements) > 0:
-            return get_tag_position(pattern.elements[0], name)
         elif pattern.get_lookup_name() == name:
             return "sub"
         else:
@@ -912,6 +930,9 @@ def remove_rule(self, lhs) -> bool:
         return False
 
     def __repr__(self) -> str:
+        print("arguments of ", id(self))
+        for p in (self.name, self.downvalues, self.formatvalues, self.attributes):
+            print(p)
         s = "<Definition: name: {}, downvalues: {}, formats: {}, attributes: {}>".format(
             self.name, self.downvalues, self.formatvalues, self.attributes
         )
diff --git a/test/builtin/test_assignment.py b/test/builtin/test_assignment.py
index 9c6fc797c..1aaef090b 100644
--- a/test/builtin/test_assignment.py
+++ b/test/builtin/test_assignment.py
@@ -5,13 +5,6 @@
 from test.helper import check_evaluation, session
 from mathics_scanner.errors import IncompleteSyntaxError
 
-DEBUGASSIGN = int(os.environ.get("DEBUGSET", "0")) == 1
-
-if DEBUGASSIGN:
-    skip_or_fail = pytest.mark.xfail
-else:
-    skip_or_fail = pytest.mark.skip
-
 
 str_test_set_with_oneidentity = """
 SetAttributes[SUNIndex, {OneIdentity}];
@@ -215,28 +208,6 @@ def test_setdelayed_oneidentity():
             None,
             None,
         ),
-    ],
-)
-def test_set_and_clear(str_expr, str_expected, msg):
-    """
-    Test calls to Set, Clear and ClearAll. If
-    str_expr is None, the session is reset,
-    in a way that the next test run over a fresh
-    environment.
-    """
-    check_evaluation(
-        str_expr,
-        str_expected,
-        to_string_expr=True,
-        to_string_expected=True,
-        hold_expected=True,
-        failure_message=msg,
-    )
-
-
-@pytest.mark.parametrize(
-    ("str_expr", "str_expected", "msg"),
-    [
         (None, None, None),
         (r"a=b; a=4; {a, b}", "{4, b}", None),
         (None, None, None),
@@ -276,7 +247,7 @@ def test_set_and_clear(str_expr, str_expected, msg):
         (None, None, None),
         (
             (
-                "A[x_]:=B[x];B[x_]:=F[x_];F[x_]:=G[x];"
+                "A[x_]:=B[x];B[x_]:=F[x];F[x_]:=G[x];"
                 "H[A[y_]]:=Q[y]; ClearAll[F];"
                 "{H[A[5]],H[B[5]],H[F[5]],H[G[5]]}"
             ),
@@ -303,24 +274,10 @@ def test_set_and_clear(str_expr, str_expected, msg):
             None,
         ),
         ("addF[Q]", "Null", None),
-        # This fails because the previous line does not set the
-        # rule as it should.
-        #
-        # In WMA,
-        #
-        # In[...]:=Downvalues[Q]                                                        # Out[...]=  {HoldPattern[functionCall:Q[___]] :> testHoldAll[functionCall]}
-        #
-        #
-        # In Mathics,
-        #
-        # Out[...]={HoldPattern[Q[___]] :> testHoldAll[Q[___]]}
-        #
-        #
         ("Q[1]", "holdallfunc[Q[1]]", None),
     ],
 )
-@skip_or_fail
-def test_set_and_clear_to_fix(str_expr, str_expected, msg):
+def test_set_and_clear(str_expr, str_expected, msg):
     """
     Test calls to Set, Clear and ClearAll. If
     str_expr is None, the session is reset,

From 784e7236b6d3f016e1b759f4e05f8929f2010f77 Mon Sep 17 00:00:00 2001
From: mmatera <matera@fisica.unlp.edu.ar>
Date: Mon, 14 Nov 2022 18:00:19 -0300
Subject: [PATCH 5/7] simplify and improve assign internals

---
 mathics/builtin/assignments/internals.py | 93 ++++++++++++++----------
 1 file changed, 56 insertions(+), 37 deletions(-)

diff --git a/mathics/builtin/assignments/internals.py b/mathics/builtin/assignments/internals.py
index d34c0e1fa..2d134c7a4 100644
--- a/mathics/builtin/assignments/internals.py
+++ b/mathics/builtin/assignments/internals.py
@@ -9,6 +9,7 @@
 from mathics.core.evaluation import MAX_RECURSION_DEPTH, set_python_recursion_limit
 from mathics.core.expression import Expression, SymbolDefault
 from mathics.core.list import ListExpression
+from mathics.core.pattern import Pattern
 from mathics.core.rules import Rule
 from mathics.core.symbols import (
     Symbol,
@@ -113,21 +114,25 @@ def assign_store_rules_by_tag(self, lhs, rhs, evaluation, tags, upset=None):
     lhs and rhs are rewritten in a normal form, where
     conditions are associated to the lhs.
     """
-
-    lhs, condition = unroll_conditions(lhs)
+    condition = None
     defs = evaluation.definitions
-    ignore_protection, tags = process_assign_other(
-        self, lhs, rhs, evaluation, tags, upset
+    tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation)
+    # TODO: check if we can invert the order, and call this just
+    # in the special cases
+    ignore_protection = process_assign_side_effects(
+        self, lhs, rhs, focus, evaluation, tags, upset
     )
     # In WMA, this does not happens. However, if we remove this,
     # some combinatorica tests fail.
     # Also, should not be at the begining?
-    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
+    lhs, rhs = process_rhs_conditions(lhs, rhs, evaluation)
     count = 0
     rule = Rule(lhs, rhs)
     position = "up" if upset else None
     for tag in tags:
-        if rejected_because_protected(self, lhs, tag, evaluation, ignore_protection):
+        if not ignore_protection and rejected_because_protected(
+            self, lhs, tag, evaluation
+        ):
             continue
         count += 1
         defs.add_rule(tag, rule, position=position)
@@ -241,17 +246,6 @@ def repl_pattern_by_symbol(expr):
 # Auxiliary routines
 
 
-def rejected_because_protected(self, lhs, tag, evaluation, ignore=False):
-    defs = evaluation.definitions
-    if not ignore and is_protected(tag, defs):
-        if lhs.get_name() == tag:
-            evaluation.message(self.get_name(), "wrsym", Symbol(tag))
-        else:
-            evaluation.message(self.get_name(), "write", Symbol(tag), lhs)
-        return True
-    return False
-
-
 def find_tag_and_check(lhs, tags, evaluation):
     name = lhs.get_head_name()
     if len(lhs.elements) != 1:
@@ -270,6 +264,17 @@ def find_tag_and_check(lhs, tags, evaluation):
     return tag
 
 
+def rejected_because_protected(self, lhs, tag, evaluation):
+    defs = evaluation.definitions
+    if is_protected(tag, defs):
+        if lhs.get_name() == tag:
+            evaluation.message(self.get_name(), "wrsym", Symbol(tag))
+        else:
+            evaluation.message(self.get_name(), "write", Symbol(tag), lhs)
+        return True
+    return False
+
+
 def unroll_patterns(lhs, rhs, evaluation) -> Tuple[BaseElement, BaseElement]:
     """
     Pattern[symb, pat]=rhs -> pat = (rhs/.(symb->pat))
@@ -507,7 +512,6 @@ def process_assign_options(self, lhs, rhs, evaluation, tags, upset):
 
 
 def process_assign_numericq(self, lhs, rhs, evaluation, tags, upset):
-    # lhs, condition = unroll_conditions(lhs)
     lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
     if rhs not in (SymbolTrue, SymbolFalse):
         evaluation.message("NumericQ", "set", lhs, rhs)
@@ -549,7 +553,7 @@ def process_assign_n(self, lhs, rhs, evaluation, tags, upset):
     lhs = Expression(SymbolN, focus, nprec)
     tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation, focus)
     count = 0
-    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
+    lhs, rhs = process_rhs_conditions(lhs, rhs, evaluation, condition)
     rule = Rule(lhs, rhs)
     for tag in tags:
         if rejected_because_protected(self, lhs, tag, evaluation):
@@ -559,19 +563,18 @@ def process_assign_n(self, lhs, rhs, evaluation, tags, upset):
     return count > 0
 
 
-def process_assign_other(
-    self, lhs, rhs, evaluation, tags=None, upset=False
-) -> Tuple[bool, list]:
+def process_assign_side_effects(
+    self, lhs, rhs, focus, evaluation, tags=None, upset=False
+) -> bool:
     """
     Process special cases, performing certain side effects, like modifying
     the value of internal variables that are not stored as rules.
 
-    The function returns a tuple of a bool value and a list of tags.
+    The function returns a a bool value.
     If lhs is one of the special cases, then the bool variable is
-    True, meaning that the `Protected` attribute should not be taken into accout.
+    True, meaning that the `Protected` attribute should not be taken into account.
     Otherwise, the value is False.
     """
-    tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation)
 
     lhs_name = lhs.get_name()
     if lhs_name == "System`$RecursionLimit":
@@ -589,8 +592,8 @@ def process_assign_other(
     elif lhs_name == "System`$MaxPrecision":
         process_assign_maxprecision(self, lhs, rhs, evaluation, tags, upset)
     else:
-        return False, tags
-    return True, tags
+        return False
+    return True
 
 
 def process_assign_attributes(self, lhs, rhs, evaluation, tags, upset):
@@ -647,7 +650,7 @@ def process_assign_default(self, lhs, rhs, evaluation, tags, upset):
         raise AssignmentException(lhs, None)
     focus = lhs.elements[0]
     tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation, focus)
-    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
+    lhs, rhs = process_rhs_conditions(lhs, rhs, evaluation, condition)
     rule = Rule(lhs, rhs)
     for tag in tags:
         if rejected_because_protected(self, lhs, tag, evaluation):
@@ -687,7 +690,7 @@ def process_assign_format(self, lhs, rhs, evaluation, tags, upset):
         ]
     lhs = focus = lhs.elements[0]
     tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation, focus)
-    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
+    lhs, rhs = process_rhs_conditions(lhs, rhs, evaluation, condition)
     rule = Rule(lhs, rhs)
     for tag in tags:
         if rejected_because_protected(self, lhs, tag, evaluation):
@@ -762,7 +765,7 @@ def process_assign_messagename(self, lhs, rhs, evaluation, tags, upset):
         raise AssignmentException(lhs, None)
     focus = lhs.elements[0]
     tags, focus = process_tags_and_upset(tags, upset, self, lhs, evaluation, focus)
-    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
+    lhs, rhs = process_rhs_conditions(lhs, rhs, evaluation, condition)
     rule = Rule(lhs, rhs)
     for tag in tags:
         # Messages can be assigned even if the symbol is protected...
@@ -773,7 +776,7 @@ def process_assign_messagename(self, lhs, rhs, evaluation, tags, upset):
     return count > 0
 
 
-def process_rhs_conditions(lhs, rhs, condition, evaluation):
+def process_rhs_conditions(lhs, rhs, evaluation, condition=None):
     """
     lhs = Condition[rhs, test] -> Condition[lhs, test]  = rhs
     """
@@ -802,8 +805,15 @@ def process_rhs_conditions(lhs, rhs, condition, evaluation):
     return lhs, rhs
 
 
-def find_tag(focus):
+def find_focus(focus):
+    """
+    Recursively, look for the (true) focus expression, i.e.,
+    the expression after strip it from Condition, Pattern and HoldPattern
+    wrapping expressions.
+    """
     name = focus.get_lookup_name()
+    if isinstance(focus, Pattern):
+        return find_focus(focus.expr)
     if name == "System`HoldPattern":
         if len(focus.elements) == 1:
             return find_tag(focus.elements[0])
@@ -812,6 +822,8 @@ def find_tag(focus):
         # "HoldPattern::argx: HoldPattern called with 2 arguments; 1 argument is expected."
         # must be shown.
         raise AssignmentException(focus, None)
+    if focus.has_form("System`Condition", 2):
+        return find_focus(focus.elements[0])
     if name == "System`Optional":
         return None
     if name == "System`Pattern" and len(focus.elements) == 2:
@@ -824,10 +836,17 @@ def find_tag(focus):
             elems = pat.elements
             if len(elems) == 0:
                 return None
-            return find_tag(elems[0])
+            return find_focus(elems[0])
         else:
-            return find_tag(pat)
-    return name
+            return find_focus(pat)
+    return focus
+
+
+def find_tag(focus):
+    focus = find_focus(focus)
+    if focus is None:
+        return None
+    return focus.get_lookup_name()
 
 
 def process_tags_and_upset(tags, upset, self, lhs, evaluation, focus=None):
@@ -837,8 +856,8 @@ def process_tags_and_upset(tags, upset, self, lhs, evaluation, focus=None):
     else:
         allow_custom = False
 
-    name = lhs.get_head_name()
-
+    # Ensures that focus is the actual focus of the expression.
+    focus = find_focus(focus)
     if (
         isinstance(focus, Expression)
         and focus.head not in NOT_EVALUATE_ELEMENTS_IN_ASSIGNMENTS

From c216a93b986a20f43bbba0ba5e10f28daf40a9a3 Mon Sep 17 00:00:00 2001
From: mmatera <matera@fisica.unlp.edu.ar>
Date: Mon, 14 Nov 2022 18:19:53 -0300
Subject: [PATCH 6/7] comment on process_rhs_conditions

---
 mathics/builtin/assignments/internals.py | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/mathics/builtin/assignments/internals.py b/mathics/builtin/assignments/internals.py
index 2d134c7a4..4102c704f 100644
--- a/mathics/builtin/assignments/internals.py
+++ b/mathics/builtin/assignments/internals.py
@@ -779,6 +779,27 @@ def process_assign_messagename(self, lhs, rhs, evaluation, tags, upset):
 def process_rhs_conditions(lhs, rhs, evaluation, condition=None):
     """
     lhs = Condition[rhs, test] -> Condition[lhs, test]  = rhs
+
+    This function is necesary as a temporal patch for a bug
+    in the pattern matching / replacement code.
+
+    In WMA,
+
+    ``f[4]/.(f[x_]:> (p[x]/;x>0))``
+
+    shold return
+
+    ``p[4]``
+
+    while ``f[-4]/.(f[x_]:> (p[x]/;x>0))``
+    would result in ``f[-4]``
+
+    In Mathics, the first case results in
+    ``p[4]/; 4>0``
+    while the second,
+    ``p[-4]/; -4>0``
+
+    This should be handled in the apply method of the BaseRule class.
     """
     # To Handle `OptionValue` in `Condition`
     rulopc = build_rulopc(lhs.get_head())

From 2ffdf024f3427f78d74cdd092868b58c8b055ea6 Mon Sep 17 00:00:00 2001
From: Juan Mauricio Matera <matera@fisica.unlp.edu.ar>
Date: Tue, 15 Nov 2022 01:08:32 -0300
Subject: [PATCH 7/7] Correct LHS evaluation in SetDelayed assignment (#603)

* improving clarity in Builtin.contribute

* adding comments. adding tests

* fix test for SetDelayed. Move special case for Set with LHS a list away from the conditional.

* more on modularize assignment. assign_elementary->assign

* fixing set_eval

* adding tests for OneIdentity

* fix OneIdentity

* remove comment

* fix pytest

* fix a bug that makes that URLSave always fails

* fix WriteString standard output

* catch not known attributes in ClearAttributes and SetAttributes

* Update 'attributes' for current standards

`mathics.builtin.attributes`:

* apply -> eval
* Add WMA links add
* Class names ordered alphabetically
* Hard breaks in docstring removed
* Some incorrect references to "leaves" changed to "attributes"

`mathics.core.attributes`:

* Add short comments above flag value

* adding comments. adding tests

* fix test for SetDelayed. Move special case for Set with LHS a list away from the conditional.

* more on modularize assignment. assign_elementary->assign

* Handle optional with a first element that is not a Pattern[]

* Update examples in OneIdentity

* More pervasive use of Symbols

symbols.py:
   system_symbols() -> symbol_set(); "systems_symbols" name is too close
Symbol( System`  to module systemsymbols. Also, we now require symbols as parameters),
   not strings.

systemsymbols.py: more system symbols

* Pattern_create -> Pattern.create

It appears this was originally Pattern.create. I suspect due to bad
modularity and a lack of understandig Python that an import could be added inside the
routine, this static method got moved outside of the class.

Later on, the modularity was fixed, but the hack persisted. These kinds
of code smells side effects of poor communication.

* Add function signature; straighten import issue

* Put test_rules_patterns tests where they belong

* Add note to add skipped example as a doctest ...

When it gets fixed.

* Changes suggested in PR review

Move core-like assignment interals out of builtins and into core.
(We may want to split up core more in the future though)

Better sort long list of assignment methods alphabetically

Some small RstT tagging in a docstring

Remove nonexistent word "evaluable"

Add yet another type annotation to a signature

Make test assert failure messages unique

* Move ASSIGNMENT_FUNCTION_MAP into core

Co-authored-by: R. Bernstein <rocky@users.noreply.github.com>
Co-authored-by: rocky <rb@dustyfeet.com>
---
 mathics/builtin/assignments/assignment.py     |  41 +-
 mathics/builtin/assignments/clear.py          |   3 +-
 mathics/builtin/assignments/types.py          |   3 +-
 mathics/builtin/assignments/upvalues.py       |   2 +-
 mathics/builtin/atomic/symbols.py             |   2 +-
 mathics/builtin/attributes.py                 |   2 +-
 mathics/builtin/scoping.py                    |   2 +-
 .../internals.py => core/assignment.py}       | 569 +++++++++---------
 mathics/core/expression.py                    |   4 +
 mathics/core/list.py                          |  28 +-
 mathics/core/systemsymbols.py                 |   1 +
 test/builtin/test_assignment.py               |  52 ++
 12 files changed, 403 insertions(+), 306 deletions(-)
 rename mathics/{builtin/assignments/internals.py => core/assignment.py} (91%)

diff --git a/mathics/builtin/assignments/assignment.py b/mathics/builtin/assignments/assignment.py
index 856942f18..7f3008bf1 100644
--- a/mathics/builtin/assignments/assignment.py
+++ b/mathics/builtin/assignments/assignment.py
@@ -4,8 +4,14 @@
 """
 
 
-from mathics.builtin.assignments.internals import _SetOperator
 from mathics.builtin.base import BinaryOperator, Builtin
+from mathics.core.assignment import (
+    ASSIGNMENT_FUNCTION_MAP,
+    AssignmentException,
+    assign_store_rules_by_tag,
+    normalize_lhs,
+)
+
 from mathics.core.attributes import (
     A_HOLD_ALL,
     A_HOLD_FIRST,
@@ -18,6 +24,39 @@
 from mathics.core.systemsymbols import SymbolFailed
 
 
+class _SetOperator:
+    """
+    This is the base class for assignment Builtin operators.
+
+    Special cases are determined by the head of the expression. Then
+    they are processed by specific routines, which are poke from
+    the ``ASSIGNMENT_FUNCTION_MAP`` dict.
+    """
+
+    # FIXME:
+    # Assigment is determined by the LHS.
+    # Are there a larger patterns or natural groupings that we are missing?
+    # For example, it might be that it
+    # we can key off of some attributes or other properties of the
+    # LHS of a builtin, instead of listing all of the builtins in that class
+    # (which may miss some).
+    # Below, we key on a string, but Symbol is more correct.
+
+    def assign(self, lhs, rhs, evaluation, tags=None, upset=False):
+        lhs, lookup_name = normalize_lhs(lhs, evaluation)
+        try:
+            # Using a builtin name, find which assignment procedure to perform,
+            # and then call that function.
+            assignment_func = ASSIGNMENT_FUNCTION_MAP.get(lookup_name, None)
+            if assignment_func:
+                return assignment_func(self, lhs, rhs, evaluation, tags, upset)
+
+            return assign_store_rules_by_tag(self, lhs, rhs, evaluation, tags, upset)
+        except AssignmentException:
+
+            return False
+
+
 class Set(BinaryOperator, _SetOperator):
     """
     <dl>
diff --git a/mathics/builtin/assignments/clear.py b/mathics/builtin/assignments/clear.py
index e46dd3ab3..8dbda4ff5 100644
--- a/mathics/builtin/assignments/clear.py
+++ b/mathics/builtin/assignments/clear.py
@@ -38,10 +38,9 @@
     SymbolUpValues,
 )
 
+from mathics.core.assignment import is_protected
 from mathics.core.atoms import String
 
-from mathics.builtin.assignments.internals import is_protected
-
 
 class Clear(Builtin):
     """
diff --git a/mathics/builtin/assignments/types.py b/mathics/builtin/assignments/types.py
index 6a5ebb99a..3d340f989 100644
--- a/mathics/builtin/assignments/types.py
+++ b/mathics/builtin/assignments/types.py
@@ -8,8 +8,7 @@
 
 from mathics.builtin.base import Builtin
 
-from mathics.builtin.assignments.internals import get_symbol_values
-
+from mathics.core.assignment import get_symbol_values
 from mathics.core.attributes import A_HOLD_ALL, A_PROTECTED
 
 
diff --git a/mathics/builtin/assignments/upvalues.py b/mathics/builtin/assignments/upvalues.py
index 817a74272..7cecd29cc 100644
--- a/mathics/builtin/assignments/upvalues.py
+++ b/mathics/builtin/assignments/upvalues.py
@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 
 
-from mathics.builtin.assignments.internals import _SetOperator
+from mathics.builtin.assignments.assignment import _SetOperator
 from mathics.builtin.base import BinaryOperator
 from mathics.core.attributes import (
     A_HOLD_ALL,
diff --git a/mathics/builtin/atomic/symbols.py b/mathics/builtin/atomic/symbols.py
index 753e4e649..fe02ed14e 100644
--- a/mathics/builtin/atomic/symbols.py
+++ b/mathics/builtin/atomic/symbols.py
@@ -7,7 +7,6 @@
 
 import re
 
-from mathics.builtin.assignments.internals import get_symbol_values
 from mathics.builtin.base import (
     Builtin,
     PrefixOperator,
@@ -16,6 +15,7 @@
 
 from mathics.builtin.atomic.strings import to_regex
 
+from mathics.core.assignment import get_symbol_values
 from mathics.core.atoms import String
 
 from mathics.core.attributes import (
diff --git a/mathics/builtin/attributes.py b/mathics/builtin/attributes.py
index 5c161ecae..70a0a3777 100644
--- a/mathics/builtin/attributes.py
+++ b/mathics/builtin/attributes.py
@@ -15,7 +15,7 @@
 
 
 from mathics.builtin.base import Predefined, Builtin
-from mathics.builtin.assignments.internals import get_symbol_list
+from mathics.core.assignment import get_symbol_list
 from mathics.core.atoms import String
 from mathics.core.attributes import (
     attributes_bitset_to_list,
diff --git a/mathics/builtin/scoping.py b/mathics/builtin/scoping.py
index 3c76731f9..1d251bfd8 100644
--- a/mathics/builtin/scoping.py
+++ b/mathics/builtin/scoping.py
@@ -4,13 +4,13 @@
 """
 
 
-from mathics.builtin.assignments.internals import get_symbol_list
 from mathics.core.attributes import (
     A_HOLD_ALL,
     A_PROTECTED,
     attribute_string_to_number,
 )
 from mathics.builtin.base import Builtin, Predefined
+from mathics.core.assignment import get_symbol_list
 from mathics.core.atoms import (
     String,
     Integer,
diff --git a/mathics/builtin/assignments/internals.py b/mathics/core/assignment.py
similarity index 91%
rename from mathics/builtin/assignments/internals.py
rename to mathics/core/assignment.py
index 64588d72e..90ca57cb9 100644
--- a/mathics/builtin/assignments/internals.py
+++ b/mathics/core/assignment.py
@@ -1,4 +1,7 @@
 # -*- coding: utf-8 -*-
+"""
+Support for Set and SetDelayed, and other assignment-like builtins
+"""
 
 from typing import Optional, Tuple
 
@@ -12,6 +15,7 @@
 from mathics.core.symbols import (
     Symbol,
     SymbolFalse,
+    SymbolList,
     SymbolMinPrecision,
     SymbolMaxPrecision,
     SymbolN,
@@ -25,6 +29,7 @@
     SymbolHoldPattern,
     SymbolMachinePrecision,
     SymbolOptionValue,
+    SymbolPart,
     SymbolPattern,
     SymbolRuleDelayed,
 )
@@ -122,6 +127,32 @@ def is_protected(tag, defin):
     return A_PROTECTED & defin.get_attributes(tag)
 
 
+def normalize_lhs(lhs, evaluation):
+    """
+    Process the lhs in a way that
+    * if it is a conditional expression, reduce it to
+      a shallow conditional expression
+      ( Conditional[Conditional[...],tst] -> Conditional[stripped_lhs, tst])
+      with `stripped_lhs` the result of strip all the conditions from lhs.
+    * if ``stripped_lhs`` is not a ``List`` or a ``Part`` expression, evaluate the
+      elements.
+
+    returns a tuple with the normalized lhs, and the lookup_name of the head in stripped_lhs.
+    """
+    cond = None
+    if lhs.get_head() is SymbolCondition:
+        lhs, cond = unroll_conditions(lhs)
+
+    lookup_name = lhs.get_lookup_name()
+    # In WMA, before the assignment, the elements of the (stripped) LHS are evaluated.
+    if isinstance(lhs, Expression) and lhs.get_head() not in (SymbolList, SymbolPart):
+        lhs = lhs.evaluate_elements(evaluation)
+    # If there was a conditional expression, rebuild it with the processed lhs
+    if cond:
+        lhs = Expression(cond.get_head(), lhs, cond.elements[1])
+    return lhs, lookup_name
+
+
 def repl_pattern_by_symbol(expr):
     elements = expr.get_elements()
     if len(elements) == 0:
@@ -237,72 +268,47 @@ def unroll_conditions(lhs) -> Tuple[BaseElement, Optional[Expression]]:
 # maybe they should be member functions of _SetOperator.
 
 
-def process_assign_recursion_limit(lhs, rhs, evaluation):
-    """
-    Set ownvalue for the $RecursionLimit symbol.
-    """
-    rhs_int_value = rhs.get_int_value()
-    # if (not rhs_int_value or rhs_int_value < 20) and not
-    # rhs.get_name() == 'System`Infinity':
-    if (
-        not rhs_int_value or rhs_int_value < 20 or rhs_int_value > MAX_RECURSION_DEPTH
-    ):  # nopep8
-
-        evaluation.message("$RecursionLimit", "limset", rhs)
-        raise AssignmentException(lhs, None)
-    try:
-        set_python_recursion_limit(rhs_int_value)
-    except OverflowError:
-        # TODO: Message
-        raise AssignmentException(lhs, None)
-    return False
-
-
-def process_assign_iteration_limit(lhs, rhs, evaluation):
-    """
-    Set ownvalue for the $IterationLimit symbol.
-    """
-
-    rhs_int_value = rhs.get_int_value()
-    if (
-        not rhs_int_value or rhs_int_value < 20
-    ) and not rhs.get_name() == "System`Infinity":
-        evaluation.message("$IterationLimit", "limset", rhs)
-        raise AssignmentException(lhs, None)
-    return False
-
-
-def process_assign_module_number(lhs, rhs, evaluation):
+def process_assign_attributes(self, lhs, rhs, evaluation, tags, upset):
     """
-    Set ownvalue for the $ModuleNumber symbol.
+    Process the case where lhs is of the form
+    `Attribute[symbol]`
     """
-    rhs_int_value = rhs.get_int_value()
-    if not rhs_int_value or rhs_int_value <= 0:
-        evaluation.message("$ModuleNumber", "set", rhs)
-        raise AssignmentException(lhs, None)
-    return False
-
+    name = lhs.get_head_name()
+    if len(lhs.elements) != 1:
+        evaluation.message_args(name, len(lhs.elements), 1)
+        raise AssignmentException(lhs, rhs)
+    tag = lhs.elements[0].get_name()
+    if not tag:
+        evaluation.message(name, "sym", lhs.elements[0], 1)
+        raise AssignmentException(lhs, rhs)
+    if tags is not None and tags != [tag]:
+        evaluation.message(name, "tag", Symbol(name), Symbol(tag))
+        raise AssignmentException(lhs, rhs)
+    attributes_list = get_symbol_list(
+        rhs, lambda item: evaluation.message(name, "sym", item, 1)
+    )
+    if attributes_list is None:
+        raise AssignmentException(lhs, rhs)
+    if A_LOCKED & evaluation.definitions.get_attributes(tag):
+        evaluation.message(name, "locked", Symbol(tag))
+        raise AssignmentException(lhs, rhs)
 
-def process_assign_line_number_and_history_length(
-    self, lhs, rhs, evaluation, tags, upset
-):
-    """
-    Set ownvalue for the $Line and $HistoryLength symbols.
-    """
+    def reduce_attributes_from_list(x: int, y: str) -> int:
+        try:
+            return x | attribute_string_to_number[y]
+        except KeyError:
+            evaluation.message("SetAttributes", "unknowattr", y)
+            return x
 
-    lhs_name = lhs.get_name()
-    rhs_int_value = rhs.get_int_value()
-    if rhs_int_value is None or rhs_int_value < 0:
-        evaluation.message(lhs_name, "intnn", rhs)
-        raise AssignmentException(lhs, None)
-    return False
+    attributes = reduce(
+        reduce_attributes_from_list,
+        attributes_list,
+        0,
+    )
 
+    evaluation.definitions.set_attributes(tag, attributes)
 
-def process_assign_random_state(self, lhs, rhs, evaluation, tags, upset):
-    # TODO: allow setting of legal random states!
-    # (but consider pickle's insecurity!)
-    evaluation.message("$RandomState", "rndst", rhs)
-    raise AssignmentException(lhs, None)
+    return True
 
 
 def process_assign_context(self, lhs, rhs, evaluation, tags, upset):
@@ -348,6 +354,142 @@ def process_assign_context_path(self, lhs, rhs, evaluation, tags, upset):
         raise AssignmentException(lhs, None)
 
 
+def process_assign_default(self, lhs, rhs, evaluation, tags, upset):
+    lhs, condition = unroll_conditions(lhs)
+    lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
+    count = 0
+    defs = evaluation.definitions
+
+    if len(lhs.elements) not in (1, 2, 3):
+        evaluation.message_args(SymbolDefault, len(lhs.elements), 1, 2, 3)
+        raise AssignmentException(lhs, None)
+    focus = lhs.elements[0]
+    tags = process_tags_and_upset_dont_allow_custom(
+        tags, upset, self, lhs, focus, evaluation
+    )
+    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
+    rule = Rule(lhs, rhs)
+    for tag in tags:
+        if rejected_because_protected(self, lhs, tag, evaluation):
+            continue
+        count += 1
+        defs.add_default(tag, rule)
+    return count > 0
+
+
+def process_assign_definition_values(self, lhs, rhs, evaluation, tags, upset):
+    name = lhs.get_head_name()
+    tag = find_tag_and_check(lhs, tags, evaluation)
+    rules = rhs.get_rules_list()
+    if rules is None:
+        evaluation.message(name, "vrule", lhs, rhs)
+        raise AssignmentException(lhs, None)
+    evaluation.definitions.set_values(tag, name, rules)
+    return True
+
+
+def process_assign_format(self, lhs, rhs, evaluation, tags, upset):
+    lhs, condition = unroll_conditions(lhs)
+    lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
+    count = 0
+    defs = evaluation.definitions
+
+    if len(lhs.elements) not in (1, 2):
+        evaluation.message_args("Format", len(lhs.elements), 1, 2)
+        raise AssignmentException(lhs, None)
+    if len(lhs.elements) == 2:
+        form = lhs.elements[1]
+        form_name = form.get_name()
+        if not form_name:
+            evaluation.message("Format", "fttp", lhs.elements[1])
+            raise AssignmentException(lhs, None)
+        # If the form is not in defs.printforms / defs.outputforms
+        # add it.
+        for form_list in (defs.outputforms, defs.printforms):
+            if form not in form_list:
+                form_list.append(form)
+    else:
+        form_name = [
+            "System`StandardForm",
+            "System`TraditionalForm",
+            "System`OutputForm",
+            "System`TeXForm",
+            "System`MathMLForm",
+        ]
+    lhs = focus = lhs.elements[0]
+    tags = process_tags_and_upset_dont_allow_custom(
+        tags, upset, self, lhs, focus, evaluation
+    )
+    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
+    rule = Rule(lhs, rhs)
+    for tag in tags:
+        if rejected_because_protected(self, lhs, tag, evaluation):
+            continue
+        count += 1
+        defs.add_format(tag, rule, form_name)
+    return count > 0
+
+
+def process_assign_iteration_limit(lhs, rhs, evaluation):
+    """
+    Set ownvalue for the $IterationLimit symbol.
+    """
+
+    rhs_int_value = rhs.get_int_value()
+    if (
+        not rhs_int_value or rhs_int_value < 20
+    ) and not rhs.get_name() == "System`Infinity":
+        evaluation.message("$IterationLimit", "limset", rhs)
+        raise AssignmentException(lhs, None)
+    return False
+
+
+def process_assign_line_number_and_history_length(
+    self, lhs, rhs, evaluation, tags, upset
+):
+    """
+    Set ownvalue for the $Line and $HistoryLength symbols.
+    """
+
+    lhs_name = lhs.get_name()
+    rhs_int_value = rhs.get_int_value()
+    if rhs_int_value is None or rhs_int_value < 0:
+        evaluation.message(lhs_name, "intnn", rhs)
+        raise AssignmentException(lhs, None)
+    return False
+
+
+def process_assign_list(self, lhs, rhs, evaluation, tags, upset):
+    if not (
+        rhs.get_head_name() == "System`List" and len(lhs.elements) == len(rhs.elements)
+    ):  # nopep8
+        evaluation.message(self.get_name(), "shape", lhs, rhs)
+        return False
+    result = True
+    for left, right in zip(lhs.elements, rhs.elements):
+        if not self.assign(left, right, evaluation):
+            result = False
+    return result
+
+
+def process_assign_makeboxes(self, lhs, rhs, evaluation, tags, upset):
+    # FIXME: the below is a big hack.
+    # Currently MakeBoxes boxing is implemented as a bunch of rules.
+    # See mathics.builtin.base contribute().
+    # I think we want to change this so it works like normal SetDelayed
+    # That is:
+    #   MakeBoxes[CubeRoot, StandardForm] := RadicalBox[3, StandardForm]
+    # rather than:
+    #   MakeBoxes[CubeRoot, StandardForm] -> RadicalBox[3, StandardForm]
+
+    makeboxes_rule = Rule(lhs, rhs, system=False)
+    definitions = evaluation.definitions
+    definitions.add_rule("System`MakeBoxes", makeboxes_rule, "down")
+    #    makeboxes_defs = evaluation.definitions.builtin["System`MakeBoxes"]
+    #    makeboxes_defs.add_rule(makeboxes_rule)
+    return True
+
+
 def process_assign_minprecision(self, lhs, rhs, evaluation, tags, upset):
     lhs_name = lhs.get_name()
     rhs_int_value = rhs.get_int_value()
@@ -379,15 +521,38 @@ def process_assign_maxprecision(self, lhs, rhs, evaluation, tags, upset):
         raise AssignmentException(lhs, None)
 
 
-def process_assign_definition_values(self, lhs, rhs, evaluation, tags, upset):
-    name = lhs.get_head_name()
-    tag = find_tag_and_check(lhs, tags, evaluation)
-    rules = rhs.get_rules_list()
-    if rules is None:
-        evaluation.message(name, "vrule", lhs, rhs)
+def process_assign_messagename(self, lhs, rhs, evaluation, tags, upset):
+    lhs, condition = unroll_conditions(lhs)
+    lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
+    count = 0
+    defs = evaluation.definitions
+    if len(lhs.elements) != 2:
+        evaluation.message_args("MessageName", len(lhs.elements), 2)
         raise AssignmentException(lhs, None)
-    evaluation.definitions.set_values(tag, name, rules)
-    return True
+    focus = lhs.elements[0]
+    tags = process_tags_and_upset_dont_allow_custom(
+        tags, upset, self, lhs, focus, evaluation
+    )
+    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
+    rule = Rule(lhs, rhs)
+    for tag in tags:
+        # Messages can be assigned even if the symbol is protected...
+        # if rejected_because_protected(self, lhs, tag, evaluation):
+        #    continue
+        count += 1
+        defs.add_message(tag, rule)
+    return count > 0
+
+
+def process_assign_module_number(lhs, rhs, evaluation):
+    """
+    Set ownvalue for the $ModuleNumber symbol.
+    """
+    rhs_int_value = rhs.get_int_value()
+    if not rhs_int_value or rhs_int_value <= 0:
+        evaluation.message("$ModuleNumber", "set", rhs)
+        raise AssignmentException(lhs, None)
+    return False
 
 
 def process_assign_options(self, lhs, rhs, evaluation, tags, upset):
@@ -415,7 +580,7 @@ def process_assign_options(self, lhs, rhs, evaluation, tags, upset):
 
 
 def process_assign_numericq(self, lhs, rhs, evaluation, tags, upset):
-    lhs, condition = unroll_conditions(lhs)
+    # lhs, condition = unroll_conditions(lhs)
     lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
     if rhs not in (SymbolTrue, SymbolFalse):
         evaluation.message("NumericQ", "set", lhs, rhs)
@@ -504,146 +669,7 @@ def process_assign_other(
     return True, tags
 
 
-def process_assign_attributes(self, lhs, rhs, evaluation, tags, upset):
-    """
-    Process the case where lhs is of the form
-    `Attribute[symbol]`
-    """
-    name = lhs.get_head_name()
-    if len(lhs.elements) != 1:
-        evaluation.message_args(name, len(lhs.elements), 1)
-        raise AssignmentException(lhs, rhs)
-    tag = lhs.elements[0].get_name()
-    if not tag:
-        evaluation.message(name, "sym", lhs.elements[0], 1)
-        raise AssignmentException(lhs, rhs)
-    if tags is not None and tags != [tag]:
-        evaluation.message(name, "tag", Symbol(name), Symbol(tag))
-        raise AssignmentException(lhs, rhs)
-    attributes_list = get_symbol_list(
-        rhs, lambda item: evaluation.message(name, "sym", item, 1)
-    )
-    if attributes_list is None:
-        raise AssignmentException(lhs, rhs)
-    if A_LOCKED & evaluation.definitions.get_attributes(tag):
-        evaluation.message(name, "locked", Symbol(tag))
-        raise AssignmentException(lhs, rhs)
-
-    def reduce_attributes_from_list(x: int, y: str) -> int:
-        try:
-            return x | attribute_string_to_number[y]
-        except KeyError:
-            evaluation.message("SetAttributes", "unknowattr", y)
-            return x
-
-    attributes = reduce(
-        reduce_attributes_from_list,
-        attributes_list,
-        0,
-    )
-
-    evaluation.definitions.set_attributes(tag, attributes)
-
-    return True
-
-
-def process_assign_default(self, lhs, rhs, evaluation, tags, upset):
-    lhs, condition = unroll_conditions(lhs)
-    lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
-    count = 0
-    defs = evaluation.definitions
-
-    if len(lhs.elements) not in (1, 2, 3):
-        evaluation.message_args(SymbolDefault, len(lhs.elements), 1, 2, 3)
-        raise AssignmentException(lhs, None)
-    focus = lhs.elements[0]
-    tags = process_tags_and_upset_dont_allow_custom(
-        tags, upset, self, lhs, focus, evaluation
-    )
-    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
-    rule = Rule(lhs, rhs)
-    for tag in tags:
-        if rejected_because_protected(self, lhs, tag, evaluation):
-            continue
-        count += 1
-        defs.add_default(tag, rule)
-    return count > 0
-
-
-def process_assign_format(self, lhs, rhs, evaluation, tags, upset):
-    lhs, condition = unroll_conditions(lhs)
-    lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
-    count = 0
-    defs = evaluation.definitions
-
-    if len(lhs.elements) not in (1, 2):
-        evaluation.message_args("Format", len(lhs.elements), 1, 2)
-        raise AssignmentException(lhs, None)
-    if len(lhs.elements) == 2:
-        form = lhs.elements[1]
-        form_name = form.get_name()
-        if not form_name:
-            evaluation.message("Format", "fttp", lhs.elements[1])
-            raise AssignmentException(lhs, None)
-        # If the form is not in defs.printforms / defs.outputforms
-        # add it.
-        for form_list in (defs.outputforms, defs.printforms):
-            if form not in form_list:
-                form_list.append(form)
-    else:
-        form_name = [
-            "System`StandardForm",
-            "System`TraditionalForm",
-            "System`OutputForm",
-            "System`TeXForm",
-            "System`MathMLForm",
-        ]
-    lhs = focus = lhs.elements[0]
-    tags = process_tags_and_upset_dont_allow_custom(
-        tags, upset, self, lhs, focus, evaluation
-    )
-    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
-    rule = Rule(lhs, rhs)
-    for tag in tags:
-        if rejected_because_protected(self, lhs, tag, evaluation):
-            continue
-        count += 1
-        defs.add_format(tag, rule, form_name)
-    return count > 0
-
-
-def process_assign_list(self, lhs, rhs, evaluation, tags, upset):
-    if not (
-        rhs.get_head_name() == "System`List" and len(lhs.elements) == len(rhs.elements)
-    ):  # nopep8
-        evaluation.message(self.get_name(), "shape", lhs, rhs)
-        return False
-    result = True
-    for left, right in zip(lhs.elements, rhs.elements):
-        if not self.assign(left, right, evaluation):
-            result = False
-    return result
-
-
-def process_assign_makeboxes(self, lhs, rhs, evaluation, tags, upset):
-    # FIXME: the below is a big hack.
-    # Currently MakeBoxes boxing is implemented as a bunch of rules.
-    # See mathics.builtin.base contribute().
-    # I think we want to change this so it works like normal SetDelayed
-    # That is:
-    #   MakeBoxes[CubeRoot, StandardForm] := RadicalBox[3, StandardForm]
-    # rather than:
-    #   MakeBoxes[CubeRoot, StandardForm] -> RadicalBox[3, StandardForm]
-
-    makeboxes_rule = Rule(lhs, rhs, system=False)
-    definitions = evaluation.definitions
-    definitions.add_rule("System`MakeBoxes", makeboxes_rule, "down")
-    #    makeboxes_defs = evaluation.definitions.builtin["System`MakeBoxes"]
-    #    makeboxes_defs.add_rule(makeboxes_rule)
-    return True
-
-
-def process_assing_part(self, lhs, rhs, evaluation, tags, upset):
+def process_assign_part(self, lhs, rhs, evaluation, tags, upset):
     """
     Special case `A[[i,j,...]]=....`
     """
@@ -667,27 +693,32 @@ def process_assing_part(self, lhs, rhs, evaluation, tags, upset):
     return walk_parts([rule.replace], indices, evaluation, rhs)
 
 
-def process_assign_messagename(self, lhs, rhs, evaluation, tags, upset):
-    lhs, condition = unroll_conditions(lhs)
-    lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
-    count = 0
-    defs = evaluation.definitions
-    if len(lhs.elements) != 2:
-        evaluation.message_args("MessageName", len(lhs.elements), 2)
+def process_assign_random_state(self, lhs, rhs, evaluation, tags, upset):
+    # TODO: allow setting of legal random states!
+    # (but consider pickle's insecurity!)
+    evaluation.message("$RandomState", "rndst", rhs)
+    raise AssignmentException(lhs, None)
+
+
+def process_assign_recursion_limit(lhs, rhs, evaluation):
+    """
+    Set ownvalue for the $RecursionLimit symbol.
+    """
+    rhs_int_value = rhs.get_int_value()
+    # if (not rhs_int_value or rhs_int_value < 20) and not
+    # rhs.get_name() == 'System`Infinity':
+    if (
+        not rhs_int_value or rhs_int_value < 20 or rhs_int_value > MAX_RECURSION_DEPTH
+    ):  # nopep8
+
+        evaluation.message("$RecursionLimit", "limset", rhs)
         raise AssignmentException(lhs, None)
-    focus = lhs.elements[0]
-    tags = process_tags_and_upset_dont_allow_custom(
-        tags, upset, self, lhs, focus, evaluation
-    )
-    lhs, rhs = process_rhs_conditions(lhs, rhs, condition, evaluation)
-    rule = Rule(lhs, rhs)
-    for tag in tags:
-        # Messages can be assigned even if the symbol is protected...
-        # if rejected_because_protected(self, lhs, tag, evaluation):
-        #    continue
-        count += 1
-        defs.add_message(tag, rule)
-    return count > 0
+    try:
+        set_python_recursion_limit(rhs_int_value)
+    except OverflowError:
+        # TODO: Message
+        raise AssignmentException(lhs, None)
+    return False
 
 
 def process_rhs_conditions(lhs, rhs, condition, evaluation):
@@ -785,60 +816,26 @@ def process_tags_and_upset_allow_custom(tags, upset, self, lhs, evaluation):
     return tags, focus
 
 
-class _SetOperator:
-    """
-    This is the base class for assignment Builtin operators.
-
-    Special cases are determined by the head of the expression. Then
-    they are processed by specific routines, which are poke from
-    the `special_cases` dict.
-    """
-
-    # There are several idea about how to reimplement this. One possibility
-    # would be to use a Symbol instead of a name as the key of this dictionary.
-    #
-    # Another possibility would be to move the specific function to be a
-    # class method associated to the corresponding builtins. In any case,
-    # the present implementation should be clear enough to understand the
-    # logic.
-    #
-
-    special_cases = {
-        "System`$Context": process_assign_context,
-        "System`$ContextPath": process_assign_context_path,
-        "System`$RandomState": process_assign_random_state,
-        "System`Attributes": process_assign_attributes,
-        "System`Default": process_assign_default,
-        "System`DefaultValues": process_assign_definition_values,
-        "System`DownValues": process_assign_definition_values,
-        "System`Format": process_assign_format,
-        "System`List": process_assign_list,
-        "System`MakeBoxes": process_assign_makeboxes,
-        "System`MessageName": process_assign_messagename,
-        "System`Messages": process_assign_definition_values,
-        "System`N": process_assign_n,
-        "System`NValues": process_assign_definition_values,
-        "System`NumericQ": process_assign_numericq,
-        "System`Options": process_assign_options,
-        "System`OwnValues": process_assign_definition_values,
-        "System`Part": process_assing_part,
-        "System`SubValues": process_assign_definition_values,
-        "System`UpValues": process_assign_definition_values,
-    }
-
-    def assign(self, lhs, rhs, evaluation, tags=None, upset=False):
-        if isinstance(lhs, Symbol):
-            name = lhs.name
-        else:
-            name = lhs.get_head_name()
-        # lhs._format_cache = None
-        try:
-            # Deal with direct assignation to properties of
-            # the definition object
-            func = self.special_cases.get(name, None)
-            if func:
-                return func(self, lhs, rhs, evaluation, tags, upset)
-
-            return assign_store_rules_by_tag(self, lhs, rhs, evaluation, tags, upset)
-        except AssignmentException:
-            return False
+# Below is a mapping from a string Symbol name into an assignment function
+ASSIGNMENT_FUNCTION_MAP = {
+    "System`$Context": process_assign_context,
+    "System`$ContextPath": process_assign_context_path,
+    "System`$RandomState": process_assign_random_state,
+    "System`Attributes": process_assign_attributes,
+    "System`Default": process_assign_default,
+    "System`DefaultValues": process_assign_definition_values,
+    "System`DownValues": process_assign_definition_values,
+    "System`Format": process_assign_format,
+    "System`List": process_assign_list,
+    "System`MakeBoxes": process_assign_makeboxes,
+    "System`MessageName": process_assign_messagename,
+    "System`Messages": process_assign_definition_values,
+    "System`N": process_assign_n,
+    "System`NValues": process_assign_definition_values,
+    "System`NumericQ": process_assign_numericq,
+    "System`Options": process_assign_options,
+    "System`OwnValues": process_assign_definition_values,
+    "System`Part": process_assign_part,
+    "System`SubValues": process_assign_definition_values,
+    "System`UpValues": process_assign_definition_values,
+}
diff --git a/mathics/core/expression.py b/mathics/core/expression.py
index 5cb02211f..e03d20904 100644
--- a/mathics/core/expression.py
+++ b/mathics/core/expression.py
@@ -538,6 +538,10 @@ def evaluate(
         return expr
 
     def evaluate_elements(self, evaluation) -> "Expression":
+        """
+        return a new expression with the same head, and the
+        evaluable elements evaluated.
+        """
         elements = [
             element.evaluate(evaluation) if isinstance(element, EvalMixin) else element
             for element in self._elements
diff --git a/mathics/core/list.py b/mathics/core/list.py
index bd5818681..19cb603e3 100644
--- a/mathics/core/list.py
+++ b/mathics/core/list.py
@@ -4,6 +4,7 @@
 from typing import Optional, Tuple
 
 from mathics.core.element import ElementsProperties
+from mathics.core.evaluation import Evaluation
 from mathics.core.expression import Expression
 from mathics.core.symbols import EvalMixin, Symbol, SymbolList
 
@@ -85,7 +86,11 @@ def __str__(self) -> str:
         return f"<ListExpression: {self._elements}>"
 
     # @timeit
-    def evaluate_elements(self, evaluation):
+    def evaluate_elements(self, evaluation: Evaluation) -> Expression:
+        """
+        return a new expression with the same head, and the
+        evaluable elements evaluated.
+        """
         elements_changed = False
         # Make tuple self._elements mutable by turning it into a list.
         elements = list(self._elements)
@@ -98,16 +103,17 @@ def evaluate_elements(self, evaluation):
                         elements_changed = True
                         elements[index] = new_value
 
-        if elements_changed:
-            # Save changed elements, making them immutable again.
-            self._elements = tuple(elements)
+        if not elements_changed:
+            return self
 
-            # TODO: we could have a specialized version of this
-            # that keeps self.value up to date when that is
-            # easy to do. That is left of some future time to
-            # decide whether doing this this is warranted.
-            self._build_elements_properties()
-            self.value = None
+        new_list = ListExpression(*elements)
+        # TODO: we could have a specialized version of this
+        # that keeps self.value up to date when that is
+        # easy to do. That is left of some future time to
+        # decide whether doing this this is warranted.
+        new_list._build_elements_properties()
+        new_list.value = None
+        return new_list
 
     @property
     def is_literal(self) -> bool:
@@ -142,7 +148,7 @@ def rewrite_apply_eval_step(self, evaluation) -> Tuple[Expression, bool]:
             self._build_elements_properties()
         if not self.elements_properties.elements_fully_evaluated:
             new = self.shallow_copy()
-            new.evaluate_elements(evaluation)
+            new = new.evaluate_elements(evaluation)
             return new, False
         return self, False
 
diff --git a/mathics/core/systemsymbols.py b/mathics/core/systemsymbols.py
index 15088a264..a49c30a5d 100644
--- a/mathics/core/systemsymbols.py
+++ b/mathics/core/systemsymbols.py
@@ -140,6 +140,7 @@
 SymbolOverflow = Symbol("System`Overflow")
 SymbolOwnValues = Symbol("System`OwnValues")
 SymbolPackages = Symbol("System`$Packages")
+SymbolPart = Symbol("System`Part")
 SymbolPattern = Symbol("System`Pattern")
 SymbolPatternTest = Symbol("System`PatternTest")
 SymbolPower = Symbol("System`Power")
diff --git a/test/builtin/test_assignment.py b/test/builtin/test_assignment.py
index 5d6cc791c..cb4e42d61 100644
--- a/test/builtin/test_assignment.py
+++ b/test/builtin/test_assignment.py
@@ -158,6 +158,58 @@ def test_setdelayed_oneidentity():
             "{a + b, Q[a, b], a + b}",
             None,
         ),
+        (None, None, None),
+        (r"a=b; a=4; {a, b}", "{4, b}", None),
+        (None, None, None),
+        (r"a=b; b=4;  {a,b}", "{4, 4}", None),
+        (None, None, None),
+        (r"a=b; b=4; Clear[a]; {a,b}", "{a, 4}", None),
+        (None, None, None),
+        ("a=b; b=4; Clear[b]; {a, b}", "{b, b}", None),
+        (None, None, None),
+        ("F[x_]:=x^2; G[x_]:=F[x]; ClearAll[F]; G[u]", "F[u]", None),
+        (None, None, None),
+        ("F[x_]:=G[x]; G[x_]:=x^2; ClearAll[G]; F[u]", "G[u]", None),
+        (None, None, None),
+        (
+            "F[x_]:=G[x]; H[F[y_]]:=Q[y]; ClearAll[F]; {H[G[5]],H[F[5]]}",
+            "{Q[5], H[F[5]]}",
+            "Arguments on the LHS are evaluated before the assignment in := after F reset",
+        ),
+        (None, None, None),
+        (
+            "F[x_]:=G[x]; H[F[y_]]^:=Q[y]; ClearAll[F]; {H[G[5]],H[F[5]]}",
+            "{Q[5], H[F[5]]}",
+            "Arguments on the LHS are evaluated before the assignment in ^:= after F reset",
+        ),
+        (None, None, None),
+        (
+            "F[x_]:=G[x]; H[F[y_]]:=Q[y]; ClearAll[G]; {H[G[5]],H[F[5]]}",
+            "{Q[5], Q[5]}",
+            "The arguments on the LHS are evaluated before the assignment in := after G reset",
+        ),
+        (None, None, None),
+        (
+            "F[x_]:=G[x]; H[F[y_]]^:=Q[y]; ClearAll[G]; {H[G[5]],H[F[5]]}",
+            "{H[G[5]], H[G[5]]}",
+            "The arguments on the LHS are evaluated before the assignment in ^:= after G reset",
+        ),
+        (None, None, None),
+        (
+            (
+                "A[x_]:=B[x];B[x_]:=F[x];F[x_]:=G[x];"
+                "H[A[y_]]:=Q[y]; ClearAll[F];"
+                "{H[A[5]],H[B[5]],H[F[5]],H[G[5]]}"
+            ),
+            "{H[F[5]], H[F[5]], H[F[5]], Q[5]}",
+            "The arguments on the LHS are completely evaluated before the assignment",
+        ),
+        (None, None, None),
+        (
+            "F[x_]:=G[x];N[F[x_]]:=x^2;ClearAll[F];{N[F[2]],N[G[2]]}",
+            "{F[2.], 4.}",
+            "Assign N rule",
+        ),
         (
             None,
             None,