Report diagnostics on malformed service interfaces instead of crashing

[pepone]

Summary

• The three ServiceMethod constructors (Protobuf/Ice/Slice) used Debug.Assert and the null-forgiving operator for shape validation. In release builds Debug.Assert is a no-op and ! adds no runtime check, so a hand-authored [RpcMethod]/[IceOperation]/[SliceOperation] on a wrong-shape method, or a version mismatch between this generator and the IDL-side generator, propagated as an opaque ""source generator threw"" MSBuild error.
• Convert each constructor into a static TryCreate factory that validates up front, reports a targeted diagnostic, and returns null on shape mismatch. Plumb an Action<Diagnostic> reporter through ServiceMethodFactory.TryCreate so the parser simply skips offending methods.
• Three new descriptors: IRSG0002 (unexpected operation attribute shape), IRSG0003 (unexpected method signature), IRSG0004 (missing generator-produced Request class).

Fixes #4460

[Copilot]

Pull request overview

This PR improves the IceRPC service source generator’s resilience to malformed or version-skewed service interfaces by replacing Debug.Assert/null-forgiving assumptions with up-front validation that reports targeted diagnostics and skips invalid methods instead of crashing the generator.

Changes:

• Introduces TryCreate factories for Protobuf/Ice/Slice ServiceMethod implementations that validate attribute shape and required generated artifacts, reporting IRSG0002/IRSG0003/IRSG0004 and returning null on mismatch.
• Threads an Action<Diagnostic> reporter through IServiceMethodFactory.TryCreate / ServiceMethodFactory so parsing can report issues and skip offending methods.
• Adds new diagnostic descriptors for attribute-shape mismatches, unexpected method signatures, and missing generated Request decode methods.

Reviewed changes

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

Show a summary per file

File	Description
src/IceRpc.ServiceGenerator/Internal/SliceServiceMethod.cs	Refactors Slice service-method creation into TryCreate with diagnostics and null-on-mismatch behavior.
src/IceRpc.ServiceGenerator/Internal/ServiceMethod.cs	Extends the factory API to accept a diagnostic reporter and allows creation to fail (null) cleanly.
src/IceRpc.ServiceGenerator/Internal/ProtobufServiceMethod.cs	Refactors Protobuf service-method creation into TryCreate and adds initial signature/attribute validation with diagnostics.
src/IceRpc.ServiceGenerator/Internal/Parser.cs	Plumbs diagnostic reporting into service method factories during parsing.
src/IceRpc.ServiceGenerator/Internal/IceServiceMethod.cs	Refactors Ice service-method creation into TryCreate with diagnostics and null-on-mismatch behavior.
src/IceRpc.ServiceGenerator/Internal/DiagnosticDescriptors.cs	Adds IRSG0002/IRSG0003/IRSG0004 diagnostic descriptors.

[Copilot]

decodeArgsMethod.ReturnType is treated as “ValueTask or ValueTask” based solely on IsGenericType, but this doesn't verify the return type is actually ValueTask/ValueTask<T>. If the generated Request.Decode...Async shape changes (or is hand-authored) to return Task/Task<T>, this will compute _parameterCount and proceed, likely producing uncompilable dispatcher code. Consider validating the decode method return type is System.Threading.Tasks.ValueTask/ValueTask<T> and reporting IRSG0003 when it isn't.

Suggested change

	if (decodeArgsMethod.ReturnType is INamedTypeSymbol returnType &&
	returnType.IsGenericType &&
	returnType.TypeArguments.Length == 1)
	{
	if (decodeArgsMethod.ReturnType is not INamedTypeSymbol returnType ||
	returnType.Name != "ValueTask" ||
	returnType.Arity > 1 ||
	returnType.ContainingNamespace.ToDisplayString() != "System.Threading.Tasks")
	{
	DiagnosticDescriptor invalidDecodeReturnTypeDescriptor = new(
	id: "IRSG0003",
	title: "Invalid generated request decode method return type",
	messageFormat:
	"The generated request decode method '{0}' on '{1}' must return System.Threading.Tasks.ValueTask or System.Threading.Tasks.ValueTask<T>.",
	category: "IceRpc.ServiceGenerator",
	defaultSeverity: DiagnosticSeverity.Error,
	isEnabledByDefault: true);
	reportDiagnostic(Diagnostic.Create(
	invalidDecodeReturnTypeDescriptor,
	decodeArgsMethod.Locations.FirstOrDefault() ?? location,
	decodeArgsMethod.Name,
	interfaceSymbol.GetFullName()));
	return null;
	}
	if (returnType.IsGenericType && returnType.TypeArguments.Length == 1)
	{

[Copilot]

This code assumes any non-generic return type is a non-generic ValueTask (“returnCount stays 0”), and any generic return type with one type argument is a ValueTask<T>. That means a hand-authored [IceOperation] on a method returning void/Task/Task<T> can slip through, and the generator will then emit dispatcher code that fails to compile instead of producing IRSG0003. Consider explicitly validating method.ReturnType is ValueTask or ValueTask<T> and also validating method.Parameters.Length matches the expected parameterCount + 2 (args + features + cancellationToken) before returning a non-null IceServiceMethod.

[Copilot]

decodeArgsMethod.ReturnType is interpreted as “ValueTask or ValueTask” using only IsGenericType/TypeArguments, without verifying the return type is actually System.Threading.Tasks.ValueTask/ValueTask<T>. Under Slice-generator/service-generator version skew (or hand-authored Request), this can yield an incorrect _parameterCount and lead to generated dispatcher code that doesn't compile. Consider validating the decode method return type is ValueTask/ValueTask<T> and report IRSG0003 when it isn't.

Suggested change

	int parameterCount = 0;
	string[] parameterFieldNames = [];
	if (decodeArgsMethod.ReturnType is INamedTypeSymbol returnType &&
	returnType.IsGenericType &&
	returnType.TypeArguments.Length == 1)
	if (decodeArgsMethod.ReturnType is not INamedTypeSymbol returnType ||
	returnType.Name != "ValueTask" ||
	returnType.ContainingNamespace.ToDisplayString() != "System.Threading.Tasks" ||
	(returnType.IsGenericType && returnType.TypeArguments.Length != 1))
	{
	reportDiagnostic(Diagnostic.Create(
	DiagnosticDescriptors.MissingGeneratedRequestClass,
	location,
	interfaceSymbol.GetFullName(),
	dispatchMethodName,
	operationName));
	return null;
	}
	int parameterCount = 0;
	string[] parameterFieldNames = [];
	if (returnType.IsGenericType)

[Copilot]

Return-type validation for the service method assumes “non-generic == ValueTask” and “generic with 1 type arg == ValueTask”, but does not check that method.ReturnType is actually ValueTask/ValueTask<T>. Likewise, there is no validation that method.Parameters.Length matches the expected parameterCount + 2 (decoded args + features + cancellationToken). With a malformed [SliceOperation] method, this factory can succeed and generate code that fails to compile instead of reporting IRSG0003 and skipping the method. Consider adding explicit ValueTask/parameter-count checks and returning null with InvalidRpcMethodSignature on mismatch.

[Copilot]

TryCreate only checks method.Parameters.Length == 0, but the generated dispatch lambda always calls service.<Method>(input, features, cancellationToken). If a user applies [RpcMethod] to a method with 1–2 parameters, this factory will succeed and then generate code that fails to compile (wrong argument count) instead of reporting IRSG0003 and skipping the method. Consider validating the full expected parameter list (at least parameter count == 3, and ideally that the last two parameters are IceRpc.Features.IFeatureCollection and CancellationToken) and return null with InvalidRpcMethodSignature when it doesn't match.

[Copilot]

The return-type validation only checks for a generic type with one type argument, but does not verify that the return type is actually ValueTask<T>. A method returning Task<T> (or any other generic type) would pass this check and lead to generated code that doesn't type-check against Dispatch*Async expectations. Consider additionally checking genericReturnType.OriginalDefinition equals System.Threading.Tasks.ValueTask<T> (and reporting IRSG0003 if not).

[pepone]

I think this adds quite a lot of complexity for an unlikely scenario, we depend on a exact version of the service-generator, and this errors are likely only possible during development when you are updating the genertors.