`assert` keyword: have compiler emit call to `Debug.Assert` overload with `CallerArgumentExpression` where possible

[brianrourkeboll]

Following https://github.com/dotnet/fsharp/pull/17519, we should see if it is possible for the F# compiler to emit a call to the System.Diagnostics.Debug.Assert overload that takes a message defaulting to CallerArgumentExpression for all usages of the assert keyword.

I.e., an expression like this

assert not true

would be translated to

System.Diagnostics.Debug.Assert (not true, "not true")

Likewise:

assert (x = 3)

→

System.Diagnostics.Debug.Assert ((x = 3), "(x = 3)")

etc.

The C# compiler now does this for System.Diagnostics.Debug.Assert(booleanExpr) by means of the OverloadResolutionPriorityAttribute, which F# does not currently support: https://github.com/dotnet/fsharp/issues/16967#issuecomment-2218267688

dotnet/csharplang#7906: developers can add weight to which methods are better in overload resolution. This seems unlikely to impact F# as much.

Want to point out one place this will intersect with F#. Consider that very likely Debug will end up looking like the following:

public static class Debug
{
  [OverloadResolutionPriority(-1)]
  public static void Assert(bool condition) { ... } 

  public static void Assert(bool condition, [CallerArgumentExpression] string? message = "") { ... }
}

Nothing will break for F# here when this happens, code will still compile as it used to. The experience for C# though will improve from a lot of Debug.Assert failed messages to the actual expression that passed into the assert. This is one part I thought might be interesting to F# .

See the BCL source:

[Conditional("DEBUG")]
[OverloadResolutionPriority(-1)] // lower priority than (bool, string) overload so that the compiler prefers using CallerArgumentExpression
public static void Assert([DoesNotReturnIf(false)] bool condition) =>
    Assert(condition, string.Empty, string.Empty);


[Conditional("DEBUG")]
public static void Assert([DoesNotReturnIf(false)] bool condition, [CallerArgumentExpression(nameof(condition))] string? message = null) =>
    Assert(condition, message, string.Empty);

SharpLab F# SharpLab C#

We could in theory do this in the F# compiler specifically for the assert keyword, without needing to support OverloadResolutionPriorityAttribute in general, by emitting a call to the new overload when it is available here:

https://github.com/dotnet/fsharp/blob/69be2cd095394e3791dd26c5dcb45bd35d098535/src/Compiler/Checking/Expressions/CheckExpressions.fs#L7699-L7706

[ijklam]

I see that the Assert(Boolean, String) method is available on all .NET runtime, so maybe this can be implemented without the CallerArgumentExpression, just read the source text out, put it to the arguments, and this can benefits all F# versions.

[T-Gro]

Once #17519 lands, it is either dedicated lookup at TcAssertExpr (which will be easy once the mechanics for obtaining text are merged) OR support OverloadResolutionPriority and get few of the other usages as side benefit (https://github.com/search?q=repo%3Adotnet%2Fruntime+OverloadResolutionPriority&type=code&p=1 - but it isn't that many...)

[ijklam]

This feature needs we can correctly recognize ranges with #line directives, otherwise it will make it not possible to build the compiler itself.

/home/vsts/work/1/s/src/Compiler/pars.fsy(2111,29): error FS0193: internal error: startIndex cannot be larger than length of string. (Parameter 'startIndex') [/home/vsts/work/1/s/src/Compiler/FSharp.Compiler.Service.fsproj::TargetFramework=net9.0]
/home/vsts/work/1/s/src/Compiler/pars.fsy(2125,29): error FS0193: internal error: startIndex cannot be larger than length of string. (Parameter 'startIndex') [/home/vsts/work/1/s/src/Compiler/FSharp.Compiler.Service.fsproj::TargetFramework=net9.0]
/home/vsts/work/1/s/src/Compiler/pars.fsy(2139,29): error FS0193: internal error: Index and length must refer to a location within the string. (Parameter 'length') [/home/vsts/work/1/s/src/Compiler/FSharp.Compiler.Service.fsproj::TargetFramework=net9.0]
/home/vsts/work/1/s/src/Compiler/pars.fsy(2111,29): error FS0193: internal error: startIndex cannot be larger than length of string. (Parameter 'startIndex') [/home/vsts/work/1/s/src/Compiler/FSharp.Compiler.Service.fsproj::TargetFramework=netstandard2.0]
/home/vsts/work/1/s/src/Compiler/pars.fsy(2125,29): error FS0193: internal error: startIndex cannot be larger than length of string. (Parameter 'startIndex') [/home/vsts/work/1/s/src/Compiler/FSharp.Compiler.Service.fsproj::TargetFramework=netstandard2.0]
/home/vsts/work/1/s/src/Compiler/pars.fsy(2139,29): error FS0193: internal error: Index and length must refer to a location within the string. (Parameter 'length') [/home/vsts/work/1/s/src/Compiler/FSharp.Compiler.Service.fsproj::TargetFramework=netstandard2.0]
    0 Warning(s)
    6 Error(s)

[T-Gro]

The nowarn PR which has landed has reworked #line directive management, can that help now?

[Martin521]

The nowarn PR which has landed has reworked #line directive management, can that help now?

#line directive management is not really reworked, it is still the same flawed thing. The PR just added a workaround that is used to map the lines back to the original (or raise a warning if that is not possible).

A real reworking would build a forward map instead of this backward map, keep the original line numbers in the ranges, and map them only when we really want the mapping. Which is only a) when formatting diagnostic messages and b) when writing the pdb file.

I think this should be doable. I have a feasibility check on my todo list. (In hindsight, I should have tackled this before doing the nowarn PR. But this insight came too late.)

[ijklam]

The original line maps introduced from the nowarn PR works badly when the compiler is compiling a file with multiple #lines for the CallerArgumentExpression cases. See this example:

# 1
System.ArgumentNullException.ThrowIfNullOrEmpty (null: (* line 1 *) string | null)

# 1
System.ArgumentNullException.ThrowIfNullOrEmpty (null: (* line 2 *) string | null)

It will compiles to

    System.ArgumentNullException.ThrowIfNullOrEmpty(null, "null: (* line 2 *) string | null");
    System.ArgumentNullException.ThrowIfNullOrEmpty(null, "null: (* line 2 *) string | null");

Maybe the original line maps only works good during parsing files? And I might mistakenly recognized it will work good too after parsing.