Fix argument count crash, null-returning builtins, and error message quality by Copilot · Pull Request #3 · kwakker35/SharpBasic

Copilot · 2026-04-05T14:10:25Z

Deep review of the SharpBasic evaluator identified several correctness and consistency issues: crashes on wrong argument counts, silent null propagation from three builtins, a misleading error message, two typos, and an off-by-one in array bounds messages.

Bug fixes

Argument count validation — Calling a SUB or FUNCTION with the wrong number of arguments previously caused an unhandled IndexOutOfRangeException. Now returns a descriptive EvalFailure:
```
Sub 'Add' expects 2 argument(s) but received 1.
```
VAL, STRING$, ASC returned null on bad input — These builtins silently returned null, which propagated as EvalSuccess(null) and produced a confusing downstream error ("Value assigned to 'x' was null"). Each now throws a specific InvalidOperationException:
- VAL("abc") → "VAL: 'abc' is not a valid number"
- STRING$("ab", 3) → "STRING$ first argument must be a single character"
- ASC(65) → "ASC requires a string argument"
Wrong error message in EvaluateBinaryExpression — "Unknown statement type: Token" → "Unknown operator: {value}"
Typo — "Invalid index suppled." → "Invalid index supplied." (two occurrences)
Off-by-one in array bounds error messages — 0-{Items.Length} → 0-{Items.Length - 1} (messages now show the valid inclusive upper bound)

Tests

Added 13 new tests covering argument count mismatch, and explicit error cases for VAL, STRING$, ASC, and CHR$
Updated 7 existing tests that asserted empty/null output from builtins to correctly expect EvalFailure

…error messages, and typos Agent-Logs-Url: https://github.com/kwakker35/SharpBasic/sessions/4b1d20b2-d914-44f6-a30f-3a5f1570b36b Co-authored-by: kwakker35 <5082729+kwakker35@users.noreply.github.com>

Copilot

Pull request overview

This PR improves SharpBasic evaluator correctness and diagnostic consistency by preventing crashes on SUB/FUNCTION calls with wrong argument counts and by converting several builtin “null returns” into explicit runtime errors, along with accompanying test updates.

Changes:

Add argument-count validation for CALLing SUBs and invoking FUNCTIONs to avoid IndexOutOfRangeException and return EvalFailure diagnostics.
Update builtins (VAL, STRING$, ASC) to throw descriptive runtime errors instead of returning null.
Improve diagnostic messages (unknown operator, array bounds upper bound, “suppled” typo), and expand/update tests to assert EvalFailure for these error paths.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
`src/SharpBasic.Evaluation/Evaluator.cs`	Adds arg-count checks for SUB/FUNCTION calls; updates builtin error behavior; improves several diagnostics.
`tests/SharpBasic.Evaluation.Tests/StdlibTests.cs`	Updates prior “empty output” expectations to `EvalFailure` and adds new builtin error-case tests.
`tests/SharpBasic.Evaluation.Tests/EvaluatorErrorTests.cs`	Adds tests covering SUB/FUNCTION call argument-count mismatches returning `EvalFailure`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-05T14:44:24Z

