[PATCH 0/2] kconfig: stop implification of choice type

[PATCH 0/2] kconfig: stop implification of choice type

[Masahiro Yamada]

There are two types supported for the choice statement,
"bool" and "tristate".

There is no ambiguity if you explicitly specifiy the type of the
choice.

For example

    choice
            bool "choose"

       ...

    endchoice

Or,

    choice
            tristate "choose"

       ...

    endchoice

Those are valid code, and clear about the behavior.

However, if you see the reality in the kernel code, most of people
omit the type definition.

Usually, the choice is written like this:

    choice
            prompt "choose"

       ...

    endchoice

The "prompt" does not specify the type at all.

You may wonder how Kconfig knows the choice type then.

When the choice type is not specified, Kconfig infers it from the first
entry within the choice block.

In the following, the choice type is bool because the first entry,
CONFIG_A, is bool.

    choice
            prompt "choose"

    config A
            bool "A"

    config B
            bool "B"

    endchoice

As described in 2/2, this has a bug when "if" ... "endif" exists
within a "choice" ... "endchoice".

Of course, I can fix this bug, but the value of this feature is
questionable.

This patch set stop the type implification. Instead, make the
default type of the choice "bool".

This is reasonable because 99% of choice blocks are bool.

The only user of tristate choice is drivers/rapidio/Kconfig.
(although that choice is unneeded because it cotains a single
entry, RAPIDIO_ENUM_BASIC)

It changed it to specify "tristate" explicitly.



Masahiro Yamada (2):
  rapidio: specify the type for tristate choice explicitly
  kconfig: do not imply the type of choice from the first entry

 Documentation/kbuild/kconfig-language.rst             |  4 +---
 drivers/rapidio/Kconfig                               |  2 +-
 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 +-
 6 files changed, 7 insertions(+), 17 deletions(-)

-- 
2.40.1

[PATCH 1/2] rapidio: specify the type for tristate choice explicitly

[Masahiro Yamada]

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

In this case, the first (and only) entry, RAPIDIO_ENUM_BASIC, is
tristate, hence this choice behaves as tristate.

Kconfig will stop this implication because it has a bug, and 99% of
choice use cases are bool. In fact, this is the only instance of
tristate choice in the kernel.

Before transitioning the default choice type to 'bool', specify the
type explicitly for this case.

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

 drivers/rapidio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rapidio/Kconfig b/drivers/rapidio/Kconfig
index b9f8514909bf..72b5b02492a1 100644
--- a/drivers/rapidio/Kconfig
+++ b/drivers/rapidio/Kconfig
@@ -60,7 +60,7 @@ config RAPIDIO_DEBUG
 	  If you are unsure about this, say N here.
 
 choice
-	prompt "Enumeration method"
+	tristate "Enumeration method"
 	depends on RAPIDIO
 	default RAPIDIO_ENUM_BASIC
 	help
-- 
2.40.1

Re: [PATCH 1/2] rapidio: specify the type for tristate choice explicitly

[Nicolas Schier]

On Sat, Apr 27, 2024 at 07:42:30PM +0900, Masahiro Yamada wrote:
> If the type of choice is not specified, it is implied by the first
> entry within the choice block.
> 
> In this case, the first (and only) entry, RAPIDIO_ENUM_BASIC, is
> tristate, hence this choice behaves as tristate.
> 
> Kconfig will stop this implication because it has a bug, and 99% of
> choice use cases are bool. In fact, this is the only instance of
> tristate choice in the kernel.
> 
> Before transitioning the default choice type to 'bool', specify the
> type explicitly for this case.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---

Reviewed-by: Nicolas Schier <n.schier@avm.de>

[PATCH 2/2] kconfig: do not imply the type of choice from the first entry

[Masahiro Yamada]

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

Re: [PATCH 2/2] kconfig: do not imply the type of choice from the first entry

[Nicolas Schier]

On Sat, Apr 27, 2024 at 07:42:31PM +0900, Masahiro Yamada wrote:
> 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>
> ---

Thanks, looks good to me.

Reviewed-by: Nicolas Schier <n.schier@avm.de>