Throw ArgumentNullExceptions for null arguments (#570)

### Motivation and Context

Use ArgumentNullException as devs expect, and avoid exception-related
work when not throwing.

### Description

.NET developers expect that passing null erroneously to a method will
result in an ArgumentNullException, but currently it's resulting in a
custom ValidationException.

There are also places where string interpolation is being used to create
the error message; the work and allocation associated with that
interpolation is going to happen regardless of whether the exception is
thrown or not.
This commit is contained in:
Stephen Toub
2023-04-27 15:12:23 -07:00
committed by GitHub
parent de0079740a
commit 7e30a4e06f
26 changed files with 102 additions and 82 deletions
@@ -40,7 +40,7 @@ public abstract class ClientBase
CompleteRequestSettings requestSettings,
CancellationToken cancellationToken = default)
{
Verify.NotNull(requestSettings, "Completion settings cannot be empty");
Verify.NotNull(requestSettings);
if (requestSettings.MaxTokens < 1)
{
@@ -126,8 +126,8 @@ public abstract class ClientBase
ChatRequestSettings requestSettings,
CancellationToken cancellationToken = default)
{
Verify.NotNull(chat, "The chat history cannot be null");
Verify.NotNull(requestSettings, "Completion settings cannot be empty");
Verify.NotNull(chat);
Verify.NotNull(requestSettings);
if (requestSettings.MaxTokens < 1)
{
@@ -47,7 +47,7 @@ public sealed class ActionPlanner
IKernel kernel,
string? prompt = null)
{
Verify.NotNull(kernel, "The planner requires a non-null kernel instance");
Verify.NotNull(kernel);
string promptTemplate = prompt ?? EmbeddedResource.Read("skprompt.txt");
@@ -140,7 +140,7 @@ public sealed class ActionPlanner
[SKFunctionInput(Description = "The current goal processed by the planner", DefaultValue = "")]
public string ListOfFunctions(string goal, SKContext context)
{
Verify.NotNull(context.Skills, "The planner requires a non-null skill collection");
Verify.NotNull(context.Skills);
var functionsAvailable = context.Skills.GetFunctionsView();
// Prepare list using the format used by skprompt.txt
@@ -89,9 +89,6 @@ internal static class SequentialPlanParser
if (!string.IsNullOrEmpty(functionName) && context.IsFunctionRegistered(skillName, functionName, out var skillFunction))
{
Verify.NotNull(functionName, nameof(functionName));
Verify.NotNull(skillFunction, nameof(skillFunction));
var planStep = new Plan(skillFunction);
var functionVariables = new ContextVariables();
@@ -29,7 +29,7 @@ public sealed class SequentialPlanner
SequentialPlannerConfig? config = null,
string? prompt = null)
{
Verify.NotNull(kernel, $"{this.GetType().FullName} requires a kernel instance.");
Verify.NotNull(kernel);
this.Config = config ?? new();
this.Config.ExcludedSkills.Add(RestrictedSkillName);
@@ -42,7 +42,7 @@ public readonly struct Embedding<TEmbedding> : IEquatable<Embedding<TEmbedding>>
[JsonConstructor]
public Embedding(IEnumerable<TEmbedding> vector)
{
Verify.NotNull(vector, nameof(vector));
Verify.NotNull(vector);
if (!IsSupported)
{
@@ -43,7 +43,7 @@ public static class EmbeddingGenerationExtensions
(this IEmbeddingGeneration<TValue, TEmbedding> generator, TValue value, CancellationToken cancellationToken = default)
where TEmbedding : unmanaged
{
Verify.NotNull(generator, "Embeddings generator cannot be NULL");
Verify.NotNull(generator);
return (await generator.GenerateEmbeddingsAsync(new[] { value }, cancellationToken).ConfigureAwait(false)).FirstOrDefault();
}
}
@@ -0,0 +1,23 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
#pragma warning disable IDE0130 // Namespace does not match folder structure
// ReSharper disable once CheckNamespace
namespace System.Runtime.CompilerServices;
#pragma warning restore IDE0130
#if !NETCOREAPP
[AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
internal sealed class CallerArgumentExpressionAttribute : Attribute
{
public CallerArgumentExpressionAttribute(string parameterName)
{
this.ParameterName = parameterName;
}
public string ParameterName { get; }
}
#endif
@@ -15,18 +15,18 @@ internal static class Verify
private static readonly Regex s_asciiLettersDigitsUnderscoresRegex = new("^[0-9A-Za-z_]*$");
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void NotNull([NotNull] object? obj, string message)
internal static void NotNull([NotNull] object? obj, [CallerArgumentExpression(nameof(obj))] string? paramName = null)
{
if (obj is null)
{
ThrowValidationException(ValidationException.ErrorCodes.NullValue, message);
ThrowArgumentNullException(paramName);
}
}
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void NotEmpty([NotNull] string? str, string message)
internal static void NotEmpty([NotNull] string? str, string? message = null, [CallerArgumentExpression(nameof(str))] string? paramName = null)
{
NotNull(str, message);
NotNull(str, paramName);
if (string.IsNullOrWhiteSpace(str))
{
ThrowValidationException(ValidationException.ErrorCodes.EmptyValue, message);
@@ -62,8 +62,8 @@ internal static class Verify
internal static void StartsWith(string text, string prefix, string message)
{
NotEmpty(text, "The text to verify cannot be empty");
NotNull(prefix, "The prefix to verify is empty");
NotEmpty(text);
NotNull(prefix);
if (!text.StartsWith(prefix, StringComparison.OrdinalIgnoreCase))
{
ThrowValidationException(ValidationException.ErrorCodes.MissingPrefix, message);
@@ -104,26 +104,6 @@ internal static class Verify
}
}
internal static void GreaterThan<T>(T value, T min, string message) where T : IComparable<T>
{
int cmp = value.CompareTo(min);
if (cmp <= 0)
{
throw new ValidationException(ValidationException.ErrorCodes.OutOfRange, message);
}
}
public static void LessThan<T>(T value, T max, string message) where T : IComparable<T>
{
int cmp = value.CompareTo(max);
if (cmp >= 0)
{
throw new ValidationException(ValidationException.ErrorCodes.OutOfRange, message);
}
}
[DoesNotReturn]
private static void ThrowInvalidName(string kind, string name) =>
throw new KernelException(
@@ -131,6 +111,14 @@ internal static class Verify
$"A {kind} can contain only ASCII letters, digits, and underscores: '{name}' is not a valid name.");
[DoesNotReturn]
private static void ThrowValidationException(ValidationException.ErrorCodes errorCodes, string message) =>
internal static void ThrowArgumentNullException(string? paramName) =>
throw new ArgumentNullException(paramName);
[DoesNotReturn]
internal static void ThrowArgumentOutOfRangeException<T>(string? paramName, T actualValue, string message) =>
throw new ArgumentOutOfRangeException(paramName, actualValue, message);
[DoesNotReturn]
internal static void ThrowValidationException(ValidationException.ErrorCodes errorCodes, string? message) =>
throw new ValidationException(errorCodes, message);
}
@@ -94,7 +94,10 @@ public sealed class SKContext
/// <returns>Delegate to execute the function</returns>
public ISKFunction Func(string skillName, string functionName)
{
Verify.NotNull(this.Skills, "The skill collection hasn't been set");
if (this.Skills is null)
{
Verify.ThrowValidationException(ValidationException.ErrorCodes.NullValue, nameof(this.Skills));
}
if (this.Skills.HasNativeFunction(skillName, functionName))
{
@@ -179,7 +179,10 @@ public class PromptTemplateConfig
public static PromptTemplateConfig FromJson(string json)
{
var result = Json.Deserialize<PromptTemplateConfig>(json);
Verify.NotNull(result, "Unable to deserialize prompt template config. The deserialized returned NULL.");
if (result is null)
{
Verify.ThrowValidationException(ValidationException.ErrorCodes.NullValue, "Unable to deserialize prompt template config. The deserialized returned NULL.");
}
return result;
}
}
@@ -69,7 +69,7 @@ public class EmbeddingTests
public void ItThrowsWithNullVector()
{
// Assert
Assert.Throws<ValidationException>(() => new Embedding<float>(null!));
Assert.Throws<ArgumentNullException>("vector", () => new Embedding<float>(null!));
}
[Fact]
@@ -1,8 +1,8 @@
// Copyright (c) Microsoft. All rights reserved.
using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.SemanticKernel.Diagnostics;
using Microsoft.SemanticKernel.Memory.Collections;
using Xunit;
@@ -27,11 +27,10 @@ public class MinHeapTests
};
// Act
var exception = Assert.Throws<ValidationException>(() => action());
var exception = Assert.Throws<ArgumentOutOfRangeException>("capacity", () => action());
// Assert
Assert.Equal(ValidationException.ErrorCodes.OutOfRange, exception.ErrorCode);
Assert.Equal("Out of range: MinHeap capacity must be greater than 0.", exception.Message);
Assert.Equal(-1, exception.ActualValue);
}
[Fact]
@@ -91,11 +90,10 @@ public class MinHeapTests
var action = () => { minHeap.Add(items, startIndex); };
// Act
var exception = Assert.Throws<ValidationException>(() => action());
var exception = Assert.Throws<ArgumentOutOfRangeException>("startAt", () => action());
// Assert
Assert.Equal(ValidationException.ErrorCodes.OutOfRange, exception.ErrorCode);
Assert.Equal("Out of range: startAt value must be less than items count.", exception.Message);
Assert.Equal(startIndex, exception.ActualValue);
}
[Fact]
@@ -42,7 +42,7 @@ public static class EmbeddingIndexExtensions
double minScore = 0.0)
where TEmbedding : unmanaged
{
Verify.NotNull(index, "Embedding index cannot be NULL");
Verify.NotNull(index);
await foreach (var match in index.GetNearestMatchesAsync(collection, embedding, 1, minScore))
{
return match;
@@ -34,7 +34,7 @@ public sealed class HuggingFaceTextCompletion : ITextCompletion, IDisposable
/// <param name="httpClientHandler">Instance of <see cref="HttpClientHandler"/> to setup specific scenarios.</param>
public HuggingFaceTextCompletion(Uri endpoint, string model, HttpClientHandler httpClientHandler)
{
Verify.NotNull(endpoint, "Endpoint cannot be null.");
Verify.NotNull(endpoint);
Verify.NotEmpty(model, "Model cannot be empty.");
this._endpoint = endpoint;
@@ -53,7 +53,7 @@ public sealed class HuggingFaceTextCompletion : ITextCompletion, IDisposable
/// <param name="model">Model to use for service API call.</param>
public HuggingFaceTextCompletion(Uri endpoint, string model)
{
Verify.NotNull(endpoint, "Endpoint cannot be null.");
Verify.NotNull(endpoint);
Verify.NotEmpty(model, "Model cannot be empty.");
this._endpoint = endpoint;
@@ -33,7 +33,7 @@ public sealed class HuggingFaceTextEmbeddingGeneration : IEmbeddingGeneration<st
/// <param name="httpClientHandler">Instance of <see cref="HttpClientHandler"/> to setup specific scenarios.</param>
public HuggingFaceTextEmbeddingGeneration(Uri endpoint, string model, HttpClientHandler httpClientHandler)
{
Verify.NotNull(endpoint, "Endpoint cannot be null.");
Verify.NotNull(endpoint);
Verify.NotEmpty(model, "Model cannot be empty.");
this._endpoint = endpoint;
@@ -52,7 +52,7 @@ public sealed class HuggingFaceTextEmbeddingGeneration : IEmbeddingGeneration<st
/// <param name="model">Model to use for service API call.</param>
public HuggingFaceTextEmbeddingGeneration(Uri endpoint, string model)
{
Verify.NotNull(endpoint, "Endpoint cannot be null.");
Verify.NotNull(endpoint);
Verify.NotEmpty(model, "Model cannot be empty.");
this._endpoint = endpoint;
+1 -1
View File
@@ -123,7 +123,7 @@ public sealed class Kernel : IKernel, IDisposable
{
// Future-proofing the name not to contain special chars
Verify.ValidSkillName(skillName);
Verify.NotNull(customFunction, $"The {nameof(customFunction)} parameter is not set to an instance of an object.");
Verify.NotNull(customFunction);
customFunction.SetDefaultSkillCollection(this.Skills);
this._skillCollection.AddSemanticFunction(customFunction);
+8 -8
View File
@@ -69,7 +69,7 @@ public sealed class KernelBuilder
/// <returns>Updated kernel builder including the logger.</returns>
public KernelBuilder WithLogger(ILogger log)
{
Verify.NotNull(log, "The logger instance provided is NULL");
Verify.NotNull(log);
this._log = log;
return this;
}
@@ -81,7 +81,7 @@ public sealed class KernelBuilder
/// <returns>Updated kernel builder including the semantic text memory entity.</returns>
public KernelBuilder WithMemory(ISemanticTextMemory memory)
{
Verify.NotNull(memory, "The memory instance provided is NULL");
Verify.NotNull(memory);
this._memory = memory;
return this;
}
@@ -93,7 +93,7 @@ public sealed class KernelBuilder
/// <returns>Updated kernel builder including the memory storage.</returns>
public KernelBuilder WithMemoryStorage(IMemoryStore storage)
{
Verify.NotNull(storage, "The memory instance provided is NULL");
Verify.NotNull(storage);
this._memoryStorage = storage;
return this;
}
@@ -107,8 +107,8 @@ public sealed class KernelBuilder
public KernelBuilder WithMemoryStorageAndTextEmbeddingGeneration(
IMemoryStore storage, IEmbeddingGeneration<string, float> embeddingGenerator)
{
Verify.NotNull(storage, "The memory instance provided is NULL");
Verify.NotNull(embeddingGenerator, "The embedding generator instance provided is NULL");
Verify.NotNull(storage);
Verify.NotNull(embeddingGenerator);
this._memory = new SemanticTextMemory(storage, embeddingGenerator);
return this;
}
@@ -120,7 +120,7 @@ public sealed class KernelBuilder
/// <returns>Updated kernel builder including the retry handler factory.</returns>
public KernelBuilder WithRetryHandlerFactory(IDelegatingHandlerFactory httpHandlerFactory)
{
Verify.NotNull(httpHandlerFactory, "The retry handler factory instance provided is NULL");
Verify.NotNull(httpHandlerFactory);
this._httpHandlerFactory = httpHandlerFactory;
return this;
}
@@ -132,7 +132,7 @@ public sealed class KernelBuilder
/// <returns>Updated kernel builder including the given configuration.</returns>
public KernelBuilder WithConfiguration(KernelConfig config)
{
Verify.NotNull(config, "The configuration instance provided is NULL");
Verify.NotNull(config);
this._config = config;
return this;
}
@@ -144,7 +144,7 @@ public sealed class KernelBuilder
/// <returns>Updated kernel builder including the updated configuration.</returns>
public KernelBuilder Configure(Action<KernelConfig> configure)
{
Verify.NotNull(configure, "The configuration action provided is NULL");
Verify.NotNull(configure);
configure.Invoke(this._config);
return this;
}
@@ -28,7 +28,10 @@ internal sealed class MinHeap<T> : IEnumerable<T> where T : IComparable<T>
public MinHeap(T minValue, int capacity)
{
Verify.GreaterThan(capacity, MinCapacity, $"MinHeap capacity must be greater than {MinCapacity}.");
if (capacity < MinCapacity)
{
Verify.ThrowArgumentOutOfRangeException(nameof(capacity), capacity, $"MinHeap capacity must be greater than {MinCapacity}.");
}
this._items = new T[capacity + 1];
//
@@ -106,11 +109,13 @@ internal sealed class MinHeap<T> : IEnumerable<T> where T : IComparable<T>
public void Add(IList<T> items, int startAt = 0)
{
Verify.NotNull(items, nameof(items));
Verify.NotNull(items);
int newItemCount = items.Count;
Verify.LessThan(startAt, newItemCount, $"{nameof(startAt)} value must be less than {nameof(items)} count.");
if (startAt >= newItemCount)
{
Verify.ThrowArgumentOutOfRangeException(nameof(startAt), startAt, $"{nameof(startAt)} value must be less than {nameof(items)}.{nameof(items.Count)}.");
}
this.EnsureCapacity(this._count + (newItemCount - startAt));
for (int i = startAt; i < newItemCount; ++i)
@@ -148,7 +153,10 @@ internal sealed class MinHeap<T> : IEnumerable<T> where T : IComparable<T>
public void EnsureCapacity(int capacity)
{
Verify.GreaterThan(capacity, MinCapacity, $"MinHeap capacity must be greater than {MinCapacity}.");
if (capacity < MinCapacity)
{
Verify.ThrowArgumentOutOfRangeException(nameof(capacity), capacity, $"MinHeap capacity must be greater than {MinCapacity}.");
}
// 0th item is always a sentinel
capacity++;
@@ -51,8 +51,8 @@ public static class MemoryConfiguration
[SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The embeddingGenerator object is disposed by the kernel")]
public static void UseMemory(this IKernel kernel, IEmbeddingGeneration<string, float> embeddingGenerator, IMemoryStore storage)
{
Verify.NotNull(storage, "The storage instance provided is NULL");
Verify.NotNull(embeddingGenerator, "The embedding generator is NULL");
Verify.NotNull(storage);
Verify.NotNull(embeddingGenerator);
kernel.RegisterMemory(new SemanticTextMemory(storage, embeddingGenerator));
}
@@ -56,8 +56,8 @@ public class VolatileMemoryStore : IMemoryStore
/// <inheritdoc/>
public Task<string> UpsertAsync(string collectionName, MemoryRecord record, CancellationToken cancel = default)
{
Verify.NotNull(record, "Memory record cannot be NULL");
Verify.NotNull(record.Metadata.Id, "Memory metadata ID cannot be NULL");
Verify.NotNull(record);
Verify.NotNull(record.Metadata.Id);
if (this.TryGetCollection(collectionName, out var collectionDict, create: false))
{
@@ -1,5 +1,6 @@
// Copyright (c) Microsoft. All rights reserved.
using System.Diagnostics.CodeAnalysis;
using Microsoft.SemanticKernel.SkillDefinition;
#pragma warning disable IDE0130
@@ -16,7 +17,7 @@ internal static class SKContextExtensions
/// <param name="skillName">The skill name</param>
/// <param name="functionName">The function name</param>
/// <param name="registeredFunction">The registered function, if found</param>
internal static bool IsFunctionRegistered(this SKContext context, string skillName, string functionName, out ISKFunction? registeredFunction)
internal static bool IsFunctionRegistered(this SKContext context, string skillName, string functionName, [NotNullWhen(true)] out ISKFunction? registeredFunction)
{
context.ThrowIfSkillCollectionNotSet();
+1 -1
View File
@@ -2,6 +2,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text.Json;
using System.Text.Json.Serialization;
@@ -395,7 +396,6 @@ public sealed class Plan : ISKFunction
{
if (context.IsFunctionRegistered(plan.SkillName, plan.Name, out var skillFunction))
{
Verify.NotNull(skillFunction, nameof(skillFunction));
plan.SetFunction(skillFunction);
}
}
@@ -87,7 +87,6 @@ public static class ImportSemanticSkillFromDirectoryExtension
if (File.Exists(configPath))
{
config = PromptTemplateConfig.FromJson(File.ReadAllText(configPath));
Verify.NotNull(config, $"Invalid prompt template configuration, unable to parse {configPath}");
}
kernel.Log.LogTrace("Config {0}: {1}", functionName, config.ToJson());
@@ -97,14 +97,14 @@ public sealed class SKFunction : ISKFunction, IDisposable
SemanticFunctionConfig functionConfig,
ILogger? log = null)
{
Verify.NotNull(functionConfig, "Function configuration is empty");
Verify.NotNull(functionConfig);
async Task<SKContext> LocalFunc(
ITextCompletion client,
CompleteRequestSettings requestSettings,
SKContext context)
{
Verify.NotNull(client, "AI LLM backed is empty");
Verify.NotNull(client);
try
{
@@ -207,7 +207,7 @@ public sealed class SKFunction : ISKFunction, IDisposable
/// <inheritdoc/>
public ISKFunction SetAIService(Func<ITextCompletion> serviceFactory)
{
Verify.NotNull(serviceFactory, "AI LLM service factory is empty");
Verify.NotNull(serviceFactory);
this.VerifyIsSemantic();
this._aiService = serviceFactory.Invoke();
return this;
@@ -216,7 +216,7 @@ public sealed class SKFunction : ISKFunction, IDisposable
/// <inheritdoc/>
public ISKFunction SetAIConfiguration(CompleteRequestSettings settings)
{
Verify.NotNull(settings, "AI LLM request settings are empty");
Verify.NotNull(settings);
this.VerifyIsSemantic();
this._aiRequestSettings = settings;
return this;
@@ -292,7 +292,7 @@ public sealed class SKFunction : ISKFunction, IDisposable
ILogger? log = null
)
{
Verify.NotNull(delegateFunction, "The function delegate is empty");
Verify.NotNull(delegateFunction);
Verify.ValidSkillName(skillName);
Verify.ValidFunctionName(functionName);
Verify.ParametersUniqueness(parameters);
@@ -497,7 +497,7 @@ public sealed class SKFunction : ISKFunction, IDisposable
private static MethodDetails GetMethodDetails(MethodInfo methodSignature, object? methodContainerInstance, ILogger? log = null)
{
Verify.NotNull(methodSignature, "Method is NULL");
Verify.NotNull(methodSignature);
var result = new MethodDetails
{
@@ -51,7 +51,7 @@ public class SkillCollection : ISkillCollection
/// <inheritdoc/>
public ISkillCollection AddNativeFunction(ISKFunction functionInstance)
{
Verify.NotNull(functionInstance, "The function is NULL");
Verify.NotNull(functionInstance);
if (!this._skillCollection.ContainsKey(functionInstance.SkillName))
{
// Important: names are case insensitive
@@ -199,7 +199,7 @@ public static class KernelOpenApiExtensions
AuthenticateRequestAsyncCallback? authCallback = null,
CancellationToken cancellationToken = default)
{
Verify.NotNull(kernel, nameof(kernel));
Verify.NotNull(kernel);
Verify.ValidSkillName(skillName);
// Parse