[WIP] replace sprintf to string.format for FSharp.Core.UnitTests

[FoggyFinder]

Remove the use of sprintf for FSharp.Core.UnitTests

#4954

[vasily-kirichenko]

Are you sure it's safe to replace %A with ToString here?

[FoggyFinder]

@vasily-kirichenko no, I have strong doubt here. Dunno why I decided to change it. I revert these lines.

[KevinRansom]

Just out of curiosity, why?

[charlesroddie]

@KevinRansom This will allow adjusted tests to run under CoreRT and .Net Native.

Requested by @dsyme : "If you like you can submit a PR to remove the use of sprintf throughout FSHarp.Core.UnitTests except where that's the thing explicitly being tested. It's useful anyway to reduce the complexity of what's being executed for different tests."

[dsyme]

I'm ok with this as a general engineering improvement, since the dependency basis of the individual tests is then much slimmer. This is useful any time anyone wants to take F# to a different .NET implementation or for Fable/JS, WebSharper, Fez and so on.

[zpodlovics]

@FoggyFinder Just a quick note. Each different printf expression should be factored out to the beginning of the file. In this way it's lot easier to refactor in case the replaced mechanism has some bugs and lot easier adapt to different F# implementations (Fable, WebSharper, Fez, etc).

From my earlier issue (#4954 (comment)): Please note, some earlier test cases was carefully designed and intentionally avoided this issue by factoring out the printing functionality to the beginning of the test code:

let failures = ref []

let report_failure (s : string) = 
    stderr.Write" NO: "
    stderr.WriteLine s
    failures := !failures @ [s]

let test (s : string) b = 
    stderr.Write(s)
    if b then stderr.WriteLine " OK"
    else report_failure (s)

For example a relative simple printf refactoring for: tests/fsharp/core/array/test.fsx The postfix of the function is usually the type as specified in printf syntax plus each argument name. eg.: sprintf_dd_hilo hi lo -> sprintf %d %d functionname argument1name argument2name.

let isNonZeroLowerBoundArraySupported = 
#if NETCORERTAPP2_0
    false
#else    
    true
#endif

let sprintf_dd_hilo hi lo =
#if NETCORERTAPP2_0
    System.String.Format("vre9u0rejkn, lo = {0}, hi = {1}", lo, hi)
#else    
    sprintf "vre9u0rejkn, lo = %d, hi = %d" lo hi
#endif

let printf_sb_eq s b =
#if NETCORERTAPP2_0
    System.Console.WriteLine("{0} = {1}", s, b)
#else    
    printfn "%s = %b" s b;
#endif

let sprintf_sa_n s a N =
#if NETCORERTAPP2_0
    System.String.Format("{0}-{1}", s, (a,N))
#else
    sprintf "%s-%A" s (a,N)
#endif    

let sprintf_sa_nstep s a step N =
#if NETCORERTAPP2_0
    System.String.Format("{0}-{1}", s ,(a,step,N))
#else
    sprintf "%s-%A" s (a,N)
#endif    

@dsyme Could you please confirm that %A, %O in the these mostly used for structural comparison in the tests. Can we assume in the testsuite that the F# structural comparison is correct (except tests that target the structural comparison itself)? So instead of using printf generated strings to do structural comparison we can use the F# compiler to do structural comparison eg.: by defining a few specific F# record type. However these f# structural comparisons works without dynamically generated code.

[charlesroddie]

@zpodlovics some earlier test cases was carefully designed and intentionally avoided this issue by factoring out the printing functionality to the beginning of the test code

Agreed that this would be nicer, and your first code block (report_failure etc.) shows a good structure, but I do not think we need #if definitions as you suggest lower down, because we do not need to maintain sprintf. As @dsyme said, moving away from sprintf (except where sprintf is specifically being tested) would 1. be an improvement all round, simplifying and reducing dependencies, and 2. may apply to more compilation toolchains than we know about at the time of writing the tests.

[zpodlovics]

@charlesroddie Yes, I agree. The ifdefs could and should be removed when the testcase refactoring finished. I leaved temporarly there so a simple conditional compiation allow me to identify the differences between the original printf vs new format strings (the typechecker helps a lot here). It's lot easier to fine tune a few common output format writer function factored out to the beginning of the test than replace one by one in 500+ line. However there is a huge difference between printf and format strings. Printf will properly typecheck both the format string and also the matching parameter type. You can only provide similar functionality with factored out format writer functions where each parameter have proper type definition.

Quick tip: The easiest way I found to identify printfs is by "redefining" it in the begining of the test (but right after the factored out output writer functions) so the editor will mark each printf/sprintf usage with typecheck error.

let printf = ()
let printfn = ()
let sprintf = ()
let sprintfn = ()

[zpodlovics]

@charlesroddie Also, in several cases conditional compilation will and still required, because corert does not support (or will not) support some features eg.: non-zero indexed arrays, dynamic codegen with deeply nested generics

[FoggyFinder]

Well, I think all occurrences of sprintf which could easily and safely replacing to String.Format were removed.

There are two sprintf:

(sqs = arr) |@ (sprintf  "Seq.%s = '%A', Array.%s = '%A'" name sqs name arr) .&. 
(ls  = arr) |@ (sprintf "List.%s = '%A', Array.%s = '%A'" name ls name arr)

Is it better to replace it with String.Format + custom function for formatting or leave unchanged?

What should to do with printf/bprintf also?

[charlesroddie]

bprintf sb f = sprintf f >>sb.Append. That is straightforward.
printf sb = sprintf sb >> Console.Write, although should probably be sprintf sb >> print so print can be defined more appropriately if needed.

[zpodlovics]

You can simply create a function with printf + format string, something like this:

eg.:

let printf1 s1 a1 s2 a2 = sprintf "Seq.%s = '%A', Array.%s = '%A'" s1 a1 s2 a2

or for a non generic case:

let printf1 = sprintf "Seq.%s = '%d', Array.%s = '%d'" s1 a1 s2 a2

[KevinRansom]

So this is a tricky PR for me. It does nothing positive for the project, and basically rewrites perfectly legal idiomatic F# for perfectly legal idiomatic .NET code. It seems to be proposed as a work around for the fact that F# as it is currently implemented is not well suited to work on target platforms that do not support a rich set of reflection APIs at runtime. Examples of this are the CoreRT OSS project on github, and perhaps Fable with a javascript code generation.

I disagree that there is any interesting engineering benefit to making this change, indeed it would cause us to have an additional code review item, to ensure no additional use of sprint occurs in future FSharp.Core test cases.

It is not our goal to fork the language based on platform capabilities, we have had enough issues with that due to the many flavors of portable libraries, we still have a Desktop / NetStandard fork, which needs to go away. We would support an effort to reduce, or better yet eliminate the requirement for reflection during printfn/printf/sprintf. Perhaps, including compile time warnings when the compiler detects that runtime reflection may be required.

Obviously this is going to be a lot of effort but would yield significant benefits to any future Ahead of Time Compilation or Transpiler strategies that F# might follow.

In the absence of a plan for something like the above, it is not clear, why this PR represents a step forward.

[charlesroddie]

We would support an effort to reduce, or better yet eliminate the requirement for reflection during printfn/printf/sprintf. Obviously this is going to be a lot of effort but would yield significant benefits to any future Ahead of Time Compilation or Transpiler strategies that F# might follow.

Agreed that this would be a preferable approach, since it would improve the compatibility and reduce the complexity of these methods in F# code at large, not just in the test suite. Unfortunately I don't believe I have the skills to help with this or to estimate its feasibility. Any thoughts @zpodlovics as you looked at that code recently?

[dsyme]

To me this as a basically harmless engineering improvement but I don't really care that much either way. I've often thought about doing it myself, especially when I've just inserted some bug deep into F# that affects "sprintf" and I want it caught early and obviously.

The way I see it is this: the FSharp.Core unittests are especially useful during the process of trialling F# on different runtime substrates. Historical examples where we've done this include WebSharper, Fez, Silverlight, PCL, Fable, testing various F# interpreters and dynamic compilers (Unquote etc.), bringing up F# on .NET Core 1.x etc. Just last week I was writing an F# interpreter to run F# code in a LivePlayer on mobile devices and minimized unit tests would have been useful.

So if it gets done, sure, it's useful, I approve, but I don't care much either way.

@charlesroddie Since @KevinRansom doesn't like this, perhaps just make a fork of the tests instead?

[dsyme]

We would support an effort to reduce, or better yet eliminate the requirement for reflection during printfn/printf/sprintf. Obviously this is going to be a lot of effort but would yield significant benefits to any future Ahead of Time Compilation or Transpiler strategies that F# might follow.

This makes sense. If it helps the history is this:

• Once upon a time (approx F# 0.6) we used compiler-generated code to avoid any reflection, so a printf string would become compiler-generated code. That can work but was at the time buggy, complex, created large numbers of closures and was not particularly fast. But the decisive thing at the time was that the FSharp.Core API supporting this code generation felt unstable and overly complex.

• In F# 1.x we moved to runtime interpretation, which needs some reflection (dynamic instantiations via MakeGenericMethod and MakeGenericType) but no explicit code generation (no refemit). This means it works on Xamarin iOS (where a last-resort imlpementation of generic code is always available, but codegen is not permitted) but not on .NET Native and CoreRT (where no last-resort implementation of generic code is available, sadly)

• In F# 3.x we moved to "fast" runtime interpretation, the current implementation (done by @v2m), with the same dependency base.

This all means I basically put the current F# "printf" implementation in the same category as LINQ: unlikely to work on implementations of .NET that don't support arbitrary MakeGenericType/MakeGenericMethod. (For this reason I'd also recommend avoiding the use of LINQ when testing primitives - as a habit but not a rule :) )

Finally a general comment: looking from F# 4.x perspective it could make sense for printf to be a type-provider-like compiler addin, responsible for checking format strings and turning format strings into type-erased code.

cheers
don

[charlesroddie]

@dsyme Since @KevinRansom doesn't like this, perhaps just make a fork of the tests instead?

Yes we'll keep working on this fork for use in manual testing of .Net Native, since changes to printfn/printf/sprintf sound like they won't be done in the short term. We'll preserve this WIP PR for visibility. @FoggyFinder

[KevinRansom]

It sounds like you have a plan. I would really prefer, if there was someone to champion looking at how we need to react to platforms without reflection, although that may I suppose have to wait until we fund it, here which will certainly not be any time soon.

You should also submit any tests you care about to the coreRT repo. I would hope that they would probably take IL assemblies that you generate. At least that way, anything you determine works would stay working.