-                            : null
-                            : null,
+        ["VAL"] = args =>
+        {


VAL indexes args[0] without first validating args.Count. A call like VAL() will throw IndexOutOfRangeException, which gets wrapped into an EvalFailure with a generic .NET message rather than a user-friendly runtime error. Consider explicitly checking the argument count and throwing an InvalidOperationException with a clear message (or centralizing builtin arity validation in EvaluateCallExpression).

Suggested change

{

{

if (args.Count < 1)

throw new InvalidOperationException("VAL requires a string argument");

Copilot · 2026-04-05T14:44:24Z

+        ["STRING$"] = args =>
+        {
+            if (args[0] is not StringValue sv)
+                throw new InvalidOperationException("STRING$ requires a string as its first argument");
+            if (args[1] is not IntValue iv2)
+                throw new InvalidOperationException("STRING$ requires an integer as its second argument");


STRING$ uses args[0]/args[1] but doesn't guard against too few arguments. STRING$("x") / STRING$() will currently surface as an IndexOutOfRangeException message. Add an argument-count check up front (or enforce builtin arity in the call site) so the evaluator reports a consistent, descriptive runtime error.

Copilot · 2026-04-05T14:44:24Z

+            return new StringValue(new string(sv.V[0], iv2.V));
+        },
        ["ASC"] = args =>
        {


ASC indexes args[0] without checking args.Count. Calling ASC() will result in an IndexOutOfRangeException wrapped into EvalFailure with an unhelpful message. Add an explicit argument-count validation to keep builtin error reporting consistent.

Suggested change

{

{

if (args.Count != 1)

throw new InvalidOperationException("ASC requires exactly one argument");

Copilot · 2026-04-05T14:44:25Z

                            stmt.Location?.Line ?? 0,
                            stmt.Location?.Col ?? 0,
-                            $"Supplied index {idx} is outside of the range 0-{arrVal.Items.Length} defined for the array: {stmt.Name}.",
+                            $"Supplied index {idx} is outside of the range 0-{arrVal.Items.Length - 1} defined for the array: {stmt.Name}.",


The updated bounds message uses arrVal.Items.Length - 1 as the inclusive upper bound. If a zero-length array can be created (e.g. DIM a(0) given current EvaluateDimStatement behavior), this will format as 0--1, which is confusing. Consider special-casing Length == 0 (e.g., report that the array is empty / has no valid indices) or revisiting array sizing semantics.

Suggested change

$"Supplied index {idx} is outside of the range 0-{arrVal.Items.Length - 1} defined for the array: {stmt.Name}.",

arrVal.Items.Length == 0

? $"Supplied index {idx} is invalid because the array {stmt.Name} is empty and has no valid indices."

: $"Supplied index {idx} is outside of the range 0-{arrVal.Items.Length - 1} defined for the array: {stmt.Name}.",

Copilot · 2026-04-05T14:44:25Z

            if (idx < 0 || idx >= arrVal.Items.Length)
            {
                return new EvalFailure(
                    [
                        new Diagnostic(
                            expr.Location?.Line ?? 0,
                            expr.Location?.Col ?? 0,
-                            $"Supplied index {idx} is outside of the range 0-{arrVal.Items.Length} defined for the array: {expr.Name}.",
+                            $"Supplied index {idx} is outside of the range 0-{arrVal.Items.Length - 1} defined for the array: {expr.Name}.",
                            DiagnosticSeverity.Error


Same as the assign path: for Length == 0, the new 0-{Length - 1} range will display as 0--1. Consider a Length == 0 special-case for clearer diagnostics (or prevent creating empty arrays).

Copilot · 2026-04-05T14:44:25Z

-    Assert.Equal("", output);
+    // LEN requires a string; passing an integer raises a runtime error.
+    var result = RunHelper.RunResult("PRINT LEN(42)");
+    Assert.IsType<EvalFailure>(result);


This test only asserts EvalFailure, so it could pass even if the failure comes from an unintended code path. To prevent regressions in builtin runtime errors, consider asserting the diagnostic message includes LEN (or the expected "requires a string" wording).

Suggested change

Assert.IsType<EvalFailure>(result);

var failure = Assert.IsType<EvalFailure>(result);

Assert.Contains(failure.Diagnostics, d => d.Message.Contains("LEN") || d.Message.Contains("requires a string"));

Copilot · 2026-04-05T14:44:25Z

+  public void CHR_Dollar_String_Argument_Returns_EvalFailure()
  {
-    // Non-integer argument → null → prints ""
-    var output = RunHelper.Run("PRINT CHR$(\"H\")");
-    Assert.Equal("", output);
+    // Non-integer argument → runtime error
+    var result = RunHelper.RunResult("PRINT CHR$(\"H\")");
+    Assert.IsType<EvalFailure>(result);
  }

  [Fact]
-  public void CHR_Dollar_Float_Argument_Returns_Empty_Output()
+  public void CHR_Dollar_Float_Argument_Returns_EvalFailure()
  {
-    // Float does not match IntValue → null → prints ""
-    var output = RunHelper.Run("PRINT CHR$(72.5)");
-    Assert.Equal("", output);
+    // Float does not match IntValue → runtime error
+    var result = RunHelper.RunResult("PRINT CHR$(72.5)");
+    Assert.IsType<EvalFailure>(result);
  }

  [Fact]
-  public void CHR_Dollar_Boolean_Argument_Returns_Empty_Output()
+  public void CHR_Dollar_Boolean_Argument_Returns_EvalFailure()
  {
-    var output = RunHelper.Run("PRINT CHR$(TRUE)");
-    Assert.Equal("", output);
+    var result = RunHelper.RunResult("PRINT CHR$(TRUE)");
+    Assert.IsType<EvalFailure>(result);


These negative CHR$ tests only assert EvalFailure, so they could pass for unrelated failures. Consider asserting the diagnostic message contains CHR$ / "requires an integer argument" to verify the intended runtime error behavior.

Initial plan

4d8d353

Copilot AI assigned Copilot and kwakker35 Apr 5, 2026

Copilot started work on behalf of kwakker35 April 5, 2026 14:10 View session

Fix argument count validation, VAL/STRING$/ASC error handling, wrong …

3151956

…error messages, and typos Agent-Logs-Url: https://github.com/kwakker35/SharpBasic/sessions/4b1d20b2-d914-44f6-a30f-3a5f1570b36b Co-authored-by: kwakker35 <5082729+kwakker35@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Conduct full review of codebase for consistency and correctness~~ Fix argument count crash, null-returning builtins, and error message quality Apr 5, 2026

Copilot AI requested a review from kwakker35 April 5, 2026 14:21

Copilot finished work on behalf of kwakker35 April 5, 2026 14:21

kwakker35 approved these changes Apr 5, 2026

View reviewed changes

kwakker35 requested a review from Copilot April 5, 2026 14:40

Copilot started reviewing on behalf of kwakker35 April 5, 2026 14:40 View session

kwakker35 marked this pull request as ready for review April 5, 2026 14:40

kwakker35 merged commit c4f784b into main Apr 5, 2026
3 checks passed

Copilot AI reviewed Apr 5, 2026

View reviewed changes

Chris-Grove-BDO deleted the copilot/full-codebase-review branch April 14, 2026 15:46

Navigation Menu

Repository navigation

Fix argument count crash, null-returning builtins, and error message quality - #3

Fix argument count crash, null-returning builtins, and error message quality#3
kwakker35 merged 2 commits into
mainfrom
copilot/full-codebase-review

Copilot AI commented Apr 5, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-        {
+        {
+            if (args.Count < 1)
+                throw new InvalidOperationException("VAL requires a string argument");

-        {
+        {
+            if (args.Count != 1)
+                throw new InvalidOperationException("ASC requires exactly one argument");

-                            $"Supplied index {idx} is outside of the range 0-{arrVal.Items.Length - 1} defined for the array: {stmt.Name}.",
+                            arrVal.Items.Length == 0
+                                ? $"Supplied index {idx} is invalid because the array {stmt.Name} is empty and has no valid indices."
+                                : $"Supplied index {idx} is outside of the range 0-{arrVal.Items.Length - 1} defined for the array: {stmt.Name}.",

	Assert.IsType<EvalFailure>(result);
	var failure = Assert.IsType<EvalFailure>(result);
	Assert.Contains(failure.Diagnostics, d => d.Message.Contains("LEN") \|\| d.Message.Contains("requires a string"));

Conversation

Copilot AI commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug fixes

Tests

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Apr 5, 2026 •

edited

Loading