Fix MemberNames API for records #49138
Conversation
| @@ -1080,7 +1080,7 @@ public override IEnumerable<string> MemberNames | |||
| { | |||
| get | |||
| { | |||
| return IsTupleType ? GetMembers().Select(m => m.Name).Distinct() : this.declaration.MemberNames; | |||
| return (IsTupleType || IsRecord) ? GetMembers().Select(m => m.Name).Distinct() : this.declaration.MemberNames; | |||
There was a problem hiding this comment.
📝 I'm debating whether we should add caching of MemberNames in SourceMemberContainerSymbol at this point... Thoughts?
There was a problem hiding this comment.
I'm debating whether we should add caching of MemberNames in SourceMemberContainerSymbol at this point... Thoughts?
I would prefer that... but it's not necessarily a huge deal if you don't (unlesss people have tons of records).
The purpose of these collectoins is to allow the IDE to search for names very efficiently (i.e. in a non-allocating fashion). So it is important that the vast majority of source types not allocate here. And, as long as that holds we'll be ok.
But it would be a bit nicer to cache IMO in case you had someone with, say, 1k generated records, and we kept generaitng this over and over again.
Note: You don't need to do Distinct. MemberNames is explicitly doc'ed as returning dupes. exactly because we don't want to do the CPU/memory work to find and remove duplicates for htese sensitive scenarios.
There was a problem hiding this comment.
MemberNames is explicitly doc'ed as returning dupes
Where do you see that?
The implementations I looked at (such as source named type symbols) seem to explicitly de-dupe.
There was a problem hiding this comment.
public ICollection<string> MemberNames
{
get
{
if (_lazyMemberNames == null)
{
var names = UnionCollection<string>.Create(this.Declarations, d => d.MemberNames);
Interlocked.CompareExchange(ref _lazyMemberNames, names, null);
}
return _lazyMemberNames;
}
}The merged decl for named types (i.e. for partials) just literally 'combines' (i.e. concatenates) all the names of all the individual parts. SourceMemberContainer just does this:
public override IEnumerable<string> MemberNames
{
get
{
return IsTupleType ? GetMembers().Select(m => m.Name).Distinct() : this.declaration.MemberNames;
}
}
So this will return dupes. We do not do any sort of 'distinct' as we want this not to cost anything in terms of memory/CPU. This was very intentional from teh impl. Source: i wrote it :)
There was a problem hiding this comment.
@CyrusNajmabadi Thanks! Removed the .Distinct :-)
|
@dotnet/roslyn-compiler for a second review. Thanks |
| "<Clone>$", | ||
| "Deconstruct" | ||
| }; | ||
| AssertEx.Equal(expectedMemberNames, comp.GetMember<NamedTypeSymbol>("C").GetPublicSymbol().MemberNames); |
There was a problem hiding this comment.
is order guaranteed here? should this test sort, just to be sfe against that?
There was a problem hiding this comment.
We probably could relax the order checking if we feel the need.
ghost
added this to the 
* upstream/master: (519 commits) Remove workaround in PEMethodSymbol.ExplicitInterfaceImplementations (dotnet#49246) Enable LSP pull model diagnostic for XAML. (dotnet#49145) Update dependencies from https://github.com/dotnet/roslyn build 20201109.8 (dotnet#49240) Add test for with expression in F1 help service (dotnet#49236) Cache RegexPatternDetector per compilation Fix RazorRemoteHostClient to maintain binary back-compat Further tweak inline hints Fix MemberNames API for records (dotnet#49138) Minor cleanups (dotnet#49204) Report warning for assignment or explicit cast of possibly null value of unconstrained type parameter type (dotnet#48803) Clean up of EditorFeatures.Cocoa.Snippets (dotnet#49188) Fix OK button state handling. Make relation between viewmodels more tightly coupled Extend make type abstract to include records (dotnet#48227) Remove duplicated implementations of C# event hookup Add select all/deselect all buttons Consolidate conditional compilation (dotnet#49150) Implement IEquatable on Microsoft.Cci.DefinitionWithLocation structure (dotnet#49162) Add new document extensions file Unify implementations Only disable structure tagger provider in LSP scenario ...
Fixes #48947