Pass caller argument expression through RoslynDebug.Assert #76435
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| @@ -13,12 +13,21 @@ namespace Roslyn.Utilities | |||
| internal static class RoslynDebug | |||
| { | |||
| /// <inheritdoc cref="Debug.Assert(bool)"/> | |||
| #if NET9_0_OR_GREATER | |||
| [OverloadResolutionPriority(-1)] | |||
There was a problem hiding this comment.
Is that really needed? Can't the other overload exist alone?
On .NET, they had to do this for backcompat. But as RoslynDebug is internal, I think it should be safe to just have the right overload?
There was a problem hiding this comment.
Might not be needed as you say. But I'd rather not risk some other DLL depending on this (via InternalsVisibleTo) and then getting MissingMethodExceptions.
There was a problem hiding this comment.
The way our IVT is setup (no IVT outside our repo) it should be safe to remove this if the repo still compiles.
There was a problem hiding this comment.
Alright, I will remove the overload resolution priority attribute.
| public static void Assert([DoesNotReturnIf(false)] bool condition, string message) | ||
| public static void Assert([DoesNotReturnIf(false)] bool condition, | ||
| #if NET | ||
| [CallerArgumentExpression(nameof(condition))] string? message = null |
There was a problem hiding this comment.
Why are we using the #if NET condition here? I get that only the core version of Debug.Assert has this overload but not sure if that should stop us from making RoslynDebug.Assert better.
There was a problem hiding this comment.
CallerArgumentExpressionAttribute isn't available on net framework
| public static void Assert([DoesNotReturnIf(false)] bool condition, string message) | ||
| public static void Assert([DoesNotReturnIf(false)] bool condition, | ||
| #if NET | ||
| [CallerArgumentExpression(nameof(condition))] string? message = null |
| @@ -18,7 +18,13 @@ internal static class RoslynDebug | |||
|
|
|||
| /// <inheritdoc cref="Debug.Assert(bool, string)"/> | |||
| [Conditional("DEBUG")] | |||
| public static void Assert([DoesNotReturnIf(false)] bool condition, string message) | |||
| public static void Assert([DoesNotReturnIf(false)] bool condition, | |||
There was a problem hiding this comment.
Agreed with this. I thought the main reason we still kept RoslynDebug.Assert around was that we haven't removed all of the callsites.
Makes RoslynDebug.Assert more consistent with Debug.Assert. Without this, on .NET 9, the message
"condition"is effectively being passed intoDebug.Assertwhich is not very useful.