Don't return error for settings set .experimental. settings that are absent

[jasonmolenda]

Summary

setting paths that include .experimental. are intended for settings that may be promoted to "real" settings in the future, or may be removed. When users put these settings in their ~/.lldbinit files, we don't want to emit an error if the setting has gone away. And if the setting has been promoted to a real setting, we want to do the right thing.

The first part of that -- not emitting an error -- did not work correctly. I fixed that, and added tests to check all of these behaviors.

[labath]

packages/Python/lldbsuite/test/settings/TestSettings.py

544–545 ↗

(On Diff #141263)

Isn't this basically what self.expect would do (only with better logging and error messages)?

source/Interpreter/OptionValueProperties.cpp

210–215 ↗

(On Diff #141263)

Not a big deal, but I would expect that the magicness of experimental would kick in only for the components which are come after experimental keyword.
So something like target.experimental.non-existant-setting should be subject to the magic behavior, but target.non-existant-setting.experimental.foo should not (?)

[jasonmolenda]

rewrote the test cases in terms of self.expect. Haven't looked at being totally correct with flagging paths with .experimental. as never-errored.

lldb/trunk/packages/Python/lldbsuite/test/settings/TestSettings.py

def test_all_settings_exist(self):
                              "target.disable-stdio",
                              "target.x86-disassembly-flavor",
                              "target.use-hex-immediates",
                              "target.hex-immediate-style",
                              "target.process.disable-memory-cache",
                              "target.process.extra-startup-command",
                              "target.process.thread.step-avoid-regexp",
                              "target.process.thread.trace-thread"])
+
+    # settings under an ".experimental" domain should have two properties:
+    #   1. If the name does not exist with "experimental" in the name path,
+    #      the name lookup should try to find it without "experimental".  So
+    #      a previously-experimental setting that has been promoted to a 
+    #      "real" setting will still be set by the original name.
+    #   2. Changing a setting with .experimental., name, where the setting
+    #      does not exist either with ".experimental." or without, should
+    #      not generate an error.  So if an experimental setting is removed,
+    #      people who may have that in their ~/.lldbinit files should not see
+    #      any errors.
+    def test_experimental_settings(self):
+        cmdinterp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+
+        # Set target.arg0 to a known value, check that we can retrieve it via
+        # the actual name and via .experimental.
+        self.expect('settings set target.arg0 first-value')
+        self.expect('settings show target.arg0', substrs=['first-value'])
+        self.expect('settings show target.experimental.arg0', substrs=['first-value'], error=False)
+
+        # Set target.arg0 to a new value via a target.experimental.arg0 name,
+        # verify that we can read it back via both .experimental., and not.
+        self.expect('settings set target.experimental.arg0 second-value', error=False)
+        self.expect('settings show target.arg0', substrs=['second-value'])
+        self.expect('settings show target.experimental.arg0', substrs=['second-value'], error=False)
+
+        # showing & setting an undefined .experimental. setting should generate no errors.
+        self.expect('settings show target.experimental.setting-which-does-not-exist', patterns=['^\s$'], error=False)
+        self.expect('settings set target.experimental.setting-which-does-not-exist true', error=False)
+
+        # A domain component before .experimental. which does not exist should give an error
+        # But the code does not yet do that.
+        # self.expect('settings set target.setting-which-does-not-exist.experimental.arg0 true', error=True)
+
+        # finally, confirm that trying to set a setting that does not exist still fails.
+        # (SHOWING a setting that does not exist does not currently yield an error.)
+        self.expect('settings set target.setting-which-does-not-exist true', error=True)

lldb/trunk/source/Interpreter/OptionValueProperties.cpp

 }
 
 Status OptionValueProperties::SetSubValue(const ExecutionContext *exe_ctx,
                                           VarSetOperationType op,
                                           llvm::StringRef name,
                                           llvm::StringRef value) {
   Status error;
   const bool will_modify = true;
+  llvm::SmallVector<llvm::StringRef, 8> components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+  for (const auto &part : components)
+    if (Properties::IsSettingExperimental(part))
+      name_contains_experimental = true;
+
+  
   lldb::OptionValueSP value_sp(GetSubValue(exe_ctx, name, will_modify, error));
   if (value_sp)
     error = value_sp->SetValueFromString(value, op);
   else {
-    if (error.AsCString() == nullptr)
+    // Don't set an error if the path contained .experimental. - those are
+    // allowed to be missing and should silently fail.
+    if (name_contains_experimental == false && error.AsCString() == nullptr) {
       error.SetErrorStringWithFormat("invalid value path '%s'", name.str().c_str());
     }
+  }
   return error;
 }
 
 uint32_t
 OptionValueProperties::GetPropertyIndex(const ConstString &name) const {
   return m_name_to_index.Find(name, SIZE_MAX);
 }