We used to ignore the language version when running nullable semantic analysis. It didn't affect whether we gave diagnostics, but now it does. An alternative to this solution would be to do the analysis, passing a dummy diagnostic bag for case (false, { NullableSemanticAnalysisEnabled: true }).

We can look into potentially giving the nullability diagnostics even if the language version is insufficient, if we ShouldRunNullableWalker for example. But I did not want to take on the test churn for it in this PR.

We used to ignore the language version when running nullable semantic analysis. It didn't affect whether we gave diagnostics, but now it does.

Can we condition those diagnostics to a warning wave instead?

To be clear, the "new" diagnostics would only be for an error scenario where a project has a pre-C# 8 language version, but also has NRT enabled using project-level settings or #nullable directives in source. It feels like we don't need to worry about gating such warnings behind a warning wave, because the project already won't compile.

We used to ignore the language version when running nullable semantic analysis. It didn't affect whether we gave diagnostics, but now it does.

I am not sure this is the behavior change that we want. It looks like this affects whether the nullability information is exposed through SemanticModel and IOperation, i.e. potentially affects behavior of analyzers and IDE features.

In reply to: 462349019 [](ancestors = 462349019)

I would suggest to avoid any observable behavior changes in this PR.

In reply to: 462651778 [](ancestors = 462651778,462349019)

It looks like we are missing a regression test that is supposed to catch this behavior change. Consider adding one.

In reply to: 462654191 [](ancestors = 462654191,462651778,462349019)

I would like to add the regression test, but I was surprised that the following test passed against a1474c7. Basically it seems like EnsureNullabilityAnalysisPerformedIfNecessary() will catch us and build out the nullability info even if we neglected to do it during binding. I should probably sync with someone who understands the nullable semantic model better to make sure I understand the situation properly.

        [Fact]
        public void CSharp73ProvidesNullableSemanticInfo()
        {
            var source = @"
class C
{
    void M(string s)
    {
        if (s == null)
        {
            s.ToString();
        }
    }
}
";
            var comp = CreateCompilation(source, options: WithNonNullTypesTrue(), parseOptions: TestOptions.Regular7_3, skipUsesIsNullable: true);
            comp.VerifyDiagnostics(
                // error CS8630: Invalid 'NullableContextOptions' value: 'Enable' for C# 7.3. Please use language version '8.0' or greater.
                Diagnostic(ErrorCode.ERR_NullableOptionNotAvailable).WithArguments("NullableContextOptions", "Enable", "7.3", "8.0").WithLocation(1, 1)
                );

            var tree = comp.SyntaxTrees.Single();
            var model = comp.GetSemanticModel(tree);

            var node = tree.GetRoot().DescendantNodes().OfType<MemberAccessExpressionSyntax>().Single();
            var info = model.GetTypeInfo(node.Expression);
            Assert.Equal(PublicNullableAnnotation.Annotated, info.Nullability.Annotation);
            Assert.Equal(PublicNullableFlowState.MaybeNull, info.Nullability.FlowState);
        }
        

I would like to add the regression test, but I was surprised that the following test passed against a1474c7. Basically it seems like EnsureNullabilityAnalysisPerformedIfNecessary() will catch us and build out the nullability info even if we neglected to do it during binding. I should probably sync with someone who understands the nullable semantic model better to make sure I understand the situation properly.

The code in method compiler doesn't affect consumers that explicitly request semantic model from a compilation. Only those that get semantic model from an analysis context in an Analyzer action are going to be affected.

In reply to: 462687379 [](ancestors = 462687379)

Did you confirm that this assert fails for implementation from iteration 2? #Closed

In the old iteration, this assert does not fail, because we never reach it. Instead an assert in MemberSemanticModel fails:

In the old iteration, this assert does not fail, because we never reach it. Instead an assert in MemberSemanticModel fails:

Could you please confirm that, if you step over that debug assert in the debugger (usually, I am able to continue after debugger breaks execution due to a debug assert while a test is being debugged), then the test (I assume at least one of these asserts) fails the "normal" x-unit way?

In reply to: 463241933 [](ancestors = 463241933)

Alternatively, you can execute release flavor of this test. Whichever you find easier.

In reply to: 463296735 [](ancestors = 463296735,463241933)

Thanks for the suggestion to run in Release mode. With that, I find that the Xunit assert failed within the AnalyzeMemberAccess method via an unexpected AD0001 diagnostic:

[xUnit.net 00:00:01.92]     Microsoft.CodeAnalysis.CSharp.UnitTests.NullablePublicAPITests.CSharp73ProvidesNullableSemanticInfo [FAIL]
  X Microsoft.CodeAnalysis.CSharp.UnitTests.NullablePublicAPITests.CSharp73ProvidesNullableSemanticInfo [863ms]
  Error Message:

Expected:
Actual:
    Diagnostic("AD0001").WithArguments("Microsoft.CodeAnalysis.CSharp.UnitTests.NullablePublicAPITests+CSharp73ProvidesNullableSemanticInfo_Analyzer", "Xunit.Sdk.EqualException", "Assert.Equal() Failure
Expected: Annotated
Actual:   None", "Exception occurred with following context:
Compilation: 638fbd72-f58d-40c4-9ddf-c62ce4a9d737
SyntaxTree:
SyntaxNode: s.ToString [MemberAccessExpressionSyntax]@[90..100) (7,12)-(7,22)

Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: Annotated
Actual:   None
   at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in C:\projects\xunit\src\xunit.assert\Asserts\EqualityAsserts.cs:line 41
   at Xunit.Assert.Equal[T](T expected, T actual) in C:\projects\xunit\src\xunit.assert\Asserts\EqualityAsserts.cs:line 25
   at Microsoft.CodeAnalysis.CSharp.UnitTests.NullablePublicAPITests.CSharp73ProvidesNullableSemanticInfo_Analyzer.AnalyzeMemberAccess(SyntaxNodeAnalysisContext context) in C:\Users\rikki\src\roslyn\src\Compilers\CSharp\Test\Symbol\Symbols\Source\NullablePublicAPITests.cs:line 1723
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__60`1.<ExecuteSyntaxNodeAction>b__60_0(ValueTuple`2 data) in C:\Users\rikki\src\roslyn\src\Compilers\Core\Portable\DiagnosticAnalyzer\AnalyzerExecutor.cs:line 880
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock[TArg](DiagnosticAnalyzer analyzer, Action`1 analyze, TArg argument, Nullable`1 info) in C:\Users\rikki\src\roslyn\src\Compilers\Core\Portable\DiagnosticAnalyzer\AnalyzerExecutor.cs:line 1569