-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix loss of nullability attributes when getting the 'constructed reduced from' method. #79400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d9050c9
f7f73f6
3a207a3
17eecd4
790baf3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -170,13 +170,13 @@ public override TypeSymbol ReceiverType | |
| } | ||
| } | ||
|
|
||
| public override TypeSymbol GetTypeInferredDuringReduction(TypeParameterSymbol reducedFromTypeParameter) | ||
| public override TypeWithAnnotations GetTypeInferredDuringReduction(TypeParameterSymbol reducedFromTypeParameter) | ||
| { | ||
| // This will throw if API shouldn't be supported or there is a problem with the argument. | ||
| var notUsed = OriginalDefinition.GetTypeInferredDuringReduction(reducedFromTypeParameter); | ||
|
|
||
| Debug.Assert((object)notUsed == null && (object)OriginalDefinition.ReducedFrom != null); | ||
| return this.TypeArgumentsWithAnnotations[reducedFromTypeParameter.Ordinal].Type; | ||
| Debug.Assert(notUsed.Type is null && OriginalDefinition.ReducedFrom is not null); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: GetTypeInferredDuringReduction calls into itself on the OriginalDef prop. So 'notUsed' now became a TypeWithAnnotations as well, requiring this check to change. |
||
| return this.TypeArgumentsWithAnnotations[reducedFromTypeParameter.Ordinal]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the crux of the fix. by returning 'Type' we were losing the This then broke an ide scenario because it looked like the compiler was saying all the type arguments before/after a change we were making were identical, when really it was losing info. |
||
| } | ||
|
|
||
| public sealed override MethodSymbol ReducedFrom | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6048,5 +6048,62 @@ class B<T, U, U> | |
| symbol = model.GetDeclaredSymbol(typeParameters[typeParameters.Length - 2]); | ||
| Assert.True(symbol.IsReferenceType); | ||
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/75570")] | ||
| public void GetConstructedReducedFrom_WithNullableInferredType() | ||
| { | ||
| var source = """ | ||
| #nullable enable | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Linq; | ||
|
|
||
| class C1 { } | ||
|
|
||
| class C2 { } | ||
|
|
||
| class D | ||
| { | ||
| public static void ConvertAll(IEnumerable<C1> values) | ||
| { | ||
| var v = values.Select(Convert); | ||
| } | ||
|
|
||
| [return: NotNullIfNotNull(nameof(value))] | ||
| private static C2? Convert(C1? value) => value is null ? null : new C2(); | ||
| } | ||
| """ + NotNullIfNotNullAttributeDefinition; | ||
|
|
||
| var comp = CreateCompilationWithMscorlib40AndSystemCore(source).VerifyDiagnostics(); | ||
|
|
||
| var tree = comp.SyntaxTrees[0]; | ||
| var model = comp.GetSemanticModel(tree); | ||
| var memberAccess = tree.GetRoot().DescendantNodes().OfType<MemberAccessExpressionSyntax>().First(); | ||
| var constructedMethodSymbol = (IMethodSymbol)model.GetSymbolInfo(memberAccess).Symbol; | ||
|
|
||
| // Ensure that both the bound reduced method symbol, and the constructed method symbol | ||
| // it was reduced from both have the right nullable annotations for the type parameters. | ||
| Assert.True(constructedMethodSymbol is | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be a bit simpler and easier to evaluate a difference, if a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personally like hitting the symbols directly :) |
||
| { | ||
| Name: nameof(Enumerable.Select), | ||
| TypeArguments: [ | ||
| { Name: "C1", NullableAnnotation: Microsoft.CodeAnalysis.NullableAnnotation.NotAnnotated }, | ||
| { Name: "C2", NullableAnnotation: Microsoft.CodeAnalysis.NullableAnnotation.Annotated } | ||
| ], | ||
| ReceiverType.Name: nameof(IEnumerable<>), | ||
| }); | ||
|
|
||
| var constructedReducedFrom = constructedMethodSymbol.GetConstructedReducedFrom(); | ||
| Assert.True(constructedReducedFrom is | ||
| { | ||
| Name: nameof(Enumerable.Select), | ||
| TypeArguments: [ | ||
| { Name: "C1", NullableAnnotation: Microsoft.CodeAnalysis.NullableAnnotation.NotAnnotated }, | ||
| { Name: "C2", NullableAnnotation: Microsoft.CodeAnalysis.NullableAnnotation.Annotated } | ||
| ], | ||
| ReceiverType.Name: nameof(Enumerable), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this related to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's removing dead code in the IDE in this feature. We don't use this pattern.