Utils: Fix error prone default value of appendOrSet

It was not readily clear that Environment::appendOrSet/prependOrSet
needed a value for "sep", otherwise it would just concat the values without separator. This got apparent when looking at usages of
appendOrSet.

Instead there are now three options, "Auto", "Colon" or "Semicolon"
with the default being "Auto", which determines the separator based on
the Environment::OsType.

Usages of appendOrSet and prependOrSet are also fixed with this commit.

Change-Id: I868e14ce38f50be9cd07292fe3f22663a62d116d
Reviewed-by: hjk <hjk@qt.io>

src/libs/utils/environment.cpp

@@ -138,7 +138,7 @@ void Environment::appendOrSetPath(const FilePath &value)
     QTC_CHECK(value.osType() == m_dict.m_osType);
     if (value.isEmpty())
         return;
-    appendOrSet("PATH", value.nativePath(), OsSpecificAspects::pathListSeparator(osType()));
+    appendOrSet("PATH", value.nativePath());
 }
 
 void Environment::prependOrSetPath(const FilePath &value)
@@ -146,20 +146,20 @@ void Environment::prependOrSetPath(const FilePath &value)
     QTC_CHECK(value.osType() == m_dict.m_osType);
     if (value.isEmpty())
         return;
-    prependOrSet("PATH", value.nativePath(), OsSpecificAspects::pathListSeparator(osType()));
+    prependOrSet("PATH", value.nativePath());
 }
 
 void Environment::prependOrSetPath(const QString &directories)
 {
-    prependOrSet("PATH", directories, OsSpecificAspects::pathListSeparator(osType()));
+    prependOrSet("PATH", directories);
 }
 
-void Environment::appendOrSet(const QString &key, const QString &value, const QString &sep)
+void Environment::appendOrSet(const QString &key, const QString &value, PathSeparator sep)
 {
     addItem(Item{std::in_place_index_t<AppendOrSet>(), key, value, sep});
 }
 
-void Environment::prependOrSet(const QString &key, const QString &value, const QString &sep)
+void Environment::prependOrSet(const QString &key, const QString &value, PathSeparator sep)
 {
     addItem(Item{std::in_place_index_t<PrependOrSet>(), key, value, sep});
 }
@@ -167,21 +167,20 @@ void Environment::prependOrSet(const QString &key, const QString &value, const Q
 void Environment::prependOrSetLibrarySearchPath(const FilePath &value)
 {
     QTC_CHECK(value.osType() == osType());
-    const QChar sep = OsSpecificAspects::pathListSeparator(osType());
     switch (osType()) {
     case OsTypeWindows: {
-        prependOrSet("PATH", value.nativePath(), sep);
+        prependOrSet("PATH", value.nativePath());
         break;
     }
     case OsTypeMac: {
         const QString nativeValue = value.nativePath();
-        prependOrSet("DYLD_LIBRARY_PATH", nativeValue, sep);
+        prependOrSet("DYLD_LIBRARY_PATH", nativeValue);
-        prependOrSet("DYLD_FRAMEWORK_PATH", nativeValue, sep);
+        prependOrSet("DYLD_FRAMEWORK_PATH", nativeValue);
         break;
     }
     case OsTypeLinux:
     case OsTypeOtherUnix: {
-        prependOrSet("LD_LIBRARY_PATH", value.nativePath(), sep);
+        prependOrSet("LD_LIBRARY_PATH", value.nativePath());
         break;
     }
     default:
@@ -404,12 +403,10 @@ void Environment::prependToPath(const FilePaths &values)
     m_dict.clear();
     for (int i = values.size(); --i >= 0; ) {
         const FilePath value = values.at(i);
-        m_changeItems.append(Item{
+        m_changeItems.append(Item{std::in_place_index_t<PrependOrSet>(),
-            std::in_place_index_t<PrependOrSet>(),
                                   QString("PATH"),
                                   value.nativePath(),
-            value.pathListSeparator()
+                                  PathSeparator::Auto});
-        });
     }
 }
 
@@ -417,12 +414,10 @@ void Environment::appendToPath(const FilePaths &values)
 {
     m_dict.clear();
     for (const FilePath &value : values) {
-        m_changeItems.append(Item{
+        m_changeItems.append(Item{std::in_place_index_t<AppendOrSet>(),
-            std::in_place_index_t<AppendOrSet>(),
                                   QString("PATH"),
                                   value.nativePath(),
-            value.pathListSeparator()
+                                  PathSeparator::Auto});
-        });
     }
 }
 
@@ -469,7 +464,7 @@ const NameValueDictionary &Environment::resolved() const
                 m_dict.m_values.insert(DictKey(key, m_dict.nameCaseSensitivity()), {value, true});
             } else {
                 // Prepend unless it is already there
-                const QString toPrepend = value + sep;
+                const QString toPrepend = value + pathListSeparator(sep);
                 if (!it.value().first.startsWith(toPrepend))
                     it.value().first.prepend(toPrepend);
             }
@@ -483,7 +478,7 @@ const NameValueDictionary &Environment::resolved() const
                 m_dict.m_values.insert(DictKey(key, m_dict.nameCaseSensitivity()), {value, true});
             } else {
                 // Prepend unless it is already there
-                const QString toAppend = sep + value;
+                const QString toAppend = pathListSeparator(sep) + value;
                 if (!it.value().first.endsWith(toAppend))
                     it.value().first.append(toAppend);
             }
