Make project configuration variables non-nullable by default

A project configuration variable with the NULL default value is naturally
assumed nullable, for example:

config [string] config.libhello.fallback_name ?= [null]

Otherwise, to make a project configuration nullable we use the `null`
variable attribute, for example:

config [string, null] config.libhello.fallback_name ?= "World"

3 files changed, 99 insertions, 18 deletions

diff --git a/doc/manual.cli b/doc/manual.cli
index 0ac053e..fcee403 100644
--- a/doc/manual.cli
+++ b/doc/manual.cli
@@ -4782,13 +4782,31 @@ is user-defined, then the default value is not evaluated.
 Note also that if the configuration value is not specified by the user and you
 haven't provided the default, the variable will be undefined, not \c{null},
 and, as a result, omitted from the persistent configuration
-(\c{build/config.build} file). However, \c{null} is a valid default value. It
-is traditionally used for \i{optional} configuration values. For example:
+(\c{build/config.build} file). In fact, unlike other variables, project
+configuration variables are by default not \i{nullable}. For example:
+
+\
+$ b configure config.libhello.fancy=[null]
+error: null value in non-nullable variable config.libhello.fancy
+\
+
+There are two ways to make \c{null} a valid value of a project configuration
+variable. Firstly, if the default value is \c{null}, then naturally the
+variable is assumed nullable. This is traditionally used for \i{optional}
+configuration values. For example:
 
 \
 config [string] config.libhello.fallback_name ?= [null]
 \
 
+If we need a nullable configuration variable but with a non-\c{null} default
+value (or no default value at all), then we have to use the \c{null} variable
+attribute. For example:
+
+\
+config [string, null] config.libhello.fallback_name ?= \"World\"
+\
+
 A common approach for representing an C/C++ enum-like value is to use
 \c{string} as a type and pattern matching for validation. In fact, validation
 and propagation can often be combined. For example, if our library needed to
@@ -4832,15 +4850,6 @@ if! $defined(config.libhello.database)
   fail 'config.libhello.database must be specified'
 \
 
-And if you want to also disallow \c{null} values, then the above check should
-be rewritten like this: \N{An undefined variable expands into a \c{null}
-value.}
-
-\
-if ($config.libhello.database == [null])
-  fail 'config.libhello.database must be specified'
-\
-
 If computing the default value is expensive or requires elaborate logic, then
 the handling of a configuration variable can be broken down into two steps
 along these lines:
diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx
index b76bb18..1e8757a 100644
--- a/libbuild2/parser.cxx
+++ b/libbuild2/parser.cxx
@@ -2862,18 +2862,23 @@ namespace build2
     next_with_attributes (t, tt);
 
     // Get variable attributes, if any, and deal with the special config.*
-    // attributes. Since currently they can only appear in the config
-    // directive, we handle them in an ad hoc manner.
+    // attributes as well as null. Since currently they can only appear in the
+    // config directive, we handle them in an ad hoc manner.
     //
     attributes_push (t, tt);
     attributes& as (attributes_top ());
 
+    bool nullable (false);
     optional<string> report;
     string report_var;
 
     for (auto i (as.begin ()); i != as.end (); )
     {
-      if (i->name == "config.report")
+      if (i->name == "null")
+      {
+        nullable = true;
+      }
+      else if (i->name == "config.report")
       {
         try
         {
@@ -2927,7 +2932,7 @@ namespace build2
 
     if (report && *report != "false" && !config)
     {
-      if (!as.empty ())
+      if (!as.empty () || nullable)
         fail (as.loc) << "unexpected attributes for report-only variable";
 
       attributes_pop ();
@@ -3029,6 +3034,9 @@ namespace build2
             peeked ().value != "false")
           fail (loc) << var << " variable default value must be literal false";
 
+        if (nullable)
+          fail (loc) << var << " variable must not be nullable";
+
         sflags |= config::save_false_omitted;
       }
 
@@ -3047,14 +3055,48 @@ namespace build2
         // all.
         //
         if (l.defined ())
+        {
+          // Peek at the attributes to detect whether the value is NULL.
+          //
+          if (!dev && !nullable)
+          {
+            // Essentially a prefix of parse_variable_value().
+            //
+            mode (lexer_mode::value, '@');
+            next_with_attributes (t, tt);
+            attributes_push (t, tt, true);
+            for (const attribute& a: attributes_pop ())
+            {
+              if (a.name == "null")
+              {
+                nullable = true;
+                break;
+              }
+            }
+          }
+
           skip_line (t, tt);
+        }
         else
         {
           value lhs, rhs (parse_variable_value (t, tt, !dev /* mode */));
           apply_value_attributes (&var, lhs, move (rhs), type::assign);
+
+          if (!nullable)
+            nullable = lhs.null;
+
           l = config::lookup_config (new_val, *root_, var, move (lhs), sflags);
         }
       }
+
+      // If the variable is not nullable, verify the value is not NULL.
+      //
+      // Note that undefined is not the same as NULL (if it is undefined, we
+      // should either see the default value or if there is no default value,
+      // then the user is expected to handle the undefined case).
+      //
+      if (!nullable && l.defined () && l->null)
+        fail (loc) << "null value in non-nullable variable " << var;
     }
 
     // We will be printing the report at either level 2 (-v) or 3 (-V)
diff --git a/tests/directive/config.testscript b/tests/directive/config.testscript
index 4049fa0..fba858f 100644
--- a/tests/directive/config.testscript
+++ b/tests/directive/config.testscript
@@ -14,7 +14,7 @@ test.arguments =
     EOI
 
   +cat <<EOI >=build/root.build
-    config [bool] config.test.fancy ?= false
+    config [bool, null] config.test.fancy ?= false
     print ($defined(config.test.fancy) ? $config.test.fancy : undefined)
     EOI
 
@@ -114,7 +114,6 @@ test.arguments =
     EOE
 }
 
-
 : default-none
 :
 {
@@ -125,7 +124,7 @@ test.arguments =
     EOI
 
   +cat <<EOI >=build/root.build
-    config [bool] config.test.fancy
+    config [bool, null] config.test.fancy
     print ($defined(config.test.fancy) ? $config.test.fancy : undefined)
     EOI
 
@@ -166,6 +165,37 @@ test.arguments =
     EOE
 }
 
+: non-nullable
+:
+{
+  .include ../common.testscript
+
+  +cat <<EOI >+build/bootstrap.build
+    using config
+    EOI
+
+  +cat <<EOI >=build/root.build
+    config [bool] config.test.fancy ?= false
+    print ($defined(config.test.fancy) ? $config.test.fancy : undefined)
+    EOI
+
+  # This must be a single, serial test since we are sharing config.build.
+  #
+  : test
+  :
+  cat <<EOI >=buildfile;
+    ./:
+    EOI
+
+  $* noop                               >'false' ;
+  $* noop      config.test.fancy=false  >'false' ;
+  $* noop      config.test.fancy=true   >'true'  ;
+
+  $* noop      config.test.fancy=[null] 2>>~/EOE/ != 0
+    /.+root.build:1:1: error: null value in non-nullable variable config\.test\.fancy/
+    EOE
+}
+
 : report
 :
 {