Update SA1008 to not crash if there is no previous token

[bjornhellander]

Fixes #2354

Protected all similar methods in LocationHelpers against null reference exception.
I found one situation where this happens in SA1008 so I added a unit test for that as well.

I also went through all uses of the SyntaxTree properties in SyntaxToken, SyntaxTrivia and SyntaxNodeOrToken and tried to make sure that they could not cause a null reference exceptions.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (be49652) 97.40% compared to head (68c49ff) 94.45%.
Report is 6 commits behind head on master.

❗ Current head 68c49ff differs from pull request most recent head 6b7973e. Consider uploading reports for the commit 6b7973e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3741      +/-   ##
==========================================
- Coverage   97.40%   94.45%   -2.95%     
==========================================
  Files         914      908       -6     
  Lines      108762   108717      -45     
  Branches     3284     3285       +1     
==========================================
- Hits       105937   102688    -3249     
- Misses       1847     5003    +3156     
- Partials      978     1026      +48     

[bjornhellander]

Ready for review

[sharwell]

I would prefer to not move in this direction. If token.SyntaxTree can be null somewhere, we should check for null at the location where a possible-null is originally introduced.

[bjornhellander]

Alright. I fear it will be hard to fix "all" cases that way, but we will see what I can do. SA1008 should be easy anyway.

[bjornhellander]

Updated to only handle SA1008.

[sharwell]

📝 This won't correctly handle the following top-level program:

 ( var a, var b) = (3, 3);
System.Console.WriteLine($"{a}");

[bjornhellander]

Good catch! Didn't think of that.
Alright, I will have a look.

[bjornhellander]

I see now that you added both a leading and a trailing space in your example. What was your intention with the leading space? What is the expected result?

[sharwell]

It should behave the same way as this:

void Method()
{
     ( var a, var b) = (3, 3);
    System.Console.WriteLine($"{a}");
}

[bjornhellander]

As far as I can understand, this is a "don't care" case, since it is the first token on the line

[bjornhellander]

Test execution seems to fail without a reason in the previous build. I noticed that both OpenCover and xunit.runner.console are quite old (from 2016 and 2018). What do you think about updating them?

[sharwell]

Test execution seems to fail without a reason in the previous build.

I think this is a different issue that I can likely work around in the build system.

[bjornhellander]

Test execution seems to fail without a reason in the previous build.

I think this is a different issue that I can likely work around in the build system.

Absolutely, I didn't mean in this PR. It looked like the test runner just died before reporting the outcome in the c# 6 debug run and in the c# 12 debug run, all tests passed but opencover.console.exe exited with code 1 without saying why. Just looked weird, so upgrading them might be a good thing. Unless you already had ideas about it.