fix(array): make the Array.length null exception containing the param…

[natalie-o-perret]

Not sure this is going against the backward compatibility policy, but the argument wasn't part of the exception thrown by Array.length in the case of a null array.

[dnfadmin]

All CLA requirements met.

[abelbraaksma]

To add to the issue: all Array.xxx function throw an ArgumentNullException, except for Array.length. As part of the mentorship program we stumbled upon this little issue and decided to use it as a "first PR" kinda effort, while realizing that it'll change the exception from the current NullReferenceException.

In typical F# code neither exception is very common, so if this is considered a backward compat issue, it's a small one. But for orthogonality, it seemed reasonable to align the exceptions here.

[KevinRansom]

@kerry-perret
I believe it's okay to change the exception here. We will improve the specificity and actionability of exceptions by emitting a better exception when the opportunity arises.

Thanks

kevin

[KevinRansom]

The failure is here: https://github.com/dotnet/fsharp/blob/ec5bad3a391357e03ff2286a264f0e4faf7d840d/tests/fsharpqa/Source/Optimizations/ForLoop/ZeroToArrLength02.fs

I think it just needs the baseline (.bsl) updating. I imagine it now generates the check on length.

[natalie-o-perret]

@KevinRansom yes it's been noticed and that's what we also suspected.

[Expandable EDIT] re-pasting my previous investigation (previous revision of this comment), trying to figure out why perl failed to run the tests

The failure is here: https://github.com/dotnet/fsharp/blob/ec5bad3a391357e03ff2286a264f0e4faf7d840d/tests/fsharpqa/Source/Optimizations/ForLoop/ZeroToArrLength02.fs

I think it just needs the baseline (.bsl) updating. I imagine it now generates the check on length.

So speaking of which, is there a recipe about the proper way to run fsi run.fsharpqa.test.fsx?

I'm trying to wrap my head around the appropriate way to setup perl, here is my journey down that rabbit hole.

It all started when I have seen that according to code in the fsi run.fsharpqa.test.fsx, the Strawberry Perl version is quite specific and relies on a version stored in the nuget cache:

FileName = Path.Combine(nugetCache, "StrawberryPerl", "5.28.0.1", "bin", "perl.exe"),

Which I suspected was from that particular nuget package https://www.nuget.org/packages/StrawberryPerl

