Add nullable annotation to the parameter types of equality operators#40838
Add nullable annotation to the parameter types of equality operators#40838y87feng merged 8 commits intodotnet:masterfrom
Conversation
|
|
||
| public static bool operator ==(C? left, C? right) | ||
| { | ||
| return global::System.Collections.Generic.EqualityComparer<global::N.C>.Default.Equals(left, right); |
There was a problem hiding this comment.
we should not be generated global:: here.
There was a problem hiding this comment.
This is a separate issue. @y87feng do you mind filing? I can help you file as needed.
There was a problem hiding this comment.
not certain how it's a different issue :) isn't this caused because of the new usage of nullable types heres?
There was a problem hiding this comment.
I don't think it is because of the new usage of nullable types:) I can check if there is an issue already
There was a problem hiding this comment.
I'm curious what woudl cause it then. None of hte other tests exhibit htis issue.
There was a problem hiding this comment.
The behavior existed before this change. It might be related to enabling nullable/using csharp 8, but was not directly caused by this change.
| [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateEqualsAndGetHashCode)] | ||
| public async Task TestEqualsNullableAnnotation() | ||
| { | ||
| await TestWithPickMembersDialogAsync( |
There was a problem hiding this comment.
does this need to go through the 'pick members dialog' test path? this doesn't feel dialog specific.
There was a problem hiding this comment.
This code indicates that I cannot test "generate operators" without having a dialog, then I think "pick members dialog" is necessary
There was a problem hiding this comment.
you could definitely update that code :) just add a default generateOperators to http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/GenerateEqualsAndGetHashCodeFromMembers/GenerateEqualsAndGetHashCodeFromMembersCodeRefactoringProvider.cs,239 and pass that along. No tests need to change, but you can now pass generateOperators: true from your test.
There was a problem hiding this comment.
What benefit is there in changing the current behavior? Current code design is that a dialog is required to allow generation of operators. If we change the code, that distinction becomes harder to follow. Testing with a mocked out dialog is not an issue
There was a problem hiding this comment.
@CyrusNajmabadi this seems to be the only outstanding issue. I'm approving as I don't see this blocking, but want to wait for your feedback before merging.
| CodeGenerationSymbolFactory.CreateParameterSymbol(_containingType, LeftName), | ||
| CodeGenerationSymbolFactory.CreateParameterSymbol(_containingType, RightName)); | ||
| CodeGenerationSymbolFactory.CreateParameterSymbol(_containingType.WithNullableAnnotation(NullableAnnotation.Annotated), LeftName), | ||
| CodeGenerationSymbolFactory.CreateParameterSymbol(_containingType.WithNullableAnnotation(NullableAnnotation.Annotated), RightName)); |
There was a problem hiding this comment.
this warrants a comment here as to waht we're doing.
|
|
||
| [WorkItem(40053, "https://github.com/dotnet/roslyn/issues/40053")] | ||
| [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateEqualsAndGetHashCode)] | ||
| public async Task TestEqualsNullableAnnotation() |
There was a problem hiding this comment.
One case we need to make sure is tested is generating operators on value types, which should not annotated the same as reference types.
|
|
||
| var generator = _document.GetLanguageService<SyntaxGenerator>(); | ||
|
|
||
| // add nullable annotation to the parameter reference type, so the value could be null |
There was a problem hiding this comment.
This comment is telling us what the code is doing, but not really why.
Needs explanation related to the fact that (in)equality operator implementations for reference types need to allow comparison against null.
ryzngard
left a comment
There was a problem hiding this comment.
Small improvement to be made on comment. Otherwise LGTM. Please wait for @CyrusNajmabadi follow up on issue before merging.
Add nullable annotation to the parameter types of equality operators #40053