Fixed incorrect suggestion for collection initialization simplification#23721
Fixed incorrect suggestion for collection initialization simplification#23721sharwell merged 5 commits intodotnet:masterfrom
Conversation
| _syntaxFacts.GetNameAndArityOfSimpleName(memberName, out var name, out var arity); | ||
|
|
||
| if (arity != 0 || !name.Equals(nameof(IList.Add))) | ||
| if (arity != 0 || !name.Equals(nameof(IList.Add)) || IsExplicitlyImplemented(memberName)) |
There was a problem hiding this comment.
❓ Will this run for cases where the local variable has the same type as the constructed object, e.g. when using var? If so, can we special case the behavior for this to avoid expensive calls to GetSymbolInfo?
There was a problem hiding this comment.
Good point, I'll check it.
There was a problem hiding this comment.
@sharwell Should we provide suggestion in this case:
var obj = new ExpandoObject();
obj.Add("string", "v");
obj.Add("int", 1);
obj.Add("object", new { X=1, Y=2 });This code won't compile but we have suggestion to use collection initialization
There was a problem hiding this comment.
I'm OK with either way in that scenario, so whatever's more efficient.
|
|
||
| protected override bool ShouldAnalize() | ||
| => _semanticModel.GetTypeInfo(_objectCreationExpression, default).Type | ||
| .GetMembers(WellKnownMemberNames.CollectionInitializerAddMethodName) |
There was a problem hiding this comment.
.Typemay return null, so this would crash on GetMembers.- Do not pass default for the cancellation token. We should have an actual cancellation token that can be passsed.
There was a problem hiding this comment.
- This won't pick up inherited .Add methods, or extension .Add methods.
| return null; | ||
| } | ||
|
|
||
| if (!ShouldAnalize()) |
|
test windows_debug_spanish_unit32_prtest |
| .Any(x => x is IMethodSymbol m && m.DeclaredAccessibility == Accessibility.Public); | ||
| protected override bool ShouldAnalyze() | ||
| { | ||
| var type = _semanticModel.GetTypeInfo(_objectCreationExpression, _cancellationToken).Type; |
There was a problem hiding this comment.
📝 This should not cause a performance problem because this expression was already being used on this this code path:
|
@jinujoseph for approval to merge |
|
Approved to merge for 15.8.Preview2 |
Fixes #23672
Ask Mode template not completed
Customer scenario
What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)
Bugs this fixes
(either VSO or GitHub links)
Workarounds, if any
Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW
Risk
This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code
Performance impact
(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")
Is this a regression from a previous update?
Root cause analysis
How did we miss it? What tests are we adding to guard against it in the future?
How was the bug found?
(E.g. customer reported it vs. ad hoc testing)
Test documentation updated?
If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.