Allow to forward obsolete types without any diagnostics.#62127
Allow to forward obsolete types without any diagnostics.#62127AlekseyTs merged 6 commits intodotnet:mainfrom
Conversation
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 2), a couple of small comments.
| diagnostics.Free(); | ||
| return lazyAttributesStoredOnThisThread; | ||
|
|
||
| void removeObsoleteDiagnosticsForForwardeTypes(ImmutableArray<CSharpAttributeData> boundAttributes, ImmutableArray<AttributeSyntax> attributesToBind, ref BindingDiagnosticBag diagnostics) |
There was a problem hiding this comment.
| void removeObsoleteDiagnosticsForForwardeTypes(ImmutableArray<CSharpAttributeData> boundAttributes, ImmutableArray<AttributeSyntax> attributesToBind, ref BindingDiagnosticBag diagnostics) | |
| void removeObsoleteDiagnosticsForForwardedTypes(ImmutableArray<CSharpAttributeData> boundAttributes, ImmutableArray<AttributeSyntax> attributesToBind, ref BindingDiagnosticBag diagnostics) | |
| ``` #Resolved |
| diagnostics.Free(); | ||
| return lazyAttributesStoredOnThisThread; | ||
|
|
||
| void removeObsoleteDiagnosticsForForwardeTypes(ImmutableArray<CSharpAttributeData> boundAttributes, ImmutableArray<AttributeSyntax> attributesToBind, ref BindingDiagnosticBag diagnostics) |
There was a problem hiding this comment.
Worth asserting that attributesToBind and boundAttributes are the same length? #Resolved
There was a problem hiding this comment.
I think it is "obvious" from the code above, but I will add one since I need to fix a typo in this code anyway.
| } | ||
| } | ||
|
|
||
| static bool isObsoleteDiagnostics(DiagnosticWithInfo d) |
There was a problem hiding this comment.
| static bool isObsoleteDiagnostics(DiagnosticWithInfo d) | |
| static bool isObsoleteDiagnostic(DiagnosticWithInfo d) | |
| ``` #Resolved |
|
@dotnet/roslyn-compiler For the second review. |
|
@dotnet/roslyn-compiler For the second review. |
| var boundAttribute = boundAttributeArray[i]; | ||
| Debug.Assert(boundAttribute is not null); | ||
| NullableWalker.AnalyzeIfNeeded(binders[i], boundAttribute, boundAttribute.Syntax, diagnostics.DiagnosticBag); | ||
| NullableWalker.AnalyzeIfNeeded(binders[i], boundAttribute, boundAttribute.Syntax, diagnostics.DiagnosticBag!); |
There was a problem hiding this comment.
Why is the suppression necessary? We have an assertion already at the top of the method: Debug.Assert(diagnostics.DiagnosticBag is not null);
Same question below (line 430) #Closed
There was a problem hiding this comment.
Why is the suppression necessary? We have an assertion already at the top of the method
We started passing it by ref, I guess.
|
|
||
| // The general strategy: | ||
| // 1. Collect locations of the first argument of each TypeForwardedTo attribute application. | ||
| // 2. Collect obsolete diagnostics reported withing the span of those locations. |
| // The general strategy: | ||
| // 1. Collect locations of the first argument of each TypeForwardedTo attribute application. | ||
| // 2. Collect obsolete diagnostics reported withing the span of those locations. | ||
| // 3. Remove the collected diagnostics, if any. |
There was a problem hiding this comment.
nit: two empty lines below #Closed
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 5)
|
Thank you!!! 😁 |
|
I didn't articulate this in my proposal above, but this boils down to:
And now we can do both 😁 |
Closes #61264.