Add IFileSystem.SourceFileReadShim returning char[]

[auduchinok]

Adds API allowing to pass char[] to be used by lexer instead of returning Stream which is read to string which is then thrown away with .ToCharArray().

PR continues work by @smoothdeveloper in #4882 but is aimed at the file system shim clients.

It would be great to use ReadOnlySpan<char> (and probably make this API return string) instead at some point.

[auduchinok]

Anything I should change?

[forki]

Ping

[smoothdeveloper]

I'm not sure I'm grasping the changes so I'll put few questions/points to try having better understanding:

• we remove member returning a Stream: Stream can be used in all kinds of wrong way, and need to be disposed, so it is probably good
• we pass char[] instead of string in several places: is it saving a roundtrip from char[] to string to char[] somewhere?
• in SourceFileReadShim, we use ReadToEnd (which gives a string) and then call ToCharArray on this, is this the only or best way? I guess reading to char directly isn't possible due to encoding, not knowing the array size, and we assume that String.ToCharArray is most efficient for that

EDIT: actually, yes it saves some char[] to string to char[] conversions, we save call to this: https://github.com/Microsoft/visualfsharp/blob/75e435e74b81bb47c4a0ced11be0d7560d2a66ea/src/fsharp/UnicodeLexing.fs#L17-L18

it might be worth checking if there are remaining internal usage of StringAsLexbuf that could instead deal with char [] if available around.

[cartermp]

@auduchinok Unfortunately, to use spans here (which I agree would be great) would require us to figure out how to adopt them all-up in the compiler. They are only available in System for .NET Core 2.1+, with no current plans to bring them into .NET Standard or .NET Framework. There will likely be a NuGet package for .NET Framework consumers, but it's not clear if it will actually be a benefit for those consumers since the desktop CLR does not have the runtime changes that were required for Span performance.

Given this, I think we should take this PR as-is (assuming it is approved) and then open a discussion about how to start using spans once F# 4.5 is fully released.

Based on benchmarking we've done, there are places where copying can be reduced quite a bit, which implies that judicious use of Span could help. But as it stands, the majority of F# users may not see those benefits.

[cartermp]

Good overall change, and should help with what was measured in #4881

[KevinRansom]

Failed test:

IDE Unit tests

2018-09-20T05:22:24.8962125Z Errors and Failures
2018-09-20T05:22:24.8962207Z 
2018-09-20T05:22:24.8962357Z 1) Failed : Tests.Service.FileSystemTests.FileSystem compilation test
2018-09-20T05:22:24.8962556Z   Expected: 2
2018-09-20T05:22:24.8962689Z Actual: 0
2018-09-20T05:22:24.8962811Z   Expected: 2
2018-09-20T05:22:24.8962933Z   But was:  0
2018-09-20T05:22:24.8963113Z at FsUnit.shouldEqual[a](a x, a y)
2018-09-20T05:22:24.8963280Z at Tests.Service.FileSystemTests.FileSystem compilation test()
2018-09-20T05:22:24.8963385Z 
2018-09-20T05:22:24.8963543Z Run Settings
2018-09-20T05:22:24.8963675Z     RuntimeFramework: V4.0
2018-09-20T05:22:24.8963808Z     RunAsX86: True
2018-09-20T05:22:24.8963957Z     WorkDirectory: D:\a\1\s\release\net40\bin
2018-09-20T05:22:24.8964135Z     MaxAgents: 1
2018-09-20T05:22:24.8964270Z     NumberOfTestWorkers: 1
2018-09-20T05:22:24.8964402Z     Verbose: True
2018-09-20T05:22:24.8964582Z 
2018-09-20T05:22:24.8964754Z Test Run Summary
2018-09-20T05:22:24.8964889Z     Overall result: Failed
2018-09-20T05:22:24.8965057Z    Tests run: 3085, Passed: 3084, Errors: 0, Failures: 1, Inconclusive: 0
2018-09-20T05:22:24.8965293Z      Not run: 102, Invalid: 0, Ignored: 102, Explicit: 0, Skipped: 0
2018-09-20T05:22:24.8965470Z   Start time: 2018-09-20 05:06:28Z
2018-09-20T05:22:24.8965617Z     End time: 2018-09-20 05:22:24Z
2018-09-20T05:22:24.8965814Z     Duration: 955.550 seconds
2018-09-20T05:22:24.8966130Z 
2018-09-20T05:22:24.9247692Z Results (nunit3) saved as D:\a\1\s\tests\TestResults\test-vs-ideunit-results.xml

[KevinRansom]

@auduchinok, I'm guessing we should close this, what do you think?

Kevin

[auduchinok]

It's OK to close it, since APIs are changed a bit since then, thank you. I'll probably open another one if ever continue this work.