@@ -512,6 +507,15 @@ Environment Environment::appliedToEnvironment(const Environment &base) const
     return res;
 }
 
+QChar Environment::pathListSeparator(PathSeparator sep) const
+{
+    if (sep == PathSeparator::Semicolon)
+        return QLatin1Char(';');
+    else if (sep == PathSeparator::Colon)
+        return QLatin1Char(':');
+    return OsSpecificAspects::pathListSeparator(osType());
+}
+
 /*!
     Returns the value of \a key in \QC's modified system environment.
     \sa Utils::Environment::systemEnvironment

src/libs/utils/environment.h

@@ -22,6 +22,8 @@ namespace Utils {
 class QTCREATOR_UTILS_EXPORT Environment final
 {
 public:
+    enum class PathSeparator { Auto, Colon, Semicolon };
+
     Environment();
     explicit Environment(OsType osType);
     explicit Environment(const QStringList &env, OsType osType = HostOsInfo::hostOs());
@@ -43,8 +45,12 @@ public:
     QStringList toStringList() const;
     QProcessEnvironment toProcessEnvironment() const;
 
-    void appendOrSet(const QString &key, const QString &value, const QString &sep = QString());
+    void appendOrSet(const QString &key,
-    void prependOrSet(const QString &key, const QString &value, const QString &sep = QString());
+                     const QString &value,
+                     PathSeparator sep = PathSeparator::Auto);
+    void prependOrSet(const QString &key,
+                      const QString &value,
+                      PathSeparator sep = PathSeparator::Auto);
 
     void appendOrSetPath(const FilePath &value);
     void prependOrSetPath(const FilePath &value);
@@ -89,6 +95,8 @@ public:
     static void modifySystemEnvironment(const EnvironmentItems &list); // use with care!!!
     static void setSystemEnvironment(const Environment &environment);  // don't use at all!!!
 
+    QChar pathListSeparator(PathSeparator sep) const;
+
     enum Type {
         SetSystemEnvironment,
         SetFixedDictionary,
@@ -107,8 +115,8 @@ public:
         std::tuple<QString, QString, bool>,          // SetValue (key, value, enabled)
         std::tuple<QString, QString>,                // SetFallbackValue (key, value)
         QString,                                     // UnsetValue (key)
-        std::tuple<QString, QString, QString>,   // PrependOrSet (key, value, separator)
+        std::tuple<QString, QString, PathSeparator>, // PrependOrSet (key, value, separator)
-        std::tuple<QString, QString, QString>,   // AppendOrSet (key, value, separator)
+        std::tuple<QString, QString, PathSeparator>, // AppendOrSet (key, value, separator)
         NameValueItems,                              // Modify
         std::monostate,                              // SetupEnglishOutput
         FilePath                                     // SetupSudoAskPass (file path of qtc-askpass or ssh-askpass)

src/plugins/cmakeprojectmanager/presetsmacros.cpp

@@ -139,10 +139,8 @@ void expand(const PresetType &preset, Environment &env, const FilePath &sourceDi
             return presetEnv.value(macroName);
         });
 
-        QString sep;
         bool append = true;
         if (key.compare("PATH", Qt::CaseInsensitive) == 0) {
-            sep = OsSpecificAspects::pathListSeparator(env.osType());
             const int index = value.indexOf("$penv{PATH}", 0, Qt::CaseInsensitive);
             if (index != 0)
                 append = false;
@@ -157,9 +155,9 @@ void expand(const PresetType &preset, Environment &env, const FilePath &sourceDi
         expandAllButEnv(preset, sourceDirectory, value);
 
         if (append)
-            env.appendOrSet(key, value, sep);
+            env.appendOrSet(key, value);
         else
-            env.prependOrSet(key, value, sep);
+            env.prependOrSet(key, value);
     });
 }
 

src/plugins/debugger/cdb/cdbengine.cpp

@@ -410,11 +410,10 @@ void CdbEngine::setupEngine()
         inferiorEnvironment.set(qtLoggingToConsoleKey, "0");
 
     static const char cdbExtensionPathVariableC[] = "_NT_DEBUGGER_EXTENSION_PATH";
-    const QString pSep = OsSpecificAspects::pathListSeparator(Utils::OsTypeWindows);
+    inferiorEnvironment.prependOrSet(cdbExtensionPathVariableC, extensionFi.absolutePath());
-    inferiorEnvironment.prependOrSet(cdbExtensionPathVariableC, extensionFi.absolutePath(), pSep);
     const QString oldCdbExtensionPath = qtcEnvironmentVariable(cdbExtensionPathVariableC);
     if (!oldCdbExtensionPath.isEmpty())
-        inferiorEnvironment.appendOrSet(cdbExtensionPathVariableC, oldCdbExtensionPath, pSep);
+        inferiorEnvironment.appendOrSet(cdbExtensionPathVariableC, oldCdbExtensionPath);
 
     m_process.setEnvironment(inferiorEnvironment);
     if (!sp.inferior.workingDirectory.isEmpty())

src/plugins/debugger/dap/pydapengine.cpp

@@ -83,9 +83,7 @@ public:
         Environment env = m_runParameters.debugger.environment;
         const FilePath debugPyDir = packageDir(m_cmd.executable(), "debugpy");
         if (QTC_GUARD(debugPyDir.isSameDevice(m_cmd.executable()))) {
-            env.appendOrSet("PYTHONPATH",
+            env.appendOrSet("PYTHONPATH", debugPyDir.path());
-                            debugPyDir.needsDevice() ? debugPyDir.path() : debugPyDir.toUserOutput(),
-                            OsSpecificAspects::pathListSeparator(debugPyDir.osType()));
         }
 
         m_proc.setEnvironment(env);

src/plugins/python/pythonlanguageclient.cpp

@@ -109,15 +109,11 @@ protected:
         Environment env = python.deviceEnvironment();
         const FilePath lspPath = pyLspPath(python);
         if (!lspPath.isEmpty() && lspPath.exists() && QTC_GUARD(lspPath.isSameDevice(python))) {
-            env.appendOrSet("PYTHONPATH",
+            env.appendOrSet("PYTHONPATH", lspPath.path());
-                            lspPath.path(),
-                            OsSpecificAspects::pathListSeparator(env.osType()));
         }
         if (!python.needsDevice()) {
             // todo check where to put this tempdir in remote setups
-            env.appendOrSet("PYTHONPATH",
+            env.appendOrSet("PYTHONPATH", m_extraPythonPath.path().toString());
-                            m_extraPythonPath.path().toString(),
-                            OsSpecificAspects::pathListSeparator(env.osType()));
         }
         if (env.hasChanges())
             setEnvironment(env);

src/plugins/qmldesigner/puppetenvironmentbuilder.cpp

@@ -223,7 +223,7 @@ void PuppetEnvironmentBuilder::addImportPaths() const
         importPaths.prepend(QLibraryInfo::path(QLibraryInfo::Qml2ImportsPath));
 
     constexpr auto pathSep = Utils::HostOsInfo::pathListSeparator();
-    m_environment.appendOrSet("QML2_IMPORT_PATH", importPaths.join(pathSep), pathSep);
+    m_environment.appendOrSet("QML2_IMPORT_PATH", importPaths.join(pathSep));
 
     qCInfo(puppetEnvirmentBuild) << "Puppet import paths:" << importPaths;
 }
@@ -237,9 +237,9 @@ void PuppetEnvironmentBuilder::addCustomFileSelectors() const
 
     customFileSelectors.append("DesignMode");
 
-    constexpr auto pathSep = Utils::HostOsInfo::pathListSeparator();
     if (!customFileSelectors.isEmpty())
-        m_environment.appendOrSet("QML_FILE_SELECTORS", customFileSelectors.join(","), pathSep);
+        m_environment.appendOrSet("QML_FILE_SELECTORS",
+                                  customFileSelectors.join(","));
 
     qCInfo(puppetEnvirmentBuild) << "Puppet selectors:" << customFileSelectors;
 }

src/plugins/qmljstools/qmljsmodelmanager.cpp

@@ -155,7 +155,9 @@ ModelManagerInterface::ProjectInfo ModelManager::defaultProjectInfoForProject(
             // Append QML2_IMPORT_PATH if it is defined in build configuration.
             // It enables qmlplugindump to correctly dump custom plugins or other dependent
             // plugins that are not installed in default Qt qml installation directory.
-            projectInfo.qmlDumpEnvironment.appendOrSet("QML2_IMPORT_PATH", bc->environment().expandedValueForKey("QML2_IMPORT_PATH"), ":");
+            projectInfo.qmlDumpEnvironment.appendOrSet("QML2_IMPORT_PATH",
+                                                       bc->environment().expandedValueForKey(
+                                                           "QML2_IMPORT_PATH"));
             // Treat every target (library or application) in the build directory
 
             FilePath dir = bc->buildDirectory();

src/plugins/qnx/qnxrunconfiguration.cpp

@@ -63,10 +63,10 @@ public:
         setRunnableModifier([this](ProcessRunData &r) {
             QString libPath = qtLibraries();
             if (!libPath.isEmpty()) {
-                r.environment.appendOrSet("LD_LIBRARY_PATH", libPath + "/lib:$LD_LIBRARY_PATH");
+                r.environment.prependOrSet("LD_LIBRARY_PATH", libPath + "/lib");
-                r.environment.appendOrSet("QML_IMPORT_PATH", libPath + "/imports:$QML_IMPORT_PATH");
+                r.environment.prependOrSet("QML_IMPORT_PATH", libPath + "/imports");
-                r.environment.appendOrSet("QML2_IMPORT_PATH", libPath + "/qml:$QML2_IMPORT_PATH");
+                r.environment.prependOrSet("QML2_IMPORT_PATH", libPath + "/qml");
-                r.environment.appendOrSet("QT_PLUGIN_PATH", libPath + "/plugins:$QT_PLUGIN_PATH");
+                r.environment.prependOrSet("QT_PLUGIN_PATH", libPath + "/plugins");
                 r.environment.set("QT_QPA_FONTDIR", libPath + "/lib/fonts");
             }
         });

src/plugins/terminal/shellintegration.cpp

@@ -184,17 +184,13 @@ void ShellIntegration::prepareProcess(Utils::Process &process)
         rcPath.copyFile(tmpRc);
 
         env.set("CLINK_HISTORY_LABEL", "QtCreator");
-        env.appendOrSet("CLINK_PATH",
+        env.appendOrSet("CLINK_PATH", tmpRc.parentDir().nativePath());
-                        tmpRc.parentDir().nativePath(),
-                        OsSpecificAspects::pathListSeparator(env.osType()));
     } else if (cmd.executable().baseName() == "fish") {
         FilePath xdgDir = FilePath::fromUserInput(m_tempDir.filePath("fish_xdg_data"));
         FilePath subDir = xdgDir.resolvePath(QString("fish/vendor_conf.d"));
         QTC_ASSERT(subDir.createDir(), return);
         filesToCopy.fish.script.copyFile(subDir.resolvePath(filesToCopy.fish.script.fileName()));
-        env.appendOrSet("XDG_DATA_DIRS",
+        env.appendOrSet("XDG_DATA_DIRS", xdgDir.toUserOutput());
-                        xdgDir.toUserOutput(),
-                        OsSpecificAspects::pathListSeparator(env.osType()));
     }
 
     process.setCommand(cmd);

tests/auto/environment/tst_environment.cpp

@@ -363,12 +363,10 @@ void tst_Environment::pathChanges()
     QFETCH(QString, value);
     QFETCH(Environment, expected);
 
-    const QString sep = OsSpecificAspects::pathListSeparator(environment.osType());
-
     if (prepend)
-        environment.prependOrSet(variable, value, sep);
+        environment.prependOrSet(variable, value);
     else
-        environment.appendOrSet(variable, value, sep);
+        environment.appendOrSet(variable, value);
 
     qDebug() << "Actual  :" << environment.toStringList();
     qDebug() << "Expected:" << expected.toStringList();