Fix spurious error when having too few arguments to a call (#930)

Fixes #782

There is logic in the compiler to confirm that the argument expression for an `out` or `inout` parameter is an l-value. That logic was producing an internal compiler error if it ran out of arguments while processing the parameter list, on the assumption that this would mean an `out` parameter had a default argument expression (which isn't something we want to support).

The problem was that the checking for call expressions will diagnose a call with too few arguments, and then leave the call in the AST to support subequent checking. This meant that any call where the user didn't supply enough arguments *and* one of the trailing argument is `out` or `inout` would produce the error for the original problem (not enough arguments), but then *also* produce the internal error because there is seemingly no argument to match with the `out` or `inout` parameter.

The right fix is to not take responsibility for diagnosing this problem at the call site, and instead to rule out default value expressions for `out` and `inout` parameters at the declaration site instead.

2 files changed, 36 insertions, 7 deletions

diff --git a/source/slang/check.cpp b/source/slang/check.cpp
index 2258a263e..166fff771 100644
--- a/source/slang/check.cpp
+++ b/source/slang/check.cpp
@@ -4737,6 +4737,22 @@ namespace Slang
                 // For example, it should not be allowed to refer
                 // to other parameters of the same function (or maybe
                 // only the parameters to its left...).
+
+                // A default argument value should not be allowed on an
+                // `out` or `inout` parameter.
+                //
+                // TODO: we could relax this by requiring the expression
+                // to yield an lvalue, but that seems like a feature
+                // with limited practical utility (and an easy source
+                // of confusing behavior).
+                //
+                // Note: the `InOutModifier` class inherits from `OutModifier`,
+                // so we only need to check for the base case.
+                //
+                if(paramDecl->FindModifier<OutModifier>())
+                {
+                    getSink()->diagnose(initExpr, Diagnostics::outputParameterCannotHaveDefaultValue);
+                }
             }
 
             paramDecl->SetCheckState(DeclCheckState::Checked);
@@ -8738,15 +8754,25 @@ namespace Slang
                             }
                             else
                             {
-                                // This implies that the function had an `out`
-                                // or `inout` parameter and they gave it a default
-                                // argument expression. I'm not even sure what
-                                // that would mean.
+                                // There are two ways we could get here, both involving
+                                // a call where the number of argument expressions is
+                                // less than the number of parameters on the callee:
+                                //
+                                // 1. There might be fewer arguments than parameters
+                                // because the trailing parameters should be defaulted
+                                //
+                                // 2. There might be fewer arguments than parameters
+                                // because the call is incorrect.
+                                //
+                                // In case (2) an error would have already been diagnosed,
+                                // and we don't want to emit another cascading error here.
                                 //
-                                // TODO: make sure this gets validated on the
-                                // declaring side.
+                                // In case (1) this implies the user declared an `out`
+                                // or `inout` parameter with a default argument expression.
+                                // That should be an error, but it should be detected
+                                // on the declaration instead of here at the use site.
                                 //
-                                SLANG_DIAGNOSE_UNEXPECTED(getSink(), invoke, "default argument expression for out/inout paameter");
+                                // Thus, it makes sense to ignore this case here.
                             }
                         }
                     }
diff --git a/source/slang/diagnostic-defs.h b/source/slang/diagnostic-defs.h
index f254f1c87..dbbf0a563 100644
--- a/source/slang/diagnostic-defs.h
+++ b/source/slang/diagnostic-defs.h
@@ -297,6 +297,9 @@ DIAGNOSTIC(30504, Error, cannotUseInitializerListForType, "cannot use initialize
 // 306xx: variables
 DIAGNOSTIC(30600, Error, varWithoutTypeMustHaveInitializer, "a variable declaration without an initial-value expression must be given an explicit type");
 
+// 307xx: parameters
+DIAGNOSTIC(30700, Error, outputParameterCannotHaveDefaultValue, "an 'out' or 'inout' parameter cannot have a default-value expression");
+
 DIAGNOSTIC(39999, Error, expectedIntegerConstantWrongType, "expected integer constant (found: '$0')")
 DIAGNOSTIC(39999, Error, expectedIntegerConstantNotConstant, "expression does not evaluate to a compile-time constant")
 DIAGNOSTIC(39999, Error, expectedIntegerConstantNotLiteral, "could not extract value from integer constant")