Received-SPF: pass (google.com: domain of linux-kernel+bounces-160998-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Masahiro Yamada <masahiroy@kernel.org>
To: linux-kbuild@vger.kernel.org
Cc: linux-kernel@vger.kernel.org,
	Matt Porter <mporter@kernel.crashing.org>,
	Alexandre Bounine <alex.bou9@gmail.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Nathan Chancellor <nathan@kernel.org>,
	Nicolas Schier <nicolas@fjasle.eu>,
	linux-doc@vger.kernel.org
Subject: [PATCH 2/2] kconfig: do not imply the type of choice from the first entry
Date: Sat, 27 Apr 2024 19:42:31 +0900
Message-Id: <20240427104231.2728905-3-masahiroy@kernel.org>
In-Reply-To: <20240427104231.2728905-1-masahiroy@kernel.org>
References: <20240427104231.2728905-1-masahiroy@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

The followng two test cases are very similar, but the latter does not
work.

[test case 1]

    choice
            prompt "choose"

    config A
            bool "A"

    if y
    config B
            bool "B"
    endif

    endchoice

[test case 2]

    choice
            prompt "choose"

    if y
    config A
            bool "A"

    config B
            bool "B"
    endif

    endchoice

Since 'if y' is always true, both of them should be equivalent to:

    choice
            prompt "choose"

    config A
            bool "A"

    config B
            bool "B"

    endchoice

However, the test case 2 warns:
  Kconfig:1:warning: config symbol defined without type

If the type of choice is not specified, it is implied from the first
entry within the choice block.

When inferring the choice type, menu_finalize() checks only direct
children of the choice. At this point, the menu entries still exist
under the 'if' entry:

  choice
  \-- if y
      |-- A
      \-- B

Later, menu_finalize() re-parents the menu, so A and B will be lifted up
right under the choice:

  choice
  |-- if y
  |-- A
  \-- B

This is complex because menu_finalize() sets attributes, restructures
the menu tree, and checks the sanity at the same time, leading to some
bugs.

It would be possible to resolve it by setting the choice type after
re-parenting, but the current mechanism looks questionable to me.

Let's default all choices to 'bool' unless the type is specified.
This change makes sense because 99% of choice use cases are bool.

There exists only one 'tristate' choice in drivers/rapidio/Kconfig.
Another (much cleaner) approach would be to remove the tristate choice
support entirely, but I have not yet made up my mind.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 Documentation/kbuild/kconfig-language.rst             |  4 +---
 scripts/kconfig/menu.c                                | 11 -----------
 scripts/kconfig/parser.y                              |  3 +++
 scripts/kconfig/tests/choice/Kconfig                  |  2 +-
 scripts/kconfig/tests/choice_value_with_m_dep/Kconfig |  2 +-
 5 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/Documentation/kbuild/kconfig-language.rst b/Documentation/kbuild/kconfig-language.rst
index 555c2f839969..42b975b8e0cf 100644
--- a/Documentation/kbuild/kconfig-language.rst
+++ b/Documentation/kbuild/kconfig-language.rst
@@ -400,9 +400,7 @@ choices::
 
 This defines a choice group and accepts any of the above attributes as
 options. A choice can only be of type bool or tristate.  If no type is
-specified for a choice, its type will be determined by the type of
-the first choice element in the group or remain unknown if none of the
-choice elements have a type specified, as well.
+specified for a choice, its type will be the default 'bool'.
 
 While a boolean choice only allows a single config entry to be
 selected, a tristate choice also allows any number of config entries
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index e01b9ee87c05..134ef120ad08 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -323,17 +323,6 @@ static void _menu_finalize(struct menu *parent, bool inside_choice)
 			is_choice = true;
 
 		if (is_choice) {
-			if (sym->type == S_UNKNOWN) {
-				/* find the first choice value to find out choice type */
-				current_entry = parent;
-				for (menu = parent->list; menu; menu = menu->next) {
-					if (menu->sym && menu->sym->type != S_UNKNOWN) {
-						menu_set_type(menu->sym->type);
-						break;
-					}
-				}
-			}
-
 			/*
 			 * Use the choice itself as the parent dependency of
 			 * the contained items. This turns the mode of the
diff --git a/scripts/kconfig/parser.y b/scripts/kconfig/parser.y
index 613fa8c9c2d0..70ea3152d9b8 100644
--- a/scripts/kconfig/parser.y
+++ b/scripts/kconfig/parser.y
@@ -230,6 +230,9 @@ choice: T_CHOICE T_EOL
 
 choice_entry: choice choice_option_list
 {
+	if (current_entry->sym->type == S_UNKNOWN)
+		menu_set_type(S_BOOLEAN);
+
 	if (!current_entry->prompt) {
 		fprintf(stderr, "%s:%d: error: choice must have a prompt\n",
 			current_entry->filename, current_entry->lineno);
diff --git a/scripts/kconfig/tests/choice/Kconfig b/scripts/kconfig/tests/choice/Kconfig
index 8cdda40868a1..4dc0d3a1e089 100644
--- a/scripts/kconfig/tests/choice/Kconfig
+++ b/scripts/kconfig/tests/choice/Kconfig
@@ -18,7 +18,7 @@ config BOOL_CHOICE1
 endchoice
 
 choice
-	prompt "tristate choice"
+	tristate "tristate choice"
 	default TRI_CHOICE1
 
 config TRI_CHOICE0
diff --git a/scripts/kconfig/tests/choice_value_with_m_dep/Kconfig b/scripts/kconfig/tests/choice_value_with_m_dep/Kconfig
index bd970cec07d6..3e600c83279a 100644
--- a/scripts/kconfig/tests/choice_value_with_m_dep/Kconfig
+++ b/scripts/kconfig/tests/choice_value_with_m_dep/Kconfig
@@ -9,7 +9,7 @@ config DEP
 	default m
 
 choice
-	prompt "Tristate Choice"
+	tristate "Tristate Choice"
 
 config CHOICE0
 	tristate "Choice 0"
-- 
2.40.1