Share common code involved into building dictionaries of members for namespace symbols #67933

AlekseyTs · 2023-04-23T02:47:19Z

This a multi-step refactoring, might be easier to review commit by commit. Each commit has a title describing the purpose of each step.

…bleArrayExtensions.AddToMultiValueDictionaryBuilder helper.

…eToMembersMap

…eric helpers

…ayExtensions and use from both compilers

AlekseyTs · 2023-04-23T04:07:41Z

@CyrusNajmabadi FYI

AlekseyTs · 2023-04-24T15:12:09Z

@dotnet/roslyn-compiler Please review

CyrusNajmabadi · 2023-04-24T17:01:23Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

+                if (existingValueOrArray is ArrayBuilder<T> arrayBuilder)
+                {
+                    // Already a builder in the accumulator, just add to that.
+                }
+                else
+                {
+                    // Just a single value in the accumulator so far.  Convert to using a builder.
+                    arrayBuilder = ArrayBuilder<T>.GetInstance(capacity: 2);


Suggested change

if (existingValueOrArray is ArrayBuilder<T> arrayBuilder)

{

// Already a builder in the accumulator, just add to that.

}

else

{

// Just a single value in the accumulator so far. Convert to using a builder.

arrayBuilder = ArrayBuilder<T>.GetInstance(capacity: 2);

if (existingValueOrArray is not ArrayBuilder<T> arrayBuilder)

{

// Just a single value in the accumulator so far. Convert to using a builder.

arrayBuilder = ArrayBuilder<T>.GetInstance(capacity: 2);

``` #WontFix

I am not interested in changing style or or inverting if conditions. Do not see any value in that.

the value is not needing a superfluous empty does-nothing body. That makes reviewing more difficult as there are more codepaths to reason about.

That makes reviewing more difficult as there are more codepaths to reason about.

I disagree with that. In my opinion restructuring this code will make it harder to review this particular PR, in my opinion. And my goal is to make it as easy to review as possible.

it was harder for me to reason about. i had to actually convince myself of what was going on because of the lack of use of better and clearer patterns. It's fine if that's not a concern of yours, but it's def a concern of mine (especially as someone who has had to work with this code a lot recently.

CyrusNajmabadi · 2023-04-24T17:01:57Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

+            where TNamedTypeSymbol : class, TNamespaceOrTypeSymbol
+            where TNamespaceSymbol : class, TNamespaceOrTypeSymbol


Suggested change

where TNamedTypeSymbol : class, TNamespaceOrTypeSymbol

where TNamespaceSymbol : class, TNamespaceOrTypeSymbol

where TNamedTypeSymbol : class, TNamespaceOrTypeSymbol

where TNamespaceSymbol : class, TNamespaceOrTypeSymbol

are the class constraints necessary here if this depend on a type parameter already with that constraint? #Resolved

are the class constraints necessary here if this depend on a type parameter already with that constraint?

Yes, interfaces satisfy class constraint. An interface as a constraint doesn't mean class.

my question is if restating the class constraint is necessary, or if that is picked up by the transitive type parameter constraint here.

my question is if restating the class constraint is necessary, or if that is picked up by the transitive type parameter constraint here.

And I answered this question, I think. With an explanation why a class constraint is not transitive.

CyrusNajmabadi · 2023-04-24T17:02:36Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

+                var builder = value as ArrayBuilder<TNamespaceOrTypeSymbol>;
+                if (builder != null)


Suggested change

var builder = value as ArrayBuilder<TNamespaceOrTypeSymbol>;

if (builder != null)

if (value in ArrayBuilder<TNamespaceOrTypeSymbol> builder)

``` #WontFix

I am not interested in changing style. Also, style changes are not welcome.

it's not a style change. it's reduction in concept count. THere's no need for as + null + variable. A single concept does all of that in a clearer and less error prone fashion.

It is a style change in my opinion. I see no value in the suggested change. If it is not broken, don't fix it.

CyrusNajmabadi · 2023-04-24T17:03:18Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

+                            hasNamespaces = true;
+                            break;
+                        }
+                    }


could just be var hasNamespaces = builder.Any(static n => n is TNamespaceSymbol); #WontFix

I am not interested in rewriting code that works.

CyrusNajmabadi · 2023-04-24T17:03:59Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

+                        ? builder.ToImmutable()
+                        : ImmutableArray<TNamespaceOrTypeSymbol>.CastUp(builder.ToDowncastedImmutable<TNamedTypeSymbol>());
+
+                    builder.Free();


calling ToImmutable followed by Free dosen't allow for the optimization of moving the internal array to the immutable array without a copy that ToImmutableAndFree allows for. #Resolved

calling ToImmutable followed by Free dosen't allow for the optimization of moving the internal array to the immutable array without a copy that ToImmutableAndFree allows for.

This is an "extract common code" refactoring, I am not interested in changing what the code is doing and how it does that.

i'm pointing out that the code you're extracting out has issues with it. Since you are working with the code now, it's a good time to actually address the problems in it. Otherwise, it's just kicking hte can down the road, and will require more effort and more reviews to take care of these things. best to just do it when our eyes are on this problem already. I trust that it's very simple to change this and get the code to remove problematic parts, and we can review in equally low amounts of time. Putting it in a separate commit is fine as well as it will make it easy to review.

i'm pointing out that the code you're extracting out has issues with it.

This is definitely something to consider as a future improvement, I will open a follow-up issue.

Since you are working with the code now, it's a good time to actually address the problems in it.

I do not agree with that. I prefer to separate the concerns.

I will open a follow-up issue.

#67990

CyrusNajmabadi · 2023-04-24T17:06:36Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

+                    }
+                    else
+                    {
+                        dictionary.Add(kvp.Key, members.As<TNamedTypeSymbol>());


you can skip the above loop and all the logic simply by doing an initial O(1) members.As<TNamedTypeSymbol>(). If that succeeds, you know it is all named types and can use it. If it fails (i.e. you get a .IsDefault array bac), it means the client constructing map saw a non-type in it. This avoid this code having to go redo the scan that was already done when map was built. #Resolved

you can skip the above loop and all the logic simply by doing an initial O(1) members.As().

This is definitely something to consider as a future improvement, I will open a follow-up issue. This PR is an "extract common code" refactoring, I am not interested in changing what the code is doing and how it does that.

I will open a follow-up issue

#67991

AlekseyTs · 2023-04-25T14:17:31Z

@dotnet/roslyn-compiler Please review

cston · 2023-04-25T16:57:08Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

+                {
+                    Debug.Assert(builder.Count > 1);
+                    bool hasNamespaces = false;
+                    for (int i = 0; (i < builder.Count) && !hasNamespaces; i++)


&& !hasNamespaces

Is this check needed? #Resolved

Is this check needed?

This function is an existing code moved from C# compiler. It is not my goal to change what it is doing

It looks like the body of the for loop changed from NameToSymbolMapBuilder.CreateMap() so the check here is no longer needed.

It looks like the body of the for loop changed from NameToSymbolMapBuilder.CreateMap() so the check here is no longer needed.

That is fair

AlekseyTs · 2023-04-25T18:06:54Z

@dotnet/roslyn-compiler For the second review

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

AlekseyTs added 7 commits April 22, 2023 19:43

Get rid of NameToSymbolMapBuilder structure, extract and reuse Immuta…

64c5cda

…bleArrayExtensions.AddToMultiValueDictionaryBuilder helper.

AddToMultiValueDictionaryBuilder - extract common code out of if`

32823f9

Switch from kind to type check in GetTypesFromMemberMap and CreateNam…

67186f4

…eToMembersMap

Convert GetTypesFromMemberMap and CreateNameToMembersMap to fully gen…

b743314

…eric helpers

Move CreateNameToMembersMap and GetTypesFromMemberMap to ImmutableArr…

ae5eb60

…ayExtensions and use from both compilers

Inline/move helpers

6e8d6bb

Inline StaticCast.From helper.

77b4431

AlekseyTs added the Area-Compilers label Apr 23, 2023

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 23, 2023

AlekseyTs marked this pull request as ready for review April 23, 2023 04:06

AlekseyTs requested a review from a team as a code owner April 23, 2023 04:06

CyrusNajmabadi reviewed Apr 24, 2023

View reviewed changes

cston reviewed Apr 25, 2023

View reviewed changes

cston approved these changes Apr 25, 2023

View reviewed changes

333fred approved these changes Apr 25, 2023

View reviewed changes

AlekseyTs commented Apr 26, 2023

View reviewed changes

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs Outdated Show resolved Hide resolved

PR feedback

e437baf

AlekseyTs enabled auto-merge (squash) April 26, 2023 13:36

AlekseyTs merged commit 7c23c2e into dotnet:main Apr 26, 2023

ghost added this to the Next milestone Apr 26, 2023

allisonchou mentioned this pull request May 2, 2023

[Automated] PRs inserted in VS build main-33702.166 #68059

Closed

Cosifne modified the milestones: Next, 17.7 P2 May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share common code involved into building dictionaries of members for namespace symbols #67933

Share common code involved into building dictionaries of members for namespace symbols #67933

AlekseyTs commented Apr 23, 2023

AlekseyTs commented Apr 23, 2023

AlekseyTs commented Apr 24, 2023

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2023

CyrusNajmabadi Apr 24, 2023

AlekseyTs Apr 24, 2023

CyrusNajmabadi Apr 24, 2023

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2023

CyrusNajmabadi Apr 24, 2023

AlekseyTs Apr 24, 2023

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2023

CyrusNajmabadi Apr 24, 2023

AlekseyTs Apr 24, 2023

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2023

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2023

CyrusNajmabadi Apr 24, 2023 •

edited

Loading

AlekseyTs Apr 24, 2023

AlekseyTs Apr 26, 2023

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2023

AlekseyTs Apr 26, 2023

AlekseyTs commented Apr 25, 2023

cston Apr 25, 2023 •

edited by AlekseyTs

Loading

AlekseyTs Apr 25, 2023

cston Apr 25, 2023

AlekseyTs Apr 25, 2023

AlekseyTs commented Apr 25, 2023

		where TNamedTypeSymbol : class, TNamespaceOrTypeSymbol
		where TNamespaceSymbol : class, TNamespaceOrTypeSymbol

		var builder = value as ArrayBuilder<TNamespaceOrTypeSymbol>;
		if (builder != null)

Share common code involved into building dictionaries of members for namespace symbols #67933

Share common code involved into building dictionaries of members for namespace symbols #67933

Conversation

AlekseyTs commented Apr 23, 2023

AlekseyTs commented Apr 23, 2023

AlekseyTs commented Apr 24, 2023

CyrusNajmabadi Apr 24, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 24, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 24, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 24, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 24, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 24, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 25, 2023

cston Apr 25, 2023 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 25, 2023

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

CyrusNajmabadi Apr 24, 2023 •

edited

Loading

CyrusNajmabadi Apr 24, 2023 •

edited by AlekseyTs

Loading

cston Apr 25, 2023 •

edited by AlekseyTs

Loading