Fix legalize_path typesfix-legalize-path

Mypy was not happy here because `_legalize_stage` function
implementation concatenates `path` and `extension` parameters, implying
that their types need to match.

You can see that initially `path` parameter was defined as a `str` while
`extension` was `bytes`.

In reality, depending on the `fragment` parameter value, `extension` was
sometimes provided as a `str` and sometimes as `bytes`. The same
parameter decided whether `path` gets converted into `bytes` within
`_legalize_stage` implementation. No surprise that mypy was confused
here.

`_legalize_stage` is only used within `Item.destination` method
implementation which accepts where `fragment` is defined. I determined
that the `fragment` parameter controls the form of the output path:

- fragment=False returned path absolute path *as bytes* (default)
- fragment=True returned path relative to the library directory as *str*.

Given the above, this commit

1. Renames `fragment` parameter to `relative_to_libdir` for clarity
2. Makes `Item.destination` to return the same type in both cases.
   I picked `bytes` since that's the type that majority of the code
   using this method expects.

   I converted the output path to str for the code that has been
   expecting a string here.
3. Decouples `_legalize_stage` and `_legalize_path` implementations from
   the `relative_to_libdir`. The logic now uses `str` type only.

6 files changed, 45 insertions, 67 deletions

diff --git a/beets/library.py b/beets/library.py
index 367b184ef..bb954237c 100644
--- a/beets/library.py
+++ b/beets/library.py
@@ -1042,12 +1042,12 @@ class Item(LibModel):
 
     def destination(
         self,
-        fragment=False,
+        relative_to_libdir=False,
         basedir=None,
         platform=None,
         path_formats=None,
         replacements=None,
-    ):
+    ) -> bytes:
         """Return the path in the library directory designated for the
         item (i.e., where the file ought to be).
 
@@ -1104,12 +1104,11 @@ class Item(LibModel):
             # When zero, try to determine from filesystem.
             maxlen = util.max_filename_length(self._db.directory)
 
-        subpath, fellback = util.legalize_path(
+        lib_path_str, fellback = util.legalize_path(
             subpath,
             replacements,
             maxlen,
             os.path.splitext(self.path)[1],
-            fragment,
         )
         if fellback:
             # Print an error message if legalization fell back to
@@ -1120,11 +1119,12 @@ class Item(LibModel):
                 "the filename.",
                 subpath,
             )
+        lib_path_bytes = lib_path_str.encode()
 
-        if fragment:
-            return util.as_string(subpath)
-        else:
-            return normpath(os.path.join(basedir, subpath))
+        if relative_to_libdir:
+            return lib_path_bytes
+
+        return normpath(os.path.join(basedir, lib_path_bytes))
 
 
 class Album(LibModel):
diff --git a/beets/util/__init__.py b/beets/util/__init__.py
index 87bb96ed4..be4366371 100644
--- a/beets/util/__init__.py
+++ b/beets/util/__init__.py
@@ -733,21 +733,17 @@ def _legalize_stage(
     replacements: Optional[Sequence[Tuple[Pattern[str], str]]],
     length: int,
     extension: str,
-    fragment: bool,
-) -> Tuple[BytesOrStr, bool]:
+) -> Tuple[str, bool]:
     """Perform a single round of path legalization steps
-    (sanitation/replacement, encoding from Unicode to bytes,
-    extension-appending, and truncation). Return the path (Unicode if
-    `fragment` is set, `bytes` otherwise) and whether truncation was
-    required.
+    1. sanitation/replacement
+    2. appending the extension
+    3. truncation.
+
+    Return the path and whether truncation was required.
     """
     # Perform an initial sanitization including user replacements.
     path = sanitize_path(path, replacements)
 
-    # Encode for the filesystem.
-    if not fragment:
-        path = bytestring_path(path)  # type: ignore
-
     # Preserve extension.
     path += extension.lower()
 
@@ -762,54 +758,42 @@ def legalize_path(
     path: str,
     replacements: Optional[Sequence[Tuple[Pattern[str], str]]],
     length: int,
-    extension: bytes,
-    fragment: bool,
-) -> Tuple[Union[BytesOrStr, bool]]:
+    extension: str,
+) -> Tuple[str, bool]:
     """Given a path-like Unicode string, produce a legal path. Return
     the path and a flag indicating whether some replacements had to be
     ignored (see below).
 
-    The legalization process (see `_legalize_stage`) consists of
-    applying the sanitation rules in `replacements`, encoding the string
-    to bytes (unless `fragment` is set), truncating components to
-    `length`, appending the `extension`.
+    This function uses `_legalize_stage` function to legalize the path, see its
+    documentation for the details of what this involves. It is called up to
+    three times in case truncation conflicts with replacements (as can happen
+    when truncation creates whitespace at the end of the string, for example).
 
-    This function performs up to three calls to `_legalize_stage` in
-    case truncation conflicts with replacements (as can happen when
-    truncation creates whitespace at the end of the string, for
-    example). The limited number of iterations iterations avoids the
+    The limited number of iterations iterations avoids the
     possibility of an infinite loop of sanitation and truncation
     operations, which could be caused by replacement rules that make the
