From 874490468d91c40aff1b9a07630a268e126a1790 Mon Sep 17 00:00:00 2001 From: Mathias Lang Date: Sun, 14 Jan 2024 14:35:28 +0100 Subject: [PATCH] Introduce PackageName, to fully type a package name Package names being passed around in the Dub codebase are string, which is problematic as it suffers from validation / semantic issues. Validation because we cannot impose restrictions on characters or length easily, and semantic because some functions expect a main package while some would accept a subpackage. Introducing a type will remedy both those issues. --- source/dub/commandline.d | 4 +- source/dub/dependency.d | 69 ++++++++++++++++++++++++++++++- source/dub/dub.d | 35 ++++++++-------- source/dub/packagemanager.d | 14 ++++--- source/dub/project.d | 6 +-- source/dub/recipe/io.d | 41 +++++++++++++----- source/dub/recipe/json.d | 25 +++++++---- source/dub/recipe/packagerecipe.d | 4 +- 8 files changed, 150 insertions(+), 48 deletions(-) diff --git a/source/dub/commandline.d b/source/dub/commandline.d index 5e80238b8..15a4eb79e 100644 --- a/source/dub/commandline.d +++ b/source/dub/commandline.d @@ -1557,7 +1557,7 @@ class BuildCommand : GenerateCommand { if (packageParts.name.startsWith(":")) return 0; - const baseName = getBasePackageName(packageParts.name); + const baseName = PackageName(packageParts.name).main; // Found locally if (dub.packageManager.getBestPackage(baseName, packageParts.range)) return 0; @@ -2788,7 +2788,7 @@ class DustmiteCommand : PackageBuildCommand { static void fixPathDependency(string pack, ref Dependency dep) { dep.visit!( (NativePath path) { - auto mainpack = getBasePackageName(pack); + auto mainpack = PackageName(pack).main; dep = Dependency(NativePath("../") ~ mainpack); }, (any) { /* Nothing to do */ }, diff --git a/source/dub/dependency.d b/source/dub/dependency.d index c0562fcf6..9809f2981 100644 --- a/source/dub/dependency.d +++ b/source/dub/dependency.d @@ -18,12 +18,79 @@ import std.array; import std.exception; import std.string; +/// Represents a fully-qualified package name +public struct PackageName +{ + /// The underlying full name of the package + private string fullName; + /// Where the separator lies, if any + private size_t separator; + + /// For compatibility in `PackageDependency` + alias toString this; + + /// Creates a new instance of this struct + public this(string fn) @safe pure + { + this.fullName = fn; + if (auto idx = fn.indexOf(':')) + this.separator = idx > 0 ? idx : fn.length; + else // We were given `:foo` + assert(0, "Argument to PackageName constructor needs to be " ~ + "a fully qualified string"); + } + + /// Private constructor to have nothrow / @nogc + private this(string fn, size_t sep) @safe pure nothrow @nogc + { + this.fullName = fn; + this.separator = sep; + } + + /// The base package name in which the subpackages may live + public PackageName main () const return @safe pure nothrow @nogc + { + return PackageName(this.fullName[0 .. this.separator], this.separator); + } + + /// The subpackage name, or an empty string if there isn't + public string sub () const return @safe pure nothrow @nogc + { + // Return `null` instead of an empty string so that + // it can be used in a boolean context, e.g. + // `if (name.sub)` would be true with empty string + return this.separator < this.fullName.length + ? this.fullName[this.separator + 1 .. $] + : null; + } + + /// Human readable representation + public string toString () const return scope @safe pure nothrow @nogc + { + return this.fullName; + } +} /** Encapsulates the name of a package along with its dependency specification. */ struct PackageDependency { + /// Backward compatibility + deprecated("Use the constructor that accepts a `PackageName` as first argument") + this(string n, Dependency s = Dependency.init) @safe pure + { + this.name = PackageName(n); + this.spec = s; + } + + // Remove once deprecated overload is gone + this(PackageName n, Dependency s = Dependency.init) @safe pure nothrow @nogc + { + this.name = n; + this.spec = s; + } + /// Name of the referenced package. - string name; + PackageName name; /// Dependency specification used to select a particular version of the package. Dependency spec; diff --git a/source/dub/dub.d b/source/dub/dub.d index 4d3bb6ef6..89ddae9f0 100644 --- a/source/dub/dub.d +++ b/source/dub/dub.d @@ -584,7 +584,8 @@ class Dub { recipe_content = recipe_content[idx+1 .. $]; auto recipe_default_package_name = path.toString.baseName.stripExtension.strip; - auto recipe = parsePackageRecipe(recipe_content, recipe_filename, null, recipe_default_package_name); + const PackageName empty; + auto recipe = parsePackageRecipe(recipe_content, recipe_filename, empty, recipe_default_package_name); enforce(recipe.buildSettings.sourceFiles.length == 0, "Single-file packages are not allowed to specify source files."); enforce(recipe.buildSettings.sourcePaths.length == 0, "Single-file packages are not allowed to specify source paths."); enforce(recipe.buildSettings.cSourcePaths.length == 0, "Single-file packages are not allowed to specify C source paths."); @@ -637,7 +638,7 @@ class Dub { try if (m_packageManager.getOrLoadPackage(path)) continue; catch (Exception e) { logDebug("Failed to load path based selection: %s", e.toString().sanitize); } } else if (!dep.repository.empty) { - if (m_packageManager.loadSCMPackage(getBasePackageName(p), dep.repository)) + if (m_packageManager.loadSCMPackage(PackageName(p).main, dep.repository)) continue; } else { if (m_packageManager.getPackage(p, dep.version_)) continue; @@ -664,12 +665,12 @@ class Dub { if (options & UpgradeOptions.dryRun) { bool any = false; - string rootbasename = getBasePackageName(m_project.rootPackage.name); + string rootbasename = PackageName(m_project.rootPackage.name).main; foreach (p, ver; versions) { if (!ver.path.empty || !ver.repository.empty) continue; - auto basename = getBasePackageName(p); + auto basename = PackageName(p).main; if (basename == rootbasename) continue; if (!m_project.selections.hasSelectedVersion(basename)) { @@ -907,7 +908,7 @@ class Dub { /// Ditto Package fetch(string packageId, in VersionRange range, PlacementLocation location, FetchOptions options, string reason = "") { - auto basePackageName = getBasePackageName(packageId); + auto basePackageName = PackageName(packageId).main; Json pinfo; PackageSupplier supplier; foreach(ps; m_packageSuppliers){ @@ -1229,7 +1230,7 @@ class Dub { Version[] listPackageVersions(string name) { Version[] versions; - auto basePackageName = getBasePackageName(name); + auto basePackageName = PackageName(name).main; foreach (ps; this.m_packageSuppliers) { try versions ~= ps.getVersions(basePackageName); catch (Exception e) { @@ -1719,7 +1720,7 @@ private class DependencyVersionResolver : DependencyResolver!(Dependency, Depend // filter out invalid/unreachable dependency specs versions = versions.filter!((v) { - bool valid = getPackage(pack, Dependency(v)) !is null; + bool valid = getPackage(PackageName(pack), Dependency(v)) !is null; if (!valid) logDiagnostic("Excluding invalid dependency specification %s %s from dependency resolution process.", pack, v); return valid; }).array; @@ -1735,7 +1736,7 @@ private class DependencyVersionResolver : DependencyResolver!(Dependency, Depend protected override Dependency[] getSpecificConfigs(string pack, TreeNodes nodes) { if (!nodes.configs.path.empty || !nodes.configs.repository.empty) { - if (getPackage(nodes.pack, nodes.configs)) return [nodes.configs]; + if (getPackage(PackageName(nodes.pack), nodes.configs)) return [nodes.configs]; else return null; } else return null; @@ -1755,7 +1756,7 @@ private class DependencyVersionResolver : DependencyResolver!(Dependency, Depend { import std.array : appender; auto ret = appender!(TreeNodes[]); - auto pack = getPackage(node.pack, node.config); + auto pack = getPackage(PackageName(node.pack), node.config); if (!pack) { // this can happen when the package description contains syntax errors logDebug("Invalid package in dependency tree: %s %s", node.pack, node.config); @@ -1764,13 +1765,13 @@ private class DependencyVersionResolver : DependencyResolver!(Dependency, Depend auto basepack = pack.basePackage; foreach (d; pack.getAllDependenciesRange()) { - auto dbasename = getBasePackageName(d.name); + auto dbasename = PackageName(d.name).main; // detect dependencies to the root package (or sub packages thereof) if (dbasename == basepack.name) { auto absdeppath = d.spec.mapToPath(pack.path).path; absdeppath.endsWithSlash = true; - auto subpack = m_dub.m_packageManager.getSubPackage(basepack, getSubPackageName(d.name), true); + auto subpack = m_dub.m_packageManager.getSubPackage(basepack, PackageName(d.name).sub, true); if (subpack) { auto desireddeppath = basepack.path; desireddeppath.endsWithSlash = true; @@ -1821,7 +1822,7 @@ private class DependencyVersionResolver : DependencyResolver!(Dependency, Depend return configs.merge(config).valid; } - private Package getPackage(string name, Dependency dep) + private Package getPackage(PackageName name, Dependency dep) { auto key = PackageDependency(name, dep); if (auto pp = key in m_packages) @@ -1831,16 +1832,16 @@ private class DependencyVersionResolver : DependencyResolver!(Dependency, Depend return p; } - private Package getPackageRaw(string name, Dependency dep) + private Package getPackageRaw(PackageName name, Dependency dep) { import dub.recipe.json; - auto basename = getBasePackageName(name); + auto basename = name.main; // for sub packages, first try to get them from the base package if (basename != name) { - auto subname = getSubPackageName(name); - auto basepack = getPackage(basename, dep); + auto subname = name.sub; + auto basepack = getPackage(name.main, dep); if (!basepack) return null; if (auto sp = m_dub.m_packageManager.getSubPackage(basepack, subname, true)) { return sp; @@ -1895,7 +1896,7 @@ private class DependencyVersionResolver : DependencyResolver!(Dependency, Depend if (desc.type == Json.Type.null_) continue; PackageRecipe recipe; - parseJson(recipe, desc, null); + parseJson(recipe, desc); auto ret = new Package(recipe); m_remotePackages[key] = ret; return ret; diff --git a/source/dub/packagemanager.d b/source/dub/packagemanager.d index 74b8542ef..13161af02 100644 --- a/source/dub/packagemanager.d +++ b/source/dub/packagemanager.d @@ -188,7 +188,7 @@ class PackageManager { * Returns: * A `Package` if one was found, `null` if none exists. */ - protected Package lookup (string name, Version vers) { + protected Package lookup (PackageName name, Version vers) { if (!this.m_initialized) this.refresh(); @@ -240,7 +240,7 @@ class PackageManager { } } - return this.lookup(name, ver); + return this.lookup(PackageName(name), ver); } /// ditto @@ -268,7 +268,7 @@ class PackageManager { // Bare mode if (loc >= this.m_repositories.length) return null; - return this.m_repositories[loc].load(name, ver, this); + return this.m_repositories[loc].load(PackageName(name), ver, this); } /// ditto @@ -374,7 +374,9 @@ class PackageManager { .format(path.toNativeString(), packageInfoFiles.map!(f => cast(string)f.filename).join("/"))); - auto content = readPackageRecipe(recipe, parent ? parent.name : null, mode); + const PackageName pname = parent + ? PackageName(parent.name) : PackageName.init; + auto content = readPackageRecipe(recipe, pname, mode); auto ret = new Package(content, path, parent, version_); ret.m_infoFile = recipe; return ret; @@ -1433,13 +1435,13 @@ package struct Location { * Returns: * A `Package` if one was found, `null` if none exists. */ - Package load (string name, Version vers, PackageManager mgr) + Package load (PackageName name, Version vers, PackageManager mgr) { if (auto pkg = this.lookup(name, vers)) return pkg; string versStr = vers.toString(); - const lookupName = getBasePackageName(name); + const lookupName = name.main; const path = this.getPackagePath(lookupName, versStr) ~ (lookupName ~ "/"); if (!path.existsDirectory()) return null; diff --git a/source/dub/project.d b/source/dub/project.d index 65c9192f3..0a50673cc 100644 --- a/source/dub/project.d +++ b/source/dub/project.d @@ -462,7 +462,7 @@ class Project { pack.simpleLint(); foreach (d; pack.getAllDependencies()) { - auto basename = getBasePackageName(d.name); + auto basename = d.name.main; d.spec.visit!( (NativePath path) { /* Valid */ }, (Repository repo) { /* Valid */ }, @@ -517,8 +517,8 @@ class Project { Dependency vspec = dep.spec; Package p; - auto basename = getBasePackageName(dep.name); - auto subname = getSubPackageName(dep.name); + auto basename = dep.name.main; + auto subname = dep.name.sub; // non-optional and optional-default dependencies (if no selections file exists) // need to be satisfied diff --git a/source/dub/recipe/io.d b/source/dub/recipe/io.d index 384b18f2e..5b9b1e092 100644 --- a/source/dub/recipe/io.d +++ b/source/dub/recipe/io.d @@ -7,6 +7,7 @@ */ module dub.recipe.io; +import dub.dependency : PackageName; import dub.recipe.packagerecipe; import dub.internal.logging; import dub.internal.vibecompat.core.file; @@ -19,7 +20,7 @@ import dub.internal.configy.Read; Params: filename = NativePath of the package recipe file - parent_name = Optional name of the parent package (if this is a sub package) + parent = Optional name of the parent package (if this is a sub package) mode = Whether to issue errors, warning, or ignore unknown keys in dub.json Returns: Returns the package recipe contents @@ -27,19 +28,28 @@ import dub.internal.configy.Read; */ deprecated("Use the overload that accepts a `NativePath` as first argument") PackageRecipe readPackageRecipe( - string filename, string parent_name = null, StrictMode mode = StrictMode.Ignore) + string filename, string parent = null, StrictMode mode = StrictMode.Ignore) { - return readPackageRecipe(NativePath(filename), parent_name, mode); + return readPackageRecipe(NativePath(filename), parent, mode); } /// ditto +deprecated("Use the overload that accepts a `PackageName` as second argument") PackageRecipe readPackageRecipe( - NativePath filename, string parent_name = null, StrictMode mode = StrictMode.Ignore) + NativePath filename, string parent, StrictMode mode = StrictMode.Ignore) +{ + return readPackageRecipe(filename, parent.length ? PackageName(parent) : PackageName.init, mode); +} + + +/// ditto +PackageRecipe readPackageRecipe(NativePath filename, + in PackageName parent = PackageName.init, StrictMode mode = StrictMode.Ignore) { import dub.internal.utils : stripUTF8Bom; string text = stripUTF8Bom(cast(string)readFile(filename)); - return parsePackageRecipe(text, filename.toNativeString(), parent_name, null, mode); + return parsePackageRecipe(text, filename.toNativeString(), parent, null, mode); } /** Parses an in-memory package recipe. @@ -50,7 +60,7 @@ PackageRecipe readPackageRecipe( contents = The contents of the recipe file filename = Name associated with the package recipe - this is only used to determine the file format from the file extension - parent_name = Optional name of the parent package (if this is a sub + parent = Optional name of the parent package (if this is a sub package) default_package_name = Optional default package name (if no package name is found in the recipe this value will be used) @@ -59,7 +69,18 @@ PackageRecipe readPackageRecipe( Returns: Returns the package recipe contents Throws: Throws an exception if an I/O or syntax error occurs */ -PackageRecipe parsePackageRecipe(string contents, string filename, string parent_name = null, +deprecated("Use the overload that accepts a `PackageName` as 3rd argument") +PackageRecipe parsePackageRecipe(string contents, string filename, string parent, + string default_package_name = null, StrictMode mode = StrictMode.Ignore) +{ + return parsePackageRecipe(contents, filename, parent.length ? + PackageName(parent) : PackageName.init, + default_package_name, mode); +} + +/// Ditto +PackageRecipe parsePackageRecipe(string contents, string filename, + in PackageName parent = PackageName.init, string default_package_name = null, StrictMode mode = StrictMode.Ignore) { import std.algorithm : endsWith; @@ -83,7 +104,7 @@ PackageRecipe parsePackageRecipe(string contents, string filename, string parent logWarn("Error was: %s", exc); // Fallback to JSON parser ret = PackageRecipe.init; - parseJson(ret, parseJsonString(contents, filename), parent_name); + parseJson(ret, parseJsonString(contents, filename), parent); } catch (Exception exc) { logWarn("Your `dub.json` file use non-conventional features that are deprecated"); logWarn("This is most likely due to duplicated keys."); @@ -91,7 +112,7 @@ PackageRecipe parsePackageRecipe(string contents, string filename, string parent logWarn("Error was: %s", exc); // Fallback to JSON parser ret = PackageRecipe.init; - parseJson(ret, parseJsonString(contents, filename), parent_name); + parseJson(ret, parseJsonString(contents, filename), parent); } // `debug = ConfigFillerDebug` also enables verbose parser output debug (ConfigFillerDebug) @@ -112,7 +133,7 @@ PackageRecipe parsePackageRecipe(string contents, string filename, string parent } } } - else if (filename.endsWith(".sdl")) parseSDL(ret, contents, parent_name, filename); + else if (filename.endsWith(".sdl")) parseSDL(ret, contents, parent, filename); else assert(false, "readPackageRecipe called with filename with unknown extension: "~filename); // Fix for issue #711: `targetType` should be inherited, or default to library diff --git a/source/dub/recipe/json.d b/source/dub/recipe/json.d index 505baff57..e7ac40d33 100644 --- a/source/dub/recipe/json.d +++ b/source/dub/recipe/json.d @@ -20,8 +20,14 @@ import std.range; import std.string : format, indexOf; import std.traits : EnumMembers; +deprecated("Use the overload that takes a `PackageName` as 3rd argument") +void parseJson(ref PackageRecipe recipe, Json json, string parent) +{ + const PackageName pname = parent ? PackageName(parent) : PackageName.init; + parseJson(recipe, json, pname); +} -void parseJson(ref PackageRecipe recipe, Json json, string parent_name) +void parseJson(ref PackageRecipe recipe, Json json, in PackageName parent = PackageName.init) { foreach (string field, value; json) { switch (field) { @@ -51,7 +57,9 @@ void parseJson(ref PackageRecipe recipe, Json json, string parent_name) enforce(recipe.name.length > 0, "The package \"name\" field is missing or empty."); - auto fullname = parent_name.length ? parent_name ~ ":" ~ recipe.name : recipe.name; + auto fullname = parent.length + ? PackageName(parent ~ ":" ~ recipe.name) + : PackageName(recipe.name); // parse build settings recipe.buildSettings.parseJson(json, fullname); @@ -110,10 +118,10 @@ Json toJson(const scope ref PackageRecipe recipe) return ret; } -private void parseSubPackages(ref PackageRecipe recipe, string parent_package_name, Json[] subPackagesJson) +private void parseSubPackages(ref PackageRecipe recipe, in PackageName parent, Json[] subPackagesJson) { - enforce(!parent_package_name.canFind(":"), format("'subPackages' found in '%s'. This is only supported in the main package file for '%s'.", - parent_package_name, getBasePackageName(parent_package_name))); + enforce(!parent.sub, format("'subPackages' found in '%s'. This is only supported in the main package file for '%s'.", + parent, parent.main)); recipe.subPackages = new SubPackage[subPackagesJson.length]; foreach (i, subPackageJson; subPackagesJson) { @@ -123,7 +131,7 @@ private void parseSubPackages(ref PackageRecipe recipe, string parent_package_na recipe.subPackages[i] = SubPackage(subpath, PackageRecipe.init); } else { PackageRecipe subinfo; - subinfo.parseJson(subPackageJson, parent_package_name); + subinfo.parseJson(subPackageJson, parent); recipe.subPackages[i] = SubPackage(null, subinfo); } } @@ -376,9 +384,10 @@ unittest { `.strip.outdent; auto jsonValue = parseJsonString(json); PackageRecipe rec1; - parseJson(rec1, jsonValue, null); + parseJson(rec1, jsonValue); PackageRecipe rec; - parseJson(rec, rec1.toJson(), null); // verify that all fields are serialized properly + // verify that all fields are serialized properly + parseJson(rec, rec1.toJson()); assert(rec.name == "projectname"); assert(rec.buildSettings.environments == ["": ["Var1": "env"]]); diff --git a/source/dub/recipe/packagerecipe.d b/source/dub/recipe/packagerecipe.d index 20926eea1..44f34b015 100644 --- a/source/dub/recipe/packagerecipe.d +++ b/source/dub/recipe/packagerecipe.d @@ -50,6 +50,7 @@ deprecated @safe unittest In case of a top level package, the qualified name is returned unmodified. */ +deprecated("Use `dub.dependency : PackageName(arg).main` instead") string getBasePackageName(string package_name) @safe pure { return package_name.findSplit(":")[0]; @@ -62,12 +63,13 @@ string getBasePackageName(string package_name) @safe pure This is the part of the package name excluding the base package name. See also $(D getBasePackageName). */ +deprecated("Use `dub.dependency : PackageName(arg).sub` instead") string getSubPackageName(string package_name) @safe pure { return package_name.findSplit(":")[2]; } -@safe unittest +deprecated @safe unittest { assert(getBasePackageName("packa:packb:packc") == "packa"); assert(getBasePackageName("pack") == "pack");