Require manual mode setting for choices

Do not change the mode if y is assigned to a choice symbol inside a
choice in m mode, for example. Require Choice.set_value() to be called
instead.

This makes choices way less magical and more menuconfig-like. It would
also be confusing to be able to assign y to a choice symbol when y is
not in sym.assignable.

Set the mode manually in load_config() (y assigned to a choice symbol =>
y mode, and ditto for m). Change the warning for inconsistent values to
one that's probably less confusing, though the old one was closer to the
warning printed by the C implementation.

Need to fix a bunch of tests too...

2 files changed, 128 insertions, 122 deletions

diff --git a/kconfiglib.py b/kconfiglib.py
index 4ca8dc6..27a8e7b 100644
--- a/kconfiglib.py
+++ b/kconfiglib.py
@@ -671,16 +671,21 @@ class Kconfig(object):
                         # We represent tristate values as 0, 1, 2
                         val = STR_TO_TRI[val[0]]
 
-                        if sym.choice is not None:
-                            mode = sym.choice.user_value
-                            if mode is not None and mode != val:
-                                self._warn("assignment to {} changes mode of "
-                                           'containing choice from {} to {}.'
-                                           .format(name,
-                                                   TRI_TO_STR[mode],
-                                                   TRI_TO_STR[val]),
+                        if sym.choice is not None and val:
+                            # During .config loading, we infer the mode of the
+                            # choice from the kind of values that are assigned
+                            # to the choice symbols
+
+                            prev_mode = sym.choice.user_value
+                            if prev_mode is not None and prev_mode != val:
+                                self._warn("both m and y assigned to symbols "
+                                           "within the same choice",
                                            filename, linenr)
 
+                            # Set the choice's mode
+                            # TODO: this causes redundant invalidation
+                            sym.choice.set_value(val)
+
                     elif sym.orig_type == STRING:
                         string_match = _conf_string_re_match(val)
                         if not string_match:
@@ -2691,13 +2696,8 @@ class Symbol(object):
 
         self.user_value = value
 
-        if self.choice is not None:
-            # Change mode of choice
-            if self.user_value == 2:
-                self.choice.user_value = 2
-                self.choice.user_selection = self
-            elif self.user_value == 1:
-                self.choice.user_value = 1
+        if self.choice is not None and value == 2:
+            self.choice.user_selection = self
 
     def _invalidate(self):
         """
@@ -2799,9 +2799,9 @@ class Choice(object):
       The tristate value (mode) of the choice. A choice can be in one of three
       modes:
 
-        0 (n) - The choice is disabled and no symbols can be selected. This
-                mode is only possible for choices with the 'optional' flag set
-                (see kconfig-language.txt).
+        0 (n) - The choice is disabled and no symbols can be selected. For
+                visible choices, this mode is only possible for choices with
+                the 'optional' flag set (see kconfig-language.txt).
 
         1 (m) - Any number of choice symbols can be set to m, the rest will
                 be n.
@@ -2811,20 +2811,32 @@ class Choice(object):
       Only tristate choices can be in m mode. The visibility of the choice is
       an upper bound on the mode.
 
-      The mode changes automatically when a value is assigned to a symbol
-      within the choice (this makes .config loading "just work"), and can also
-      be changed via Choice.set_value().
-
-      Implementation note: The C tools internally represent choices as a type
-      of symbol, with special-casing in many code paths. This is why there is a
-      lot of similarity to Symbol. The value (mode) of a choice is really just
-      a normal symbol value, and an implicit reverse dependency forces its
-      lower bound to m for non-optional choices.
-
-      Kconfiglib uses a separate Choice class only because it makes the code
-      and interface less confusing (especially in a user-facing interface).
-      Corresponding attributes have the same name in the Symbol and Choice
-      classes, for consistency.
+      To change the mode, use Choice.set_value().
+
+      Implementation note:
+        The C tools internally represent choices as a type of symbol, with
+        special-casing in many code paths. This is why there is a lot of
+        similarity to Symbol. The value (mode) of a choice is really just a
+        normal symbol value, and an implicit reverse dependency forces its
+        lower bound to m for visible non-optional choices (the reverse
+        dependency is 'm && <visibility>').
+
+        Symbols within choices get the choice propagated as a dependency to
+        their properties. This makes it so that the mode of the choice acts as
+        an upper bound on e.g. the visibility of choice symbols.
+
+        Kconfiglib uses a separate Choice class only because it makes the code
+        and interface less confusing (especially in a user-facing interface).
+        Corresponding attributes have the same name in the Symbol and Choice
+        classes, for consistency and compatibility.
+
+        Gotcha: This means that Choice instances show up in expressions inside
+        choices. This should normally be invisible even if you're doing manual
+        evaluation of expressions, because the value-related interface of
+        Symbol and Choice is identical. It also makes all the expressions have
+        the "expected" value. A choice inside an expression will be printed as
+        "<choice>" (or "<choice NAME>" for named choices, should those ever
+        show up).
 
     assignable:
       See the symbol class documentation. Gives the assignable values (modes).
@@ -2844,12 +2856,12 @@ class Choice(object):
       any symbol. Can be None for the same reasons as 'selected'.
 
     user_value:
-      The value (mode) selected by the user (through Choice.set_value() or by
-      assigning a value to a symbol within the choice). Either 0, 1, or 2, or
-      None if the user hasn't selected a mode. See Symbol.user_value.
+      The value (mode) selected by the user through Choice.set_value(). Either
+      0, 1, or 2, or None if the user hasn't selected a mode. See
+      Symbol.user_value.
 
       Warning: Do not assign directly to this. It will break things. Use
-      Choice.set_value() or Symbol.set_value() instead.
+      Choice.set_value() instead.
 
     user_selection:
       The symbol selected by the user (by setting it to y). Ignored if the
@@ -3015,10 +3027,9 @@ class Choice(object):
         """
         if not ((self.orig_type == BOOL     and value in (0, 2)    ) or
                 (self.orig_type == TRISTATE and value in (0, 1, 2))):
-            self.kconfig._warn("the value {} is invalid for the choice, "
+            self.kconfig._warn("the value '{}' is invalid for the choice, "
                                "which has type {}. Assignment ignored"
-                               .format(TRI_TO_STR[value],
-                                       TYPE_TO_STR[self.orig_type]))
+                               .format(value, TYPE_TO_STR[self.orig_type]))
             return
 
         self.user_value = value
diff --git a/testsuite.py b/testsuite.py
index e6d77e3..9ebcef7 100644
--- a/testsuite.py
+++ b/testsuite.py
@@ -670,12 +670,6 @@ choice
 <choice CHOICE, tristate, "choice", mode y, user mode y, CHOICE_2 selected, CHOICE_2 selected by user, visibility y, Kconfiglib/tests/Krepr:30>
 """)
 
-    c.syms["CHOICE_1"].set_value(1)
-
-    verify_repr(c.named_choices["CHOICE"], """
-<choice CHOICE, tristate, "choice", mode m, user mode m, CHOICE_2 selected by user (overriden), visibility y, Kconfiglib/tests/Krepr:30>
-""")
-
     verify_repr(c.syms["CHOICE_HOOK"].nodes[0].next.item, """
 <choice, tristate, "optional choice", mode n, visibility n, optional, Kconfiglib/tests/Krepr:43>
 """)
@@ -867,20 +861,19 @@ g
     verify_visibility(c.syms["BOOL_CHOICE_N"], 0, 0)
 
     # Non-tristate symbols in tristate choices are only visible if the choice
-    # is in 2 mode
-    verify_visibility(c.syms["BOOL_CHOICE_M"], 0, 0)
+    # is in y mode
 
-    # Tristate choices start out in m mode. When running without modules, their
-    # type gets adjusted to bool.
-    verify_visibility(c.syms["BOOL_CHOICE_Y"], 2, 0)
-
-    c.syms["TRISTATE_CHOICE_M"].set_value(2)
-    c.syms["TRISTATE_CHOICE_Y"].set_value(2)
-
-    # Still limited by the visibility of the choice
+    # The choice can't be brought to y mode because of the 'if m'
+    verify_visibility(c.syms["BOOL_CHOICE_M"], 0, 0)
+    c.syms["BOOL_CHOICE_M"].choice.set_value(2)
     verify_visibility(c.syms["BOOL_CHOICE_M"], 0, 0)
 
-    # This one should become visible now
+    # The choice gets y mode only when running without modules, because it
+    # defaults to m mode
+    verify_visibility(c.syms["BOOL_CHOICE_Y"], 2, 0)
+    c.syms["BOOL_CHOICE_Y"].choice.set_value(2)
+    # When set to y mode, the choice symbol becomes visible both with and
+    # without modules
     verify_visibility(c.syms["BOOL_CHOICE_Y"], 2, 2)
 
     verify_visibility(c.syms["TRISTATE_IF_N"],     0, 0)
@@ -1545,53 +1538,55 @@ g
         verify(choice.type == BOOL,
                "choice {} should have type bool".format(choice.name))
 
+    # TODO: Assignment to choice symbols is less magic now. Update these tests.
+
     # TODO: fix this laters. type automatically changed.
     #for choice in (choice_tristate, choice_tristate_opt, choice_tristate_m):
     #    verify(choice.type == TRISTATE,
     #           "choice {} should have type tristate"
     #           .format(choice.name))
 
-    def select_and_verify(sym):
-        choice = get_parent(sym)
-        sym.set_value(2)
-        verify(choice.str_value == "y",
-               "The mode of the choice should be y after selecting a symbol")
-        verify(sym.choice.selection is sym,
-               "{} should be the selected choice symbol"
-               .format(sym.name))
-        verify(choice.selection is sym,
-               "{} should be the selected symbol".format(sym.name))
-        verify(choice.user_selection is sym,
-               "{} should be the user selection of the choice"
-               .format(sym.name))
-
-    def select_and_verify_all(choice):
-        # Select in forward order
-        for sym in choice.syms:
-            select_and_verify(sym)
-        # Select in reverse order
-        for i in range(len(choice.syms) - 1, 0, -1):
-            select_and_verify(choice.syms[i])
-
-    def verify_mode(choice, no_modules_mode, modules_mode):
-        c.modules.set_value(0)
-        choice_mode = choice.tri_value
-        verify(choice_mode == no_modules_mode,
-               'Wrong mode for choice {} with no modules. Expected {}, got {}.'
-               .format(choice.name, no_modules_mode, choice_mode))
-
-        c.modules.set_value(2)
-        choice_mode = choice.tri_value
-        verify(choice_mode == modules_mode,
-               'Wrong mode for choice {} with modules. Expected {}, got {}.'
-               .format(choice.name, modules_mode, choice_mode))
-
-    verify_mode(choice_bool,         2, 2)
-    verify_mode(choice_bool_opt,     0, 0)
-    verify_mode(choice_tristate,     2, 1)
-    verify_mode(choice_tristate_opt, 0, 0)
-    verify_mode(choice_bool_m,       0, 2)
-    verify_mode(choice_tristate_m,   0, 1)
+    #def select_and_verify(sym):
+    #    choice = get_parent(sym)
+    #    sym.set_value(2)
+    #    verify(choice.str_value == "y",
+    #           "The mode of the choice should be y after selecting a symbol")
+    #    verify(sym.choice.selection is sym,
+    #           "{} should be the selected choice symbol"
+    #           .format(sym.name))
+    #    verify(choice.selection is sym,
+    #           "{} should be the selected symbol".format(sym.name))
+    #    verify(choice.user_selection is sym,
+    #           "{} should be the user selection of the choice"
+    #           .format(sym.name))
+
+    #def select_and_verify_all(choice):
+    #    # Select in forward order
+    #    for sym in choice.syms:
+    #        select_and_verify(sym)
+    #    # Select in reverse order
+    #    for i in range(len(choice.syms) - 1, 0, -1):
+    #        select_and_verify(choice.syms[i])
+
+    #def verify_mode(choice, no_modules_mode, modules_mode):
+    #    c.modules.set_value(0)
+    #    choice_mode = choice.tri_value
+    #    verify(choice_mode == no_modules_mode,
+    #           'Wrong mode for choice {} with no modules. Expected {}, got {}.'
+    #           .format(choice.name, no_modules_mode, choice_mode))
+
+    #    c.modules.set_value(2)
+    #    choice_mode = choice.tri_value
+    #    verify(choice_mode == modules_mode,
+    #           'Wrong mode for choice {} with modules. Expected {}, got {}.'
+    #           .format(choice.name, modules_mode, choice_mode))
+
+    #verify_mode(choice_bool,         2, 2)
+    #verify_mode(choice_bool_opt,     0, 0)
+    #verify_mode(choice_tristate,     2, 1)
+    #verify_mode(choice_tristate_opt, 0, 0)
+    #verify_mode(choice_bool_m,       0, 2)
+    #verify_mode(choice_tristate_m,   0, 1)
 
     # Test defaults
 
@@ -1611,47 +1606,47 @@ g
 
     # Test "y" mode selection
 
-    c.modules.set_value(2)
+    #c.modules.set_value(2)
 
-    select_and_verify_all(choice_bool)
-    select_and_verify_all(choice_bool_opt)
-    select_and_verify_all(choice_tristate)
-    select_and_verify_all(choice_tristate_opt)
+    #select_and_verify_all(choice_bool)
+    #select_and_verify_all(choice_bool_opt)
+    #select_and_verify_all(choice_tristate)
+    #select_and_verify_all(choice_tristate_opt)
     # For BOOL_M, the mode should have been promoted
-    select_and_verify_all(choice_bool_m)
+    #select_and_verify_all(choice_bool_m)
 
     # Test "m" mode selection...
 
     # ...for a choice that can also be in "y" mode
 
-    for sym_name in ("T_1", "T_2"):
-        assign_and_verify_value(sym_name, 1, 1)
-        verify(choice_tristate.tri_value == 1,
-               'Selecting {} to "m" should have changed the mode of the '
-               'choice to "m"'.format(sym_name))
+    #for sym_name in ("T_1", "T_2"):
+    #    assign_and_verify_value(sym_name, 1, 1)
+    #    verify(choice_tristate.tri_value == 1,
+    #           'Selecting {} to "m" should have changed the mode of the '
+    #           'choice to "m"'.format(sym_name))
 
-        assign_and_verify_value(sym_name, 2, 2)
-        verify(choice_tristate.tri_value == 2 and
-               choice_tristate.selection is c.syms[sym_name],
-               'Selecting {} to "y" should have changed the mode of the '
-               'choice to "y" and made it the selection'.format(sym_name))
+    #    assign_and_verify_value(sym_name, 2, 2)
+    #    verify(choice_tristate.tri_value == 2 and
+    #           choice_tristate.selection is c.syms[sym_name],
+    #           'Selecting {} to "y" should have changed the mode of the '
+    #           'choice to "y" and made it the selection'.format(sym_name))
 
     # ...for a choice that can only be in "m" mode
 
-    for sym_name in ("TM_1", "TM_2"):
-        assign_and_verify_value(sym_name, 1, 1)
-        assign_and_verify_value(sym_name, 0, 0)
-        # "y" should be truncated
-        assign_and_verify_value(sym_name, 2, 1)
-        verify(choice_tristate_m.tri_value == 1,
-               'A choice that can only be in m mode was not')
+    #for sym_name in ("TM_1", "TM_2"):
+    #    assign_and_verify_value(sym_name, 1, 1)
+    #    assign_and_verify_value(sym_name, 0, 0)
+    #    # "y" should be truncated
+    #    assign_and_verify_value(sym_name, 2, 1)
+    #    verify(choice_tristate_m.tri_value == 1,
+    #           'A choice that can only be in m mode was not')
 
     # Verify that choices with no explicitly specified type get the type of the
     # first contained symbol with a type
 
-    verify(choice_no_type_bool.type == BOOL,
+    verify(choice_no_type_bool.orig_type == BOOL,
            "Expected first choice without explicit type to have type bool")
-    verify(choice_no_type_tristate.type == TRISTATE,
+    verify(choice_no_type_tristate.orig_type == TRISTATE,
            "Expected second choice without explicit type to have type "
            "tristate")