fix: skip empty source values for flags that cannot parse them

docs/v3/examples/flags/value-sources.md

@@ -136,6 +136,11 @@ func main() {
 Note that default values are set in the same order as they are defined in the
 `Sources` param. This allows the user to choose order of priority
 
+An empty value from any source, such as an environment variable that is set but empty
+or an empty file, only counts as a value for string and bool flags, where it means an
+empty string and `false`. For any other flag it is skipped, so it does not satisfy
+`Required`, does not make `IsSet` true, and does not hide a later source.
+
 #### Values from alternate input sources (YAML, TOML, and others)
 
 There is a separate package [altsrc](https://github.com/urfave/cli-altsrc) that adds support for getting flag values

flag_impl.go

@@ -135,13 +135,24 @@ func (f *FlagBase[T, C, V]) PostParse() error {
 	tracef("postparse (flag=%[1]q)", f.Name)
 
 	if !f.hasBeenSet {
-		if val, source, found := f.Sources.LookupWithSource(); found {
-			// reflect.TypeOf yields nil when T is an interface type (e.g.
-			// GenericFlag) and the value is nil, so the kind has to be
-			// derived defensively.
-			kind := reflect.Invalid
-			if ty := reflect.TypeOf(f.Value); ty != nil {
-				kind = ty.Kind()
+		// reflect.TypeOf yields nil when T is an interface type (e.g.
+		// GenericFlag) and the value is nil, so the kind has to be
+		// derived defensively.
+		kind := reflect.Invalid
+		if ty := reflect.TypeOf(f.Value); ty != nil {
+			kind = ty.Kind()
+		}
+
+		// An empty value is a value only for a string, and reads as false
+		// for a bool. Any other kind has nothing to parse from it, so an
+		// empty source is skipped: it neither marks the flag as set nor
+		// hides a later source in the chain.
+		emptyIsValue := kind == reflect.String || kind == reflect.Bool
+
+		for _, source := range f.Sources.Chain {
+			val, found := source.Lookup()
+			if !found || (val == "" && !emptyIsValue) {
+				continue
 			}
 
 			if val != "" || kind == reflect.String {
@@ -151,12 +162,13 @@ func (f *FlagBase[T, C, V]) PostParse() error {
 						val, f.Value, source, f.Name, err,
 					)
 				}
-			} else if val == "" && kind == reflect.Bool {
+			} else {
 				_ = f.Set(f.Name, "false")
 			}
 
 			f.hasBeenSet = true
 			f.fromSource = true
+			break
 		}
 	}
 

flag_test.go

@@ -6,6 +6,7 @@ import (
 	"flag"
 	"fmt"
 	"os"
+	"path/filepath"
 	"reflect"
 	"regexp"
 	"runtime"
@@ -668,6 +669,118 @@ func TestStringFlagDefaultText(t *testing.T) {
 	assert.Equal(t, expected, fl.String())
 }
 
+// An empty value from a source can only be a value for a string, and reads as
+// false for a bool. For any other kind there is nothing to parse, so the source
+// must not count as setting the flag: that would satisfy Required with a zero
+// value, report IsSet for a flag that only holds its default, and hide a later
+// source in the chain that does carry a value.
+func TestFlagsFromEmptyEnv(t *testing.T) {
+	testCases := []struct {
+		name        string
+		env         map[string]string
+		fl          Flag
+		wantValue   any
+		wantIsSet   bool
+		errContains string
+	}{
+		{
+			name:        "required int from an empty env var is not set",
+			env:         map[string]string{"PORT": ""},
+			fl:          &IntFlag{Name: "port", Required: true, Sources: EnvVars("PORT")},
+			errContains: `Required flag "port" not set`,
+		},
+		{
+			name:        "required duration from an empty env var is not set",
+			env:         map[string]string{"TIMEOUT": ""},
+			fl:          &DurationFlag{Name: "timeout", Required: true, Sources: EnvVars("TIMEOUT")},
+			errContains: `Required flag "timeout" not set`,
+		},
+		{
+			name:      "int keeps its default and is not set",
+			env:       map[string]string{"PORT": ""},
+			fl:        &IntFlag{Name: "port", Value: 80, Sources: EnvVars("PORT")},
+			wantValue: 80,
+		},
+		{
+			name:      "string slice keeps its default and is not set",
+			env:       map[string]string{"TAGS": ""},
+			fl:        &StringSliceFlag{Name: "tags", Value: []string{"a"}, Sources: EnvVars("TAGS")},
+			wantValue: []string{"a"},
+		},
+		{
+			name:      "an empty env var does not hide a later one",
+			env:       map[string]string{"LEGACY_PORT": "", "PORT": "8080"},
+			fl:        &IntFlag{Name: "port", Value: 80, Sources: EnvVars("LEGACY_PORT", "PORT")},
+			wantValue: 8080,
+			wantIsSet: true,
+		},
+		{
+			name:        "an unparsable env var still fails before a later one",
+			env:         map[string]string{"LEGACY_PORT": "abc", "PORT": "8080"},
+			fl:          &IntFlag{Name: "port", Sources: EnvVars("LEGACY_PORT", "PORT")},
+			errContains: `could not parse "abc" as int value from environment variable "LEGACY_PORT"`,
+		},
+		{
+			name:      "an empty string is still a value",
+			env:       map[string]string{"NAME": ""},
+			fl:        &StringFlag{Name: "name", Value: "def", Sources: EnvVars("NAME")},
+			wantValue: "",
+			wantIsSet: true,
+		},
+		{
+			name:      "an empty bool still reads as false",
+			env:       map[string]string{"DEBUG": ""},
+			fl:        &BoolFlag{Name: "debug", Value: true, Sources: EnvVars("DEBUG")},
+			wantValue: false,
+			wantIsSet: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			for k, v := range tc.env {
+				t.Setenv(k, v)
+			}
+
+			name := tc.fl.Names()[0]
+			var gotValue any
+			var gotIsSet bool
+			cmd := &Command{
+				Flags: []Flag{tc.fl},
+				Action: func(_ context.Context, cmd *Command) error {
+					gotValue = cmd.Value(name)
+					gotIsSet = cmd.IsSet(name)
+					return nil
+				},
+			}
+
+			err := cmd.Run(buildTestContext(t), []string{"run"})
+			if tc.errContains != "" {
+				require.ErrorContains(t, err, tc.errContains)
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tc.wantValue, gotValue)
+			assert.Equal(t, tc.wantIsSet, gotIsSet)
+		})
+	}
+}
+
+// The skip applies to every source in the chain, not only to environment
+// variables, so an empty file is treated the same way as an empty variable.
+func TestFlagFromEmptyFileSource(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "port")
+	require.NoError(t, os.WriteFile(path, nil, 0o600))
+
+	cmd := &Command{
+		Flags: []Flag{&IntFlag{Name: "port", Required: true, Sources: Files(path)}},
+	}
+
+	err := cmd.Run(buildTestContext(t), []string{"run"})
+	require.ErrorContains(t, err, `Required flag "port" not set`)
+}
+
 func TestStringFlagWithEnvVarHelpOutput(t *testing.T) {
 	t.Setenv("APP_FOO", "derp")