-    string longer. The flag returned from this function indicates that
+    string longer.
+
+    The flag returned from this function indicates that
     the path has to be truncated twice (indicating that replacements
     made the string longer again after it was truncated); the
     application should probably log some sort of warning.
     """
+    args = length, as_string(extension)
 
-    if fragment:
-        # Outputting Unicode.
-        extension = extension.decode("utf-8", "ignore")
-
-    first_stage_path, _ = _legalize_stage(
-        path, replacements, length, extension, fragment
+    first_stage, _ = os.path.splitext(
+        _legalize_stage(path, replacements, *args)[0]
     )
 
-    # Convert back to Unicode with extension removed.
-    first_stage_path, _ = os.path.splitext(displayable_path(first_stage_path))
-
     # Re-sanitize following truncation (including user replacements).
-    second_stage_path, retruncated = _legalize_stage(
-        first_stage_path, replacements, length, extension, fragment
-    )
+    second_stage, truncated = _legalize_stage(first_stage, replacements, *args)
 
-    # If the path was once again truncated, discard user replacements
-    # and run through one last legalization stage.
-    if retruncated:
-        second_stage_path, _ = _legalize_stage(
-            first_stage_path, None, length, extension, fragment
-        )
+    if not truncated:
+        return second_stage, False
 
-    return second_stage_path, retruncated
+    # If the path was truncated, discard user replacements
+    # and run through one last legalization stage.
+    return _legalize_stage(first_stage, None, *args)[0], True
 
 
 def py3_path(path: Union[bytes, str]) -> str:
diff --git a/beets/vfs.py b/beets/vfs.py
index 4a9681a92..0fc4a9aa9 100644
--- a/beets/vfs.py
+++ b/beets/vfs.py
@@ -46,7 +46,7 @@ def libtree(lib):
     """
     root = Node({}, {})
     for item in lib.items():
-        dest = item.destination(fragment=True)
-        parts = util.components(dest)
+        dest = item.destination(relative_to_libdir=True)
+        parts = util.components(util.as_string(dest))
         _insert(root, parts, item.id)
     return root
diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py
index 3d7396401..ab238bd55 100644
--- a/beetsplug/bpd/__init__.py
+++ b/beetsplug/bpd/__init__.py
@@ -35,7 +35,7 @@ import beets.ui
 from beets import dbcore, vfs
 from beets.library import Item
 from beets.plugins import BeetsPlugin
-from beets.util import bluelet
+from beets.util import as_string, bluelet
 
 PROTOCOL_VERSION = "0.16.0"
 BUFSIZE = 1024
@@ -1131,7 +1131,7 @@ class Server(BaseServer):
 
     def _item_info(self, item):
         info_lines = [
-            "file: " + item.destination(fragment=True),
+            "file: " + as_string(item.destination(relative_to_libdir=True)),
             "Time: " + str(int(item.length)),
             "duration: " + f"{item.length:.3f}",
             "Id: " + str(item.id),
diff --git a/beetsplug/convert.py b/beetsplug/convert.py
index 22a88fd5d..cea99d5c1 100644
--- a/beetsplug/convert.py
+++ b/beetsplug/convert.py
@@ -609,13 +609,7 @@ class ConvertPlugin(BeetsPlugin):
             # strings we get from item.destination to bytes.
             items_paths = [
                 os.path.relpath(
-                    util.bytestring_path(
-                        item.destination(
-                            basedir=dest,
-                            path_formats=path_formats,
-                            fragment=False,
-                        )
-                    ),
+                    item.destination(basedir=dest, path_formats=path_formats),
                     pl_dir,
                 )
                 for item in items
diff --git a/test/test_library.py b/test/test_library.py
index c9d0440b5..82cc0092b 100644
--- a/test/test_library.py
+++ b/test/test_library.py
@@ -33,7 +33,7 @@ from beets import config, plugins, util
 from beets.test import _common
 from beets.test._common import item
 from beets.test.helper import TestHelper
-from beets.util import bytestring_path, syspath
+from beets.util import as_string, bytestring_path, syspath
 
 # Shortcut to path normalization.
 np = util.normpath
@@ -419,14 +419,14 @@ class DestinationTest(_common.TestCase):
     def test_unicode_normalized_nfd_on_mac(self):
         instr = unicodedata.normalize("NFC", "caf\xe9")
         self.lib.path_formats = [("default", instr)]
-        dest = self.i.destination(platform="darwin", fragment=True)
-        self.assertEqual(dest, unicodedata.normalize("NFD", instr))
+        dest = self.i.destination(platform="darwin", relative_to_libdir=True)
+        self.assertEqual(as_string(dest), unicodedata.normalize("NFD", instr))
 
     def test_unicode_normalized_nfc_on_linux(self):
         instr = unicodedata.normalize("NFD", "caf\xe9")
         self.lib.path_formats = [("default", instr)]
-        dest = self.i.destination(platform="linux", fragment=True)
-        self.assertEqual(dest, unicodedata.normalize("NFC", instr))
+        dest = self.i.destination(platform="linux", relative_to_libdir=True)
+        self.assertEqual(as_string(dest), unicodedata.normalize("NFC", instr))
 
     def test_non_mbcs_characters_on_windows(self):
         oldfunc = sys.getfilesystemencoding
@@ -444,8 +444,8 @@ class DestinationTest(_common.TestCase):
     def test_unicode_extension_in_fragment(self):
         self.lib.path_formats = [("default", "foo")]
         self.i.path = util.bytestring_path("bar.caf\xe9")
-        dest = self.i.destination(platform="linux", fragment=True)
-        self.assertEqual(dest, "foo.caf\xe9")
+        dest = self.i.destination(platform="linux", relative_to_libdir=True)
+        self.assertEqual(as_string(dest), "foo.caf\xe9")
 
     def test_asciify_and_replace(self):
         config["asciify_paths"] = True