Then I needed to set up the env var according to the fsharp\tests\fsharpqa\readme.md: set `

TEST_UPDATE_BSL=1

At this stage, fsi run.fsharpqa.test.fsx leads to another error stating that Win32::Process was missing, so I installed the missing cpan package:

C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa>C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\StrawberryPerl.5.28.0.1\bin\cpan.bat install Win32::Process
Loading internal logger. Log::Log4perl recommended for better logging
CPAN: CPAN::SQLite loaded ok (v0.211)
CPAN: LWP::UserAgent loaded ok (v6.34)
Fetching with LWP:
http://cpan.strawberryperl.com/authors/01mailrc.txt.gz
CPAN: YAML::XS loaded ok (v0.70)
Fetching with LWP:
http://cpan.strawberryperl.com/modules/02packages.details.txt.gz
Fetching with LWP:
http://cpan.strawberryperl.com/modules/03modlist.data.gz
Creating database file ...
Done!
CPAN: Module::CoreList loaded ok (v5.20180622)
Win32::Process is up to date (0.16).
and changed the path accordingly to the StrawberryPerl nuget package *.exe, aka in my case: fsharp\tests\fsharpqa\StrawberryPerl.5.28.0.1\bin\perl.exe

but then bumped into something else about a missing failures.lst file:

C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa>"C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\Common7\IDE\CommonExtensions\Microsoft\FSharp\fsi.exe" run.fsharpqa.test.fsx
Runall.pl line 900:  Fatal Error: No such file or directory
Could not open fail list 'failures.lst'.
C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\..\..\tests\fsharpqa\testenv\bin\runall.pl terminated abnormally.
Can't use an undefined value as a symbol reference at C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\..\..\tests\fsharpqa\testenv\bin\runall.pl line 1002.
System.Exception: exit code: 2
   at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1639.Invoke(String message) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\printf.fs:line 1639
   at FSI_0001.runPerl(String[] arguments) in C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\run.fsharpqa.test.fsx:line 62
   at <StartupCode$FSI_0001>.$FSI_0001.main@() in C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\run.fsharpqa.test.fsx:line 65
Stopped due to error

So I checked this issue: #6593

and created an empty failures.lst file in fsharp\tests\TestResults, but again:

C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa>"C:\Program Files (x86)\Microsoft Visual Studio\2
019\Professional\Common7\IDE\CommonExtensions\Microsoft\FSharp\fsi.exe" run.fsharpqa.test.fsx                                                                                                   
Runall.pl line 2572:  Parsing error in C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\source\Com
pilerOptions\fsc\determinism\env.lst, with '    SOURCE=dummy.fs                            PRECMD="\$FSC_PIPE du
                                        && \$FSI_PIPE copyArtifacts.fsx" POSTCMD="\$FSI_PIPE --nologo --quiet --
exec binaryCompare.fsx false"': invalid space character in usage                                                
C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\..\..\tests\fsharpqa\testenv\bin\runall.pl termin
ated abnormally.                                                                                                
System.Exception: exit code: 3                                                                                  
   at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1639.Invoke(String message) in F:\workspace
\_work\1\s\src\fsharp\FSharp.Core\printf.fs:line 1639                                                           
   at FSI_0001.runPerl(String[] arguments) in C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\run
.fsharpqa.test.fsx:line 62                                                                                      
   at <StartupCode$FSI_0001>.$FSI_0001.main@() in C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa
\run.fsharpqa.test.fsx:line 65                                                                                  
Stopped due to error     

I feel I'm heading in the wrong direction 🤸‍♀️

Will try with the steps detailed here https://github.com/dotnet/fsharp/blob/main/TESTGUIDE.md#fsharpqa-suite

[KevinRansom]

@kerry-perret

The way I run fsharpqa is:

build -testfsharpqa -c release

That will run all of the tests. When I want to narrow it down, I just comment out all of the tests except the directory I want to focus on in this file here: https://github.com/dotnet/fsharp/blob/main/tests/fsharpqa/Source/test.lst

I have no idea about : run.fsharpqa.test.fsx , it looks like it was added by some community members who found it helpful.

[cartermp]

FYI, @vzarytovskii is quite close with migrating the bulk of these tests to just be plain 'ole xUnit tests. Sometime soon it should be a lot more pleasant to update these!

[cartermp]

Thanks, this is a good change. Technically a breaking change since someone could be expecting the current exception behavior...but seriously, nobody is actually doing that. Approved subject to tests passing!

[KevinRansom]

@cartermp , indeed he is, life is about to get a whole hep better for us.

[natalie-o-perret]

@kerry-perret

The way I run fsharpqa is:

build -testfsharpqa -c release

That will run all of the tests. When I want to narrow it down, I just comment out all of the tests except the directory I want to focus on in this file here: https://github.com/dotnet/fsharp/blob/main/tests/fsharpqa/Source/test.lst

I have no idea about : run.fsharpqa.test.fsx , it looks like it was added by some community members who found it helpful.

It's actually my bad I initially just rushed trying things till I bumped into the relevant bit of documentation.

[KevinRansom]

How I would handle this is:

1. build -c release -testfsharpqa
2. It takes about 10 minutes to run to completion
3. Ensure it fails with 1 failure
4. cd c:\kevinransom\fsharp\tests\fsharpqa\Source\Optimizations\ForLoop
   5 Compare ZeroToArrLength02.il ZeroToArrLength02.il.bsl
   6 Verify that the substantial difference in il is due to the length comparison
   it should be noted that il generation always causes some differences in il, the comparer we use is expected to ignore those.
5. If you are satisfied that the change in generated il is warranted, then update the baseline by copying ZeroToArrLength02.il to ZeroToArrLength02.il.bsl
6. rerun the tests to ensure they now pass, commit and push the changes.

I hope this helps.

Kevin

[natalie-o-perret]

I am fairly aware of what should be done once the tests have managed to run, the thing is that I'm still facing that issue with perl down the line:

[...]

3038 tests passed (99.97%)
1 tests failed (0.03%)
0 tests were cascaded failures (0.00%)
0 tests returned "no result" (0.00%)
0 tests timed out (0.00%)
0 tests had test errors (0.00%)
========
3039 Total executed tests

16 tests were skipped
========
3055 Total tests (executed & skipped)

19m 38s Total time
Command failed to execute with exit code 1: C:\Users\Michelle\.nuget\packages\\StrawberryPerl\5.28.0.1\bin\perl.exe "C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\testenv\bin\runall.pl" -resultsroot "C:\Users\Michelle\Desktop\Personal\Repos\fsharp\artifacts\TestResults\release" -results test-net40-fsharpqa-results.log -log test-net40-fsharpqa-errors.log -fail test-net40-fsharpqa-errors -cleanup:no -procs:8
System.Management.Automation.RuntimeException: Command failed to execute with exit code 1: C:\Users\Michelle\.nuget\packages\\StrawberryPerl\5.28.0.1\bin\perl.exe "C:\Users\Michelle\Desktop\Personal\Repos\fsharp\tests\fsharpqa\testenv\bin\runall.pl" -resultsroot "C:\Users\Michelle\Desktop\Personal\Repos\fsharp\artifacts\TestResults\release" -results test-net40-fsharpqa-results.log -log test-net40-fsharpqa-errors.log -fail test-net40-fsharpqa-errors -cleanup:no -procs:8
at Exec-CommandCore, C:\Users\Michelle\Desktop\Personal\Repos\fsharp\eng\build-utils.ps1: line 50
at Exec-Console, C:\Users\Michelle\Desktop\Personal\Repos\fsharp\eng\build-utils.ps1: line 119
at <ScriptBlock>, C:\Users\Michelle\Desktop\Personal\Repos\fsharp\eng\build.ps1: line 525
at <ScriptBlock>, <No file>: line 1

which echoes what I bumped into in a first place #10337 (comment)

[abelbraaksma]

What might help, and what we didn't try yesterday night, is to install Perl using the official ActiveState Perl installer. When I did it the first time around, I didn't use Chocolatey like you did last time, but used the ActiveState download and installer directly. Which suggests it would set the INC path correctly for the libs needed here.

You might try that (but perhaps first uninstall and remove from path the current one you have).

[natalie-o-perret]

I'm gonna push the updated ZeroToArrLength02.il.bsl.

I just have some concerns about the perl thingy 🤷‍♀️

[natalie-o-perret]

@abelbraaksma

True, I think I tried a few other perl installs ydy before gtb, but can definitely take a whack at installing ActivePerl instead of the other flavours.

🤞

[abelbraaksma]

It's the one that's mentioned in the dev guide, so theoretically it ought to be the right one ;).

(but if you didn't change the path env variable, you'd still be running the older installs, make sure to update that)

[natalie-o-perret]

@abelbraaksma thanks
Correct me if I'm wrong, but I went thru both:

• TESTGUIDE
• DEVGUIDE

And seems that there is no specific mention of the "Active" edition, shall we add it to the TESTGUIDE.md?

(I even went with the search on the whole repo, just in case I was missing out something)

[EDIT]
Seems the all the tests are now passing without any specific install of perl whatsoever 🧙‍♀️ (just the Strawberry edition fetched via the nuget packages) 🤸‍♀️.

Running the tests one more time, just to make sure I'm not hallucinating 🩹, so I can finally wrap it up.

3039 tests passed (100.00%)
0 tests failed (0.00%)
0 tests returned "no result" (0.00%)
0 tests timed out (0.00%)
0 tests had test errors (0.00%)
========
3039 Total executed tests

16 tests were skipped
========
3055 Total tests (executed & skipped)

17m 19s Total time

[abelbraaksma]

Test guide says this:

The FSharpQA suite relies on Perl, StrawberryPerl package from nuget is used automatically by the test suite.

Though I would have sworn it used to be ActiveState. I guess it's now downloaded automatically, like you wrote. My memory is a bit stale it seems ;). Glad it works now!

[KevinRansom]

@kerry-perret , so the issue you ran into was a previously installed perl? and the fix was to remove them, is that the conclusion?

[natalie-o-perret]

@kerry-perret , so the issue you ran into was a previously installed perl? and the fix was to remove them, is that the conclusion?

The issue was two-folded:

• Previous manual installation of perl
• Made things even worse by manually adding the Strawberry Perl edition as part of the nuget global package cache and added the Win32::Process module

It all started cause we were trying to run fsi run.fsharpqa.test.fsx without the rest of the build pipeline which I believe (I might to need have a close look at the whole process), contains some important steps to properly setup Strawberry Perl via nuget prior to run the tests.

[abelbraaksma]

Maybe we should update the Readme md file on fsharpqa, as that's where the instructions were to run that script to regenerate the bsl IL files.

[KevinRansom]

I'm interested that installing a perl messes with our use of the strawberry perl runner, since we are specifying the full path to the strawberry perl runner, I wonder what is going wrong?

[natalie-o-perret]

I'm interested that installing a perl messes with our use of the strawberry perl runner, since we are specifying the full path to the strawberry perl runner, I wonder what is going wrong?

I think the issue lies more in fact that I was running fsi run.fsharpqa.test.fsx, and looking at the file content:

    use perlProcess =
        ProcessStartInfo(
            FileName = Path.Combine(nugetCache, "StrawberryPerl", "5.28.0.1", "bin", "perl.exe"),
            Arguments = (arguments |> Array.map(fun a -> @"""" + a + @"""") |> String.concat " "),
            WorkingDirectory = Path.Combine(rootFolder, "tests", "fsharpqa", "source"),
            RedirectStandardOutput = true,
            RedirectStandardError = true,
            UseShellExecute = false
        )
        |> Process.Start

    while (not perlProcess.StandardOutput.EndOfStream) do
        perlProcess.StandardOutput.ReadLine() |> printfn "%s" 
    while (not perlProcess.StandardError.EndOfStream) do
        perlProcess.StandardError.ReadLine() |> printfn "%s" 
    perlProcess.WaitForExit()
    if perlProcess.ExitCode <> 0 then
        failwithf "exit code: %i" perlProcess.ExitCode

let testResultDir = Path.Combine(rootFolder, "tests", "TestResults")
let perlScript = Path.Combine(rootFolder, "tests", "fsharpqa", "testenv", "bin", "runall.pl")
runPerl [|perlScript; "-resultsroot";testResultDir ;"-ttags:Determinism"|]

Except that the fsx script alone is not gonna add the relevant package to the nuget cache, but it is still required, to run the script as expected.

The manual installation of Perl, which defaulted to C:\Perl, didn't match the hardcoded path of Perl in the fsx file:

FileName = Path.Combine(nugetCache, "StrawberryPerl", "5.28.0.1", "bin", "perl.exe"),

What was surprising is more when I tried to add myself the Strawberry Perl to the global nuget cache, it seems there was some missing modules. That's the reason why I installed the Win32::Process module but it still didn't work, there was probably some other missing modules.

[KevinRansom]

Thanks for this contribution

Kevin