Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 34 additions & 21 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,13 @@ private void SetAnalyzedNullability(BoundExpression? expression, TypeWithState r
/// </summary>
private void SetAnalyzedNullability(BoundExpression? expr, VisitResult result, bool? isLvalue = null)
{
if (expr == null || _disableNullabilityAnalysis) return;
if (expr == null
// BoundExpressionWithNullability is not produced by the binder but is used within nullability analysis to pass information to internal components.
|| expr.Kind == BoundKind.ExpressionWithNullability
Copy link
Member Author

@RikkiGibson RikkiGibson Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I tried to just add this kind to s_skippedExpressions. But that just controls whether the debug visitor will visit that kind. Essentially it just states an expectation that NullableWalker won't visit something, when, what we want here is the opposite, to tolerate that NullableWalker will visit something which the debug verifier won't visit. Possibly I misunderstood something, though, and this isn't the best place to make this change, or an appropriate type to use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically being done so that we can cook up a receiver expression for the property pattern access, and let the VisitArguments machinery handle the reinference of the extension, conversion of property receiver to extension parameter type, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my question is: should we actually be peaking through the BoundExpressionWithNullability here and setting the underlying expression's state? Feels like we may miss something if we don't?

Copy link
Member Author

@RikkiGibson RikkiGibson Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is there is no underlying expression in the bound tree for an extension property pattern. I just know what the receiver type is and would like to thread it through, having all the extension reinference and conversion checks from VisitArguments just work.

|| _disableNullabilityAnalysis)
{
return;
}

#if DEBUG
// https://github.com/dotnet/roslyn/issues/34993: This assert is essential for ensuring that we aren't
Expand Down Expand Up @@ -3901,7 +3907,7 @@ void visitElement(BoundNode element, BoundCollectionExpression node, TypeWithAnn
// Reinfer the addMethod signature and convert the argument to parameter type
var addMethod = initializer.AddMethod;
MethodSymbol reinferredAddMethod;
if (!addMethod.IsExtensionMethod)
if (!addMethod.IsExtensionMethod && !addMethod.GetIsNewExtensionMember())
{
reinferredAddMethod = (MethodSymbol)AsMemberOfType(targetCollectionType, addMethod);
}
Expand Down Expand Up @@ -4406,7 +4412,8 @@ void setAnalyzedNullabilityAsContinuation(
{
var symbol = objectInitializer.MemberSymbol;

if (symbol != null)
// https://github.com/dotnet/roslyn/issues/78828: adjust extension member based on new containing type
if (symbol != null && !symbol.GetIsNewExtensionMember())
{
Debug.Assert(TypeSymbol.Equals(objectInitializer.Type, GetTypeOrReturnType(symbol), TypeCompareKind.IgnoreNullableModifiersForReferenceTypes));
symbol = AsMemberOfType(containingType, symbol);
Expand Down Expand Up @@ -4611,7 +4618,7 @@ static MethodSymbol addMethodAsMemberOfContainingType(BoundCollectionElementInit
argumentResults = builder.ToImmutableAndFree();
}
}
else
else if (!method.GetIsNewExtensionMember())
{
// Tracked by https://github.com/dotnet/roslyn/issues/78828: Do we need to do anything special for new extensions here?
method = (MethodSymbol)AsMemberOfType(containingType, method);
Expand Down Expand Up @@ -7098,7 +7105,8 @@ private static bool HasImplicitTypeArguments(BoundNode node)
or BoundForEachStatement
or BoundPropertyAccess
or BoundIncrementOperator
or BoundCompoundAssignmentOperator)
or BoundCompoundAssignmentOperator
or BoundDagPropertyEvaluation)
{
return true;
}
Expand Down Expand Up @@ -8656,8 +8664,7 @@ private TypeWithState GetAdjustedResult(TypeWithState type, int slot)
private static Symbol AsMemberOfType(TypeSymbol? type, Symbol symbol)
{
Debug.Assert((object)symbol != null);
// https://github.com/dotnet/roslyn/issues/78828: This method should not be used with new extension members.
//Debug.Assert(!symbol.GetIsNewExtensionMember());
Debug.Assert(!symbol.GetIsNewExtensionMember(), symbol.ToDisplayString());

var containingType = type as NamedTypeSymbol;
if (containingType is null || containingType.IsErrorType() || symbol is ErrorMethodSymbol)
Expand Down Expand Up @@ -11326,6 +11333,25 @@ private TypeWithAnnotations GetDeclaredParameterResult(ParameterSymbol parameter
return null;
}

private (PropertySymbol updatedProperty, bool returnNotNull) ReInferAndVisitExtensionPropertyAccess(BoundNode node, PropertySymbol property, BoundExpression receiver)
{
Debug.Assert(property.GetIsNewExtensionMember());
ImmutableArray<BoundExpression> arguments = [receiver];

var extensionParameter = property.ContainingType.ExtensionParameter;
Debug.Assert(extensionParameter is not null);
ImmutableArray<ParameterSymbol> parameters = [extensionParameter];

ImmutableArray<RefKind> refKindsOpt = extensionParameter.RefKind == RefKind.Ref ? [RefKind.Ref] : default;

// Tracked by https://github.com/dotnet/roslyn/issues/37238 : properties/indexers should account for NotNullIfNotNull
var (updatedProperty, _, returnNotNull) = VisitArguments(node, arguments, refKindsOpt, parameters, default, defaultArguments: default,
expanded: false, invokedAsExtensionMethod: false, property, firstArgumentResult: null);

Debug.Assert(updatedProperty is not null);
return (updatedProperty, returnNotNull);
}

public override BoundNode? VisitPropertyAccess(BoundPropertyAccess node)
{
var property = node.PropertySymbol;
Expand All @@ -11334,20 +11360,7 @@ private TypeWithAnnotations GetDeclaredParameterResult(ParameterSymbol parameter
if (property.GetIsNewExtensionMember())
{
Debug.Assert(node.ReceiverOpt is not null);
ImmutableArray<BoundExpression> arguments = [node.ReceiverOpt];

var extensionParameter = property.ContainingType.ExtensionParameter;
Debug.Assert(extensionParameter is not null);
ImmutableArray<ParameterSymbol> parameters = [extensionParameter];

ImmutableArray<RefKind> refKindsOpt = extensionParameter.RefKind == RefKind.Ref ? [RefKind.Ref] : default;

// Tracked by https://github.com/dotnet/roslyn/issues/37238 : properties/indexers should account for NotNullIfNotNull
bool returnNotNull;
(updatedProperty, _, returnNotNull) = VisitArguments(node, arguments, refKindsOpt, parameters, default, defaultArguments: default,
expanded: false, invokedAsExtensionMethod: false, property, firstArgumentResult: null);

Debug.Assert(updatedProperty is not null);
(updatedProperty, _) = ReInferAndVisitExtensionPropertyAccess(node, property, node.ReceiverOpt);

TypeWithAnnotations typeWithAnnotations = GetTypeOrReturnTypeWithAnnotations(updatedProperty);
FlowAnalysisAnnotations memberAnnotations = GetRValueAnnotations(updatedProperty);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,9 @@ public PossiblyConditionalState Clone()
case BoundDagPropertyEvaluation e:
{
Debug.Assert(inputSlot > 0);
var property = (PropertySymbol)AsMemberOfType(inputType, e.Property); // Tracked by https://github.com/dotnet/roslyn/issues/78828 : this needs to handle extension properties
var property = e.Property.GetIsNewExtensionMember()
? ReInferAndVisitExtensionPropertyAccess(e, e.Property, new BoundExpressionWithNullability(e.Syntax, expression, NullableAnnotation.NotAnnotated, inputType)).updatedProperty
: (PropertySymbol)AsMemberOfType(inputType, e.Property);
var type = property.TypeWithAnnotations;
var output = new BoundDagTemp(e.Syntax, type.Type, e);
int outputSlot = GetOrCreateSlot(property, inputSlot, forceSlotEvenIfEmpty: true);
Expand Down
7 changes: 5 additions & 2 deletions src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27761,7 +27761,7 @@ static class E
Diagnostic(ErrorCode.ERR_NameofExtensionMember, "c.Property.Property").WithLocation(2, 29));
}

[Fact(Skip = "Assertion in NullableWalker.AsMemberOfType")] // Tracked by https://github.com/dotnet/roslyn/issues/78828 : Nullability analysis of properties
[Fact]
public void Nameof_Instance_Property_Generic_01()
{
var src = """
Expand All @@ -27779,7 +27779,10 @@ static class E
}
""";
var comp = CreateCompilation(src);
CompileAndVerify(comp, expectedOutput: "Property").VerifyDiagnostics();
comp.VerifyEmitDiagnostics(
// (2,29): error CS9316: Extension members are not allowed as an argument to 'nameof'.
// System.Console.Write(nameof(i.Property));
Diagnostic(ErrorCode.ERR_NameofExtensionMember, "i.Property").WithLocation(2, 29));

var tree = comp.SyntaxTrees.Single();
var model = comp.GetSemanticModel(tree);
Expand Down
181 changes: 172 additions & 9 deletions src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs
Original file line number Diff line number Diff line change
Expand Up @@ -386,15 +386,8 @@ public static class E
}
}
""";
try
{
var comp = CreateCompilation([src, OverloadResolutionPriorityAttributeDefinition]);
// Tracked by https://github.com/dotnet/roslyn/issues/78828 : assertion in NullableWalker
CompileAndVerify(comp, expectedOutput: "42").VerifyDiagnostics();
}
catch (InvalidOperationException)
{
}
var comp = CreateCompilation([src, OverloadResolutionPriorityAttributeDefinition]);
CompileAndVerify(comp, expectedOutput: "42").VerifyDiagnostics();
}

[Fact]
Expand Down Expand Up @@ -2282,6 +2275,176 @@ class C
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x2").WithLocation(7, 5));
}

[Fact]
public void Nullability_PropertyPattern_Reinference_01()
{
var source = """
#nullable enable
using System.Collections.Generic;

class Program
{
void M1(bool b)
{
var item = "a";
if (b)
{
item = null;
}

var list = M2(item)/*T:System.Collections.Generic.List<string?>!*/;
if (list is { First: var first })
{
first.ToString(); // 1
}
}

List<T> M2<T>(T item) => [item];
}

static class ListExtensions
{
extension<T>(List<T> list)
{
public T First => list[0];
}
}
""";

var comp = CreateCompilation(source);
comp.VerifyTypes();
comp.VerifyEmitDiagnostics(
// (17,13): warning CS8602: Dereference of a possibly null reference.
// first.ToString(); // 1
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "first").WithLocation(17, 13));
}

[Fact]
public void Nullability_PropertyPattern_Reinference_02()
{
var source = """
#nullable enable
using System.Collections.Generic;

class Program
{
void M1(bool b)
{
var item = "a";
if (b)
{
item = null;
}

var list = M2(item)/*T:System.Collections.Generic.List<string?>!*/;
if (list is { First: var first }) // 1
{
first.ToString();
}
}

List<T> M2<T>(T item) => [item];
}

static class ListExtensions
{
extension(List<string> list)
{
public string First => list[0];
}
}
""";

var comp = CreateCompilation(source);
comp.VerifyTypes();
comp.VerifyEmitDiagnostics(
// (15,23): warning CS8620: Argument of type 'List<string?>' cannot be used for parameter 'list' of type 'List<string>' in 'extension(List<string>)' due to differences in the nullability of reference types.
// if (list is { First: var first }) // 1
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "First").WithArguments("System.Collections.Generic.List<string?>", "System.Collections.Generic.List<string>", "list", "extension(List<string>)").WithLocation(15, 23));
}

[Fact]
public void Nullability_PropertyPattern_Reinference_03()
{
var source = """
#nullable enable
using System.Collections.Generic;

class Program
{
void M1(bool b)
{
var item = "a";
if (b)
{
item = null;
}

var list = M2(item)/*T:System.Collections.Generic.List<string?>!*/;
if (list is { First: var first }) // 1
{
first.ToString(); // 2
}
}

List<T> M2<T>(T item) => [item];
}

static class ListExtensions
{
extension<T>(List<T> list) where T : class
{
public T First => list[0];
}
}
""";

// Tracked by https://github.com/dotnet/roslyn/issues/78830 : diagnostic quality consider reporting a better containing symbol
var comp = CreateCompilation(source);
comp.VerifyTypes();
comp.VerifyEmitDiagnostics(
// (15,23): warning CS8634: The type 'string?' cannot be used as type parameter 'T' in the generic type or method 'ListExtensions.extension<T>(List<T>)'. Nullability of type argument 'string?' doesn't match 'class' constraint.
// if (list is { First: var first }) // 1
Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterReferenceTypeConstraint, "First").WithArguments("ListExtensions.extension<T>(System.Collections.Generic.List<T>)", "T", "string?").WithLocation(15, 23),
// (17,13): warning CS8602: Dereference of a possibly null reference.
// first.ToString(); // 2
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "first").WithLocation(17, 13));
}

[Fact]
public void Nullability_PropertyPattern_Postcondition_01()
{
var source = """
#nullable enable
using System.Diagnostics.CodeAnalysis;

class Program
{
void M(object? obj)
{
if (obj is { AsNotNull: var notNull })
notNull.ToString();
}
}

static class Extensions
{
extension(object? obj)
{
[NotNull]
public object? AsNotNull => obj!;
}
}
""";

var comp = CreateCompilation([source, NotNullAttributeDefinition]);
// Tracked by https://github.com/dotnet/roslyn/issues/78828 : should we extend member post-conditions to work with extension members?
comp.VerifyEmitDiagnostics(
// (9,13): warning CS8602: Dereference of a possibly null reference.
// notNull.ToString();
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "notNull").WithLocation(9, 13));
}

[Fact]
public void WellKnownAttribute_Conditional()
{
Expand Down