ExpressionCompiler: Add processing for types implementing implicit cast operator (STUD-78703) #392
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds processing for types implementing implicit cast operators to the ExpressionCompiler, addressing STUD-78703. The implementation refactors the FormatWithTypeCast method to intelligently determine the appropriate casting strategy based on whether user-defined implicit operators exist between types.
Changes:
- Refactored
FormatWithTypeCastto use a strategy-based approach for determining casting behavior - Added logic to detect user-defined implicit conversion operators between source and target types
- Introduced helper methods for variable type lookup and casting strategy determination
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == targetType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == sourceType); | ||
|
|
||
| bool hasTargetToSource = sourceType.GetMethods(flags) | ||
| .Concat(targetType.GetMethods(flags)) | ||
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == sourceType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == targetType); |
There was a problem hiding this comment.
The method calls GetParameters() twice on the same method within the LINQ query predicate (once to check Length and once to get ParameterType). GetParameters() can be expensive as it allocates a new array each time. Consider storing the result in a local variable within the lambda or use a more efficient approach to avoid redundant array allocations.
| .Any(m => m.Name == "op_Implicit" | |
| && m.ReturnType == targetType | |
| && m.GetParameters().Length == 1 | |
| && m.GetParameters()[0].ParameterType == sourceType); | |
| bool hasTargetToSource = sourceType.GetMethods(flags) | |
| .Concat(targetType.GetMethods(flags)) | |
| .Any(m => m.Name == "op_Implicit" | |
| && m.ReturnType == sourceType | |
| && m.GetParameters().Length == 1 | |
| && m.GetParameters()[0].ParameterType == targetType); | |
| .Any(m => | |
| { | |
| var parameters = m.GetParameters(); | |
| return m.Name == "op_Implicit" | |
| && m.ReturnType == targetType | |
| && parameters.Length == 1 | |
| && parameters[0].ParameterType == sourceType; | |
| }); | |
| bool hasTargetToSource = sourceType.GetMethods(flags) | |
| .Concat(targetType.GetMethods(flags)) | |
| .Any(m => | |
| { | |
| var parameters = m.GetParameters(); | |
| return m.Name == "op_Implicit" | |
| && m.ReturnType == sourceType | |
| && parameters.Length == 1 | |
| && parameters[0].ParameterType == targetType; | |
| }); |
| using System.Linq; | ||
| using System.Linq.Expressions; | ||
| using System.Reflection; | ||
| using System.Runtime.Serialization; |
There was a problem hiding this comment.
The using directive for System.Runtime.Serialization is added but never used in the code. This unused import should be removed to keep the code clean.
| using System.Runtime.Serialization; |
| // Check if there are implicit operators in both directions | ||
| bool hasSourceToTarget = sourceType.GetMethods(flags) | ||
| .Concat(targetType.GetMethods(flags)) | ||
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == targetType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == sourceType); | ||
|
|
||
| bool hasTargetToSource = sourceType.GetMethods(flags) | ||
| .Concat(targetType.GetMethods(flags)) | ||
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == sourceType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == targetType); | ||
|
|
||
| // Return true only if there are operators in BOTH directions | ||
| return hasSourceToTarget && hasTargetToSource; |
There was a problem hiding this comment.
The logic for HasUserDefinedImplicitOperator requires BOTH directions (source to target AND target to source) of implicit conversion to exist. This requirement is too strict and incorrect. In C#, an implicit conversion only needs to exist in ONE direction. For example, if a type CustomInt has 'public static implicit operator int(CustomInt value)', the compiler can implicitly convert CustomInt to int without requiring the reverse operator. Requiring bidirectional operators will miss most legitimate implicit conversions. The method should check for hasSourceToTarget only, not require hasTargetToSource.
| private Type FindVariableType(string expressionText) | ||
| { | ||
| // Search through compiled data contexts (from most recent to oldest) | ||
| foreach (var compiledDataContext in _compiledDataContexts) | ||
| { | ||
| return SafeCastText(coreExpressionText, resultType); | ||
| if (compiledDataContext.Fields.TryGetValue(expressionText, out var memberData) || | ||
| compiledDataContext.Properties.TryGetValue(expressionText, out memberData)) | ||
| { | ||
| return memberData.Type; | ||
| } | ||
| } | ||
|
|
||
| return ExplicitCastText(coreExpressionText, resultType); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The FindVariableType method only looks up simple variable identifiers in the compiled data contexts. It will return null for any complex expression (e.g., "myVar.Property", "myMethod()", "myVar + 1"). This limitation means the implicit operator detection will not work for any expression beyond a simple variable name. Consider either documenting this limitation clearly in the method comment, or renaming the method to something like "FindSimpleVariableType" to indicate that it only handles simple identifiers.
| // Check if there are implicit operators in both directions | ||
| bool hasSourceToTarget = sourceType.GetMethods(flags) | ||
| .Concat(targetType.GetMethods(flags)) | ||
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == targetType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == sourceType); | ||
|
|
||
| bool hasTargetToSource = sourceType.GetMethods(flags) | ||
| .Concat(targetType.GetMethods(flags)) | ||
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == sourceType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == targetType); | ||
|
|
||
| // Return true only if there are operators in BOTH directions |
There was a problem hiding this comment.
The comment states that operators are checked in both directions and returns true only if operators exist in BOTH directions, but this behavior is incorrect for implicit operator semantics. Implicit operators are unidirectional by design in C#. The comment should be corrected to reflect the proper logic once the bug is fixed.
| private string FormatWithTypeCast(string coreExpressionText, Type resultType) | ||
| { | ||
| if (resultType == null) | ||
| // Early return for null or empty inputs | ||
| if (resultType == null || string.IsNullOrEmpty(coreExpressionText)) | ||
| { | ||
| return coreExpressionText; | ||
| } | ||
|
|
||
| var variableType = FindVariableType(coreExpressionText); | ||
|
|
||
| // If types are exactly identical, no cast needed | ||
| if (variableType != null && variableType == resultType) | ||
| { | ||
| return coreExpressionText; | ||
| } | ||
|
|
||
| if (CanUseSafeCastOperator(resultType)) | ||
| // Determine the best casting strategy | ||
| var castingStrategy = DetermineCastingStrategy(variableType, resultType); | ||
|
|
||
| return castingStrategy switch | ||
| { | ||
| CastingStrategy.NoCast => coreExpressionText, | ||
| CastingStrategy.SafeCast => SafeCastText(coreExpressionText, resultType), | ||
| CastingStrategy.ExplicitCast => ExplicitCastText(coreExpressionText, resultType), | ||
| _ => coreExpressionText | ||
| }; | ||
| } | ||
|
|
||
| private Type FindVariableType(string expressionText) | ||
| { | ||
| // Search through compiled data contexts (from most recent to oldest) | ||
| foreach (var compiledDataContext in _compiledDataContexts) | ||
| { | ||
| return SafeCastText(coreExpressionText, resultType); | ||
| if (compiledDataContext.Fields.TryGetValue(expressionText, out var memberData) || | ||
| compiledDataContext.Properties.TryGetValue(expressionText, out memberData)) | ||
| { | ||
| return memberData.Type; | ||
| } | ||
| } | ||
|
|
||
| return ExplicitCastText(coreExpressionText, resultType); | ||
| return null; | ||
| } | ||
|
|
||
| private static CastingStrategy DetermineCastingStrategy(Type sourceType, Type targetType) | ||
| { | ||
| // If source type is unknown, use safe casting for reference types | ||
| if (sourceType == null) | ||
| { | ||
| return CanUseSafeCastOperator(targetType) ? CastingStrategy.SafeCast : CastingStrategy.ExplicitCast; | ||
| } | ||
|
|
||
| // Check for user-defined implicit operators | ||
| if (HasUserDefinedImplicitOperator(sourceType, targetType)) | ||
| { | ||
| return CastingStrategy.NoCast; | ||
| } | ||
|
|
||
| if (CanUseSafeCastOperator(targetType)) | ||
| { | ||
| return CastingStrategy.SafeCast; | ||
| } | ||
|
|
||
| return CastingStrategy.ExplicitCast; | ||
| } | ||
|
|
||
| private static bool HasUserDefinedImplicitOperator(Type sourceType, Type targetType) | ||
| { | ||
| if (sourceType == null || targetType == null) | ||
| return false; | ||
|
|
||
| // Don't use user-defined operators for nullable types - they have special semantics | ||
| if (IsNullableValueType(targetType) || IsNullableValueType(sourceType)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| const BindingFlags flags = BindingFlags.Public | BindingFlags.Static; | ||
|
|
||
| try | ||
| { | ||
| // Check if there are implicit operators in both directions | ||
| bool hasSourceToTarget = sourceType.GetMethods(flags) | ||
| .Concat(targetType.GetMethods(flags)) | ||
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == targetType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == sourceType); | ||
|
|
||
| bool hasTargetToSource = sourceType.GetMethods(flags) | ||
| .Concat(targetType.GetMethods(flags)) | ||
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == sourceType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == targetType); | ||
|
|
||
| // Return true only if there are operators in BOTH directions | ||
| return hasSourceToTarget && hasTargetToSource; | ||
| } | ||
| catch | ||
| { | ||
| // In case of reflection errors, assume no user-defined operators | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The PR description states "Pending: add tests" which indicates that test coverage for the new implicit cast operator functionality has not been added yet. Given that this code introduces new behavior around type casting and implicit operators, comprehensive tests are needed to verify: 1) detection of user-defined implicit operators, 2) proper casting strategy selection, 3) handling of nullable types, 4) edge cases like complex expressions. Tests should be added before merging to ensure the functionality works correctly.
| catch | ||
| { | ||
| // In case of reflection errors, assume no user-defined operators | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The catch block swallows all exceptions without any logging or specific exception handling. While the comment explains the intent, reflection operations can fail for various reasons (security exceptions, type load exceptions, etc.). Consider at minimum catching only expected exceptions like SecurityException, TypeLoadException, or MemberAccessException rather than catching all exceptions indiscriminately. This would help identify unexpected issues during development and debugging.
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == targetType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == sourceType); | ||
|
|
||
| bool hasTargetToSource = sourceType.GetMethods(flags) | ||
| .Concat(targetType.GetMethods(flags)) | ||
| .Any(m => m.Name == "op_Implicit" | ||
| && m.ReturnType == sourceType | ||
| && m.GetParameters().Length == 1 | ||
| && m.GetParameters()[0].ParameterType == targetType); |
There was a problem hiding this comment.
The method calls GetParameters() twice on the same method within the LINQ query predicate (once to check Length and once to get ParameterType). GetParameters() can be expensive as it allocates a new array each time. Consider storing the result in a local variable within the lambda or use a more efficient approach to avoid redundant array allocations.
| .Any(m => m.Name == "op_Implicit" | |
| && m.ReturnType == targetType | |
| && m.GetParameters().Length == 1 | |
| && m.GetParameters()[0].ParameterType == sourceType); | |
| bool hasTargetToSource = sourceType.GetMethods(flags) | |
| .Concat(targetType.GetMethods(flags)) | |
| .Any(m => m.Name == "op_Implicit" | |
| && m.ReturnType == sourceType | |
| && m.GetParameters().Length == 1 | |
| && m.GetParameters()[0].ParameterType == targetType); | |
| .Any(m => | |
| { | |
| var parameters = m.GetParameters(); | |
| return m.Name == "op_Implicit" | |
| && m.ReturnType == targetType | |
| && parameters.Length == 1 | |
| && parameters[0].ParameterType == sourceType; | |
| }); | |
| bool hasTargetToSource = sourceType.GetMethods(flags) | |
| .Concat(targetType.GetMethods(flags)) | |
| .Any(m => | |
| { | |
| var parameters = m.GetParameters(); | |
| return m.Name == "op_Implicit" | |
| && m.ReturnType == sourceType | |
| && parameters.Length == 1 | |
| && parameters[0].ParameterType == targetType; | |
| }); |
| foreach (var compiledDataContext in _compiledDataContexts) | ||
| { | ||
| return SafeCastText(coreExpressionText, resultType); | ||
| if (compiledDataContext.Fields.TryGetValue(expressionText, out var memberData) || | ||
| compiledDataContext.Properties.TryGetValue(expressionText, out memberData)) | ||
| { | ||
| return memberData.Type; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
1 participant
https://uipath.atlassian.net/browse/STUD-78703
Pending: add tests