Check NotNullIfNotNull in implementation

[RikkiGibson]

Closes #44539

VS validation: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/274873

[RikkiGibson]

This introduces multiple new warnings to the bootstrap build. I think the warnings are.. fair? Interesting.

• src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs(183,13): error CS8825: Return value must be non-null because parameter 'node' is non-null. [src/Compilers/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.csproj]

roslyn/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

Lines 174 to 184 in 3a976eb

	[return: NotNullIfNotNull("node")]
	private BoundExpression? VisitExpression(BoundExpression? node)
	{
	if (node == null)
	{
	return node;
	}
	Debug.Assert(!node.HasErrors, "nodes with errors should not be lowered");
	return VisitExpressionImpl(node);
	}

• src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxRewriter.cs(46,17): error CS8825: Return value must be non-null because parameter 'node' is non-null. [src/Compilers/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.csproj]

roslyn/src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxRewriter.cs

Lines 35 to 52 in 3a976eb

	[return: NotNullIfNotNull("node")]
	public override SyntaxNode? Visit(SyntaxNode? node)
	{
	if (node != null)
	{
	_recursionDepth++;
	StackGuard.EnsureSufficientExecutionStack(_recursionDepth);
	var result = ((CSharpSyntaxNode)node).Accept(this);
	_recursionDepth--;
	return result;
	}
	else
	{
	return null;
	}
	}

[jcouv]

The local rewriter warning should be fixed by annotating VisitExpressionImpl with NotNullIfNotNull.
The CSharpSyntaxRewriter.cs one needs a bit more validation. Can we rewrite to null? If not, then an assertion would do the trick.

[RikkiGibson]

The local rewriter warning should be fixed by annotating VisitExpressionImpl with NotNullIfNotNull.

I think this might not be right, because VisitExpressionImpl's parameter has a non-null type, and adding the attribute introduces a different warning:

src\Compilers\CSharp\Portable\Lowering\LocalRewriter\LocalRewriter.cs(232,13): warning CS8825: Return value must be non-null because parameter 'node' is non-null.

I think something about the Debug.Assert is giving the visited a maybe-null state afterwards. Perhaps the pure null check?

[RikkiGibson]

I have opened a tracking issue #47682 and have suppressed the new warnings, since "proper" fixes for the warnings seem non-trivial.

[RikkiGibson]

@dotnet/roslyn-compiler Please review

[jaredpar]

Why didn't u use GetFlowAnalysisAnnotations here?

[RikkiGibson]

This code was moved from parameterHasBadState below. However GetFlowAnalysisAnnotations(Symbol) here would just dispatch out to ParameterSymbol.FlowAnalysisAnnotations.

It could make sense to change this to GetParameterAnnotations in order to avoid attribute binding cycles. Since I can't come up with a crashing scenario for this off the top of my head I would like to punt changes to it to #47635

[jaredpar]

Please add a few cases for constructors where the only code in the method is a :this call to another constructor and verify that we validate not-null correctly in those cases.

[RikkiGibson]

I wasn't sure exactly what this meant--do you want tests where constructor parameters have [NotNull] and [NotNullIfNotNull]?

[RikkiGibson]

Changing base so we can get this change into 16.8.