core(checkSourceLengthRule): Fix issues with multiple block comments

[Numpsy]

ref #869 - showing an example of some source that causes an error with the current code, before doing any changes to try to fix it

[Numpsy]

This gets the extra test to pass, but the performance is worse - with the existing code I get this from the benchmarks

| Method         | Mean    | Error    | StdDev   | Gen0       | Gen1      | Gen2      | Allocated |
|--------------- |--------:|---------:|---------:|-----------:|----------:|----------:|----------:|
| LintParsedFile | 1.299 s | 0.0113 s | 0.0100 s | 15000.0000 | 6000.0000 | 2000.0000 | 262.67 MB |

and with this change it's

| Method         | Mean    | Error    | StdDev   | Gen0       | Gen1      | Gen2      | Allocated |
|--------------- |--------:|---------:|---------:|-----------:|----------:|----------:|----------:|
| LintParsedFile | 2.118 s | 0.0150 s | 0.0140 s | 19000.0000 | 8000.0000 | 2000.0000 | 307.44 MB |

Which seems a large drop, though less than it was prior to #879

Special casing the single-comment case to use String.Remove instead of StringBuilder shaves off 0.22 megabytes of allocations, so it may or may not be worth it.

I'll try profiling it to see where the extra time is spent.

[Numpsy]

Though maybe some of that change is because it changes the results of some of the following code

[Numpsy]

Ok, the change apparently makes the benchmark go into the if skipResult > config.MaxLines then case 962 times rather than the 566 times with the current code, which would make it slower due to finding more results

[knocte]

@Numpsy do the last commits help perf?

[Numpsy]

Ok, I see what's happening here - The benchmark lint on the master branch with the extended rule set is hitting the exception described in #869 whilst running the MaxLinesInUnion rule, and throwing all the way up into the exception handler at

FSharpLint/src/FSharpLint.Core/Application/Lint.fs

Line 313 in ea530b8

| exn -> Failed(fileInfo.File, exn) |> lintInfo.ReportLinterProgress

, and that error doesn't get reported during the benchmark run - so the benchmark appears to have completed, but has actually aborted after only running a subset of rules.
Dumping out the exception where it's caught gets

System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'start')
   at FSharpLint.Rules.Helper.SourceLength.stripMultilineComments@57-1.Invoke(String currSource, Tuple`2 tupledArg) in G:\Dev\FSharpLint\src\FSharpLint.Core\Rules\Conventions\SourceLength\SourceLengthHelper.fs:line 60
   at Microsoft.FSharp.Collections.ListModule.Fold[T,TState](FSharpFunc`2 folder, TState state, FSharpList`1 list) in D:\a\_work\1\s\src\fsharp\src\FSharp.Core\list.fs:line 295
   at FSharpLint.Rules.Helper.SourceLength.checkSourceLengthRule[a](Config config, Range range, String fileContents, a errorName, Range[] skipRanges) in G:\Dev\FSharpLint\src\FSharpLint.Core\Rules\Conventions\SourceLength\SourceLengthHelper.fs:line 69
   at FSharpLint.Rules.MaxLinesInUnion.runner(Config config, AstNodeRuleParams args) in G:\Dev\FSharpLint\src\FSharpLint.Core\Rules\Conventions\SourceLength\MaxLinesInUnion.fs:line 12
   at FSharpLint.Framework.Rules.runAstNodeRule(RuleMetadata`1 rule, AstNodeRuleParams config) in G:\Dev\FSharpLint\src\FSharpLint.Core\Framework\Rules.fs:line 100
   at Microsoft.FSharp.Collections.ArrayModule.Collect[T,TResult](FSharpFunc`2 mapping, T[] array) in D:\a\_work\1\s\src\fsharp\src\FSharp.Core\array.fs:line 151
   at FSharpLint.Application.Lint.astNodeSuggestions@168.Invoke(Tuple`2 tupledArg) in G:\Dev\FSharpLint\src\FSharpLint.Core\Application\Lint.fs:line 168
   at Microsoft.FSharp.Collections.ArrayModule.Collect[T,TResult](FSharpFunc`2 mapping, T[] array) in D:\a\_work\1\s\src\fsharp\src\FSharp.Core\array.fs:line 151
   at FSharpLint.Application.Lint.runAstNodeRules(RunAstNodeRulesConfig config) in G:\Dev\FSharpLint\src\FSharpLint.Core\Application\Lint.fs:line 166
   at FSharpLint.Application.Lint.lint(LintInfo lintInfo, FileParseInfo fileInfo) in G:\Dev\FSharpLint\src\FSharpLint.Core\Application\Lint.fs:line 261

So then it appears that what appeared to be a performance degradation is actually because the lint is running all the rules now instead of exiting part way through, and that just takes more time

[knocte]

hitting the exception described in #869 whilst

You mean the ArgumentOutOfRangeException?

completed, but has actually aborted after only running a subset of rules.

The Failed(fileInfo.File, exn) scenario doesn't end up as an error (as in exitCode!=0)???

[Numpsy]

You mean the ArgumentOutOfRangeException?

Yup

The Failed(fileInfo.File, exn) scenario doesn't end up as an error

The benchmark app is ignoring the results of the lint and uses OptionalLintParameters.Default for the lint invocation, which sets ReportLinterProgress to None which seems to result in the errors being thrown away.

[knocte]

so you're saying the ArgumentRangeOfException is happening when running benchmark tests, but not in the SelfCheck target?

[Numpsy]

I tracked the exception down to processing the union case at https://github.com/fsprojects/FSharpLint/blob/master/tests/TypeChecker.fs#L11256

I believe that test file is used for the performance tests, but won't be included in the self check.

Looking at the unit tests, I see there is a TestFixture for MaxLinesInUnion, but no actual tests (it contains TODO: Add tests.), so I've added a new test there which fails with an exception in the current master and passes on this branch.

There is perhaps more scope here to either make the benchmark app abort on an error, and/or run more tests against TypeChecker.fs

[Numpsy]

@knocte are you ok with moving stripMultilineComments and adding InternalsVisibleTo so it can be tested directly, or would you prefer to keep it where it is and just test via the rules that use it?