Fixed incorrect suggestion for object initialization simplification#23395
Fixed incorrect suggestion for object initialization simplification#23395jinujoseph merged 9 commits intodotnet:masterfrom
Conversation
| ISymbol member) | ||
| { | ||
| var implementation = classOrStructType?.FindImplementationForInterfaceMember(member); | ||
| switch (implementation) |
There was a problem hiding this comment.
swithc seems like overkill here. why not just:
return implementation is IPropertySymbol property && ...| } | ||
| return implementation is IPropertySymbol property && | ||
| property.ExplicitInterfaceImplementations.Length > 0 | ||
| && property.DeclaredAccessibility == Accessibility.Private; |
There was a problem hiding this comment.
&& on previous line.
|
@dotnet/roslyn-ide Can we get a final review on this community PR? Thanks |
| public void Main() | ||
| { | ||
| IExample e = [||]new C(); | ||
| e.Name = string.Empty; |
There was a problem hiding this comment.
so the order causing one to work and not the other? would it be possible to support both if possible?
| class C | ||
| Sub Bar() | ||
| Dim c As IExample = [||]New Foo | ||
| c.LastName = String.Empty |
|
retest this please |
| var implementation = classOrStructType?.FindImplementationForInterfaceMember(member); | ||
| return implementation is IPropertySymbol property && | ||
| property.ExplicitInterfaceImplementations.Length > 0 && | ||
| property.DeclaredAccessibility == Accessibility.Private; |
There was a problem hiding this comment.
note: vb can have public implementations where the member name does not match.
| Implements IExample | ||
|
|
||
| Private Property Name As String Implements IExample.Name | ||
| Public Property LastName As String Implements IExample.LastName |
There was a problem hiding this comment.
can we have a test where these names don't match. i.e. "Public Property MyLastName Implements IExample.LastName"?
|
@zaytsev-victor could you resolve the conflict and address cyrus feedback pls. |
| class C | ||
| Sub Bar() | ||
| Dim c As IExample = New Foo With { | ||
| .LastName = String.Empty |
There was a problem hiding this comment.
this is error? can we make it to use MyLastName ?
# Conflicts: # src/Features/Core/Portable/UseObjectInitializer/ObjectCreationExpressionAnalyzer.cs
|
@CyrusNajmabadi can you review if your feedback is addressed |
|
Yup. I will take a look later. For some reason my github rules are not working and i'm not getting notifications about these directly :-/ |
| keyKeyword:=Nothing, | ||
| dotToken:=match.MemberAccessExpression.OperatorToken, | ||
| name:=DirectCast(match.MemberAccessExpression.Name, IdentifierNameSyntax), | ||
| name:=SyntaxFactory.IdentifierName(match.MemberName), |
There was a problem hiding this comment.
this likely will not work for names that are keywords. You store .ValueText which won't have hte propery escaping here.
There was a problem hiding this comment.
@CyrusNajmabadi I've tested it with Class, Property and Date keywords. It works, probably we don't need to escape names in object initializer.
| property.ExplicitInterfaceImplementations.Length > 0 && | ||
| property.DeclaredAccessibility == Accessibility.Private; | ||
|
|
||
| } |
There was a problem hiding this comment.
extra newline above this.
|
retest windows_debug_vs-integration_prtest please |
|
approved to merge for 15.8.Preview4 |
Customer scenario
User can see suggestion for object initialization simplification even if result won't compile.
Bugs this fixes:
Fixes #23368
Risk
Low
Performance impact
Low
Is this a regression from a previous update?
No