Add some post-parsing warnings

These are easiest to check after parsing, since a symbol/choice can be
defined in multiple locations:

 - Warn if a symbol or choice defined without a type. Also warn for
   choice value symbols defined without a type, even if they
   automatically get their type from the choice. This feature isn't
   well-known and probably not used deliberately.

 - Warn if a choice is defined without a prompt

 - Warn of a choice default symbol is not contained in the choice

Also move _name_and_loc_str() from the symbol class to the global scope
and generalize it to be able to handle choices.

7 files changed, 72 insertions, 26 deletions

diff --git a/kconfiglib.py b/kconfiglib.py
index 164cf62..e68e58f 100644
--- a/kconfiglib.py
+++ b/kconfiglib.py
@@ -701,7 +701,7 @@ class Kconfig(object):
                             self._warn("'{}' is not a valid value for the {} "
                                        "symbol {}. Assignment ignored."
                                        .format(val, TYPE_TO_STR[sym.orig_type],
-                                               sym._name_and_loc_str()))
+                                               _name_and_loc_str(sym)))
                             continue
 
                         # We represent tristate values as 0, 1, 2
@@ -726,7 +726,7 @@ class Kconfig(object):
                         if not string_match:
                             self._warn("Malformed string literal in "
                                        "assignment to {}. Assignment ignored."
-                                       .format(sym._name_and_loc_str()),
+                                       .format(_name_and_loc_str(sym)),
                                        filename, linenr)
                             continue
 
@@ -761,7 +761,7 @@ class Kconfig(object):
                         display_user_val = sym.user_value
 
                     warn_msg = '{} set more than once. Old value: "{}", new value: "{}".'.format(
-                        sym._name_and_loc_str(), display_user_val, display_val
+                        _name_and_loc_str(sym), display_user_val, display_val
                     )
 
                     if display_user_val == display_val:
@@ -2708,7 +2708,7 @@ class Symbol(object):
                       "Assignment ignored." \
                       .format(TRI_TO_STR[value] if value in (0, 1, 2) else
                                  "'{}'".format(value),
-                              self._name_and_loc_str(),
+                              _name_and_loc_str(self),
                               TYPE_TO_STR[self.orig_type])
 
             if self.orig_type in (BOOL, TRISTATE) and value in ("n", "m", "y"):
@@ -2721,7 +2721,7 @@ class Symbol(object):
         if self.env_var is not None:
             self.kconfig._warn("ignored attempt to assign user value to "
                                "{}, which gets its value from the environment"
-                               .format(self._name_and_loc_str()))
+                               .format(_name_and_loc_str(self)))
             return False
 
         if self.choice and value == 2:
@@ -2976,7 +2976,7 @@ class Symbol(object):
                 return
 
         if self.kconfig._warn_no_prompt:
-            self.kconfig._warn(self._name_and_loc_str() + " has no prompt, "
+            self.kconfig._warn(_name_and_loc_str(self) + " has no prompt, "
                                "meaning user values have no effect on it")
 
     def _warn_select_unsatisfied_deps(self):
@@ -2987,7 +2987,7 @@ class Symbol(object):
         """
         warn_msg = "{} has unsatisfied direct dependencies ({}), but is " \
                    "currently being selected by the following symbols:" \
-                   .format(self._name_and_loc_str(),
+                   .format(_name_and_loc_str(self),
                            expr_str(self.direct_dep))
 
         # Returns a warning string if 'select' is actually selecting us, and
@@ -3009,7 +3009,7 @@ class Symbol(object):
 
             msg = "\n{}, with value {}, direct dependencies {} " \
                   "(value: {})" \
-                  .format(selecting_sym._name_and_loc_str(),
+                  .format(_name_and_loc_str(selecting_sym),
                           selecting_sym.str_value,
                           expr_str(selecting_sym.direct_dep),
                           TRI_TO_STR[expr_value(selecting_sym.direct_dep)])
@@ -3040,18 +3040,6 @@ class Symbol(object):
 
         self.kconfig._warn(warn_msg)
 
-    def _name_and_loc_str(self):
-        """
-        Helper for giving the symbol name and location(s) in e.g. warnings
-        """
-        if not self.nodes:
-            return self.name + " (undefined)"
-
-        return "{} (defined at {})".format(
-            self.name,
-            ", ".join("{}:{}".format(node.filename, node.linenr)
-                      for node in self.nodes))
-
 class Choice(object):
     """
     Represents a choice statement:
@@ -3999,6 +3987,22 @@ def _sym_choice_str(sc):
 
     return "\n".join(lines) + "\n"
 
+def _name_and_loc_str(sc):
+    """
+    Helper for giving the symbol/choice name and location(s) in e.g.
+    warnings
+    """
+    name = sc.name or "<choice>"
+
+    if not sc.nodes:
+        return name + " (undefined)"
+
+    return "{} (defined at {})".format(
+        name,
+        ", ".join("{}:{}".format(node.filename, node.linenr)
+                  for node in sc.nodes))
+
+
 # Menu manipulation
 
 def _expr_depends_on(expr, sym):
@@ -4147,6 +4151,8 @@ def _finalize_tree(node):
             node.next = cur.next
             cur.next = None
 
+        _check_sanity(node.item)
+
 
     if node.list:
         # We have a node with child nodes where the child nodes are now
@@ -4158,6 +4164,32 @@ def _finalize_tree(node):
     # Empty choices (node.list None) are possible, so this needs to go outside
     if isinstance(node.item, Choice):
         _finalize_choice(node)
+        _check_sanity(node.item)
+
+def _check_sanity(sc):
+    """
+    Prints warnings for bad things that are easiest to check after parsing
+
+    sc: Symbol or Choice
+    """
+    if sc.type == UNKNOWN:
+        sc.kconfig._warn("{} defined without a type"
+                         .format(_name_and_loc_str(sc)))
+
+    if isinstance(sc, Choice):
+        for node in sc.nodes:
+            if node.prompt:
+                break
+        else:
+            sc.kconfig._warn("{} defined without a prompt"
+                             .format(_name_and_loc_str(sc)))
+
+        for sym, _ in sc.defaults:
+            if sym.choice is not sc:
+                sc.kconfig._warn("the default selection {} of {} is not "
+                                 "contained in the choice"
+                                 .format(_name_and_loc_str(sym),
+                                         _name_and_loc_str(sc)))
 
 #
 # Public global constants
diff --git a/tests/Kassignable b/tests/Kassignable
index f134a74..26b4177 100644
--- a/tests/Kassignable
+++ b/tests/Kassignable
@@ -9,6 +9,7 @@ if UNDEFINED && "const"
 endif
 
 config NO_PROMPT
+	bool
 
 config STRING
 	string "string"
diff --git a/tests/Kdefconfig_existent_but_n b/tests/Kdefconfig_existent_but_n
index 17b8980..2d55e97 100644
--- a/tests/Kdefconfig_existent_but_n
+++ b/tests/Kdefconfig_existent_but_n
@@ -2,6 +2,7 @@
 # Should produce None due to the "depends on n"
 
 config FOO
+    string
     option env="BAR"
 
 config A
diff --git a/tests/Khelp b/tests/Khelp
index fdc81ed..c201176 100644
--- a/tests/Khelp
+++ b/tests/Khelp
@@ -1,4 +1,5 @@
 config TWO_HELP_STRINGS
+    bool
     help
       first help string
 
@@ -10,10 +11,12 @@ config TWO_HELP_STRINGS
       second help string
 
 config NO_BLANK_AFTER_HELP
+    bool
     help
       help for
       NO_BLANK_AFTER_HELP
 choice CHOICE_HELP
+    bool "choice with help"
     help
   help for
   CHOICE_HELP
diff --git a/tests/Klocation b/tests/Klocation
index 3901ebf..00d4f4a 100644
--- a/tests/Klocation
+++ b/tests/Klocation
@@ -2,8 +2,10 @@ if UNDEFINED
 endif
 
 config SINGLE_DEF
+    bool
 
 config MULTI_DEF
+    bool
 
 # Throw in some line continuations too to make sure it doesn't mess up the line
 # numbers
diff --git a/tests/Klocation_included b/tests/Klocation_included
index fb1afaa..6e19f76 100644
--- a/tests/Klocation_included
+++ b/tests/Klocation_included
@@ -3,13 +3,16 @@
 config MULTI_DEF
 
 choice CHOICE
+	bool "choice"
 endchoice
 
 config MENU_HOOK
+	bool
 
 menu "menu"
 endmenu
 
 config COMMENT_HOOK
+	bool
 
 comment "comment"
diff --git a/testsuite.py b/testsuite.py
index a250c6e..083568d 100644
--- a/testsuite.py
+++ b/testsuite.py
@@ -801,19 +801,19 @@ g
     verify_locations(c.syms["SINGLE_DEF"].nodes, "tests/Klocation:4")
 
     verify_locations(c.syms["MULTI_DEF"].nodes,
-      "tests/Klocation:6",
-      "tests/Klocation:29",
+      "tests/Klocation:7",
+      "tests/Klocation:31",
       "tests/Klocation_included:3",
-      "tests/Klocation:45")
+      "tests/Klocation:47")
 
     verify_locations(c.named_choices["CHOICE"].nodes,
                      "tests/Klocation_included:5")
 
     verify_locations([c.syms["MENU_HOOK"].nodes[0].next],
-                     "tests/Klocation_included:10")
+                     "tests/Klocation_included:12")
 
     verify_locations([c.syms["COMMENT_HOOK"].nodes[0].next],
-                     "tests/Klocation_included:15")
+                     "tests/Klocation_included:18")
 
     # Test recursive 'source' detection
 
@@ -1520,7 +1520,11 @@ g
 
     print("Testing choice semantics")
 
-    c = Kconfig("Kconfiglib/tests/Kchoice")
+    # Would warn for choice value symbols defined without a type, even
+    # though the type is automatically derived. This is probably more
+    # helpful than ignoring those cases, as this feature isn't used
+    # deliberately anywhere from what I've seen.
+    c = Kconfig("Kconfiglib/tests/Kchoice", warn=False)
 
     for name in "BOOL", "BOOL_OPT", "BOOL_M", "DEFAULTS":
         verify(c.named_choices[name].orig_type == BOOL,