Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Collection expressions: synthesize single-element list type for IEnumerable<T>, IReadOnlyCollection<T>, IReadOnlyList<T> #71147

Merged
merged 29 commits into from
Jan 25, 2024
Merged

Collection expressions: synthesize single-element list type for IEnumerable<T>, IReadOnlyCollection<T>, IReadOnlyList<T> #71147

merged 29 commits into from
Jan 25, 2024

Conversation

alrz
Copy link
Member

@alrz alrz commented Dec 7, 2023 •
edited
Loading

Contributes to #68785

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 7, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 7, 2023
@alrz
Copy link
Member Author

alrz commented Dec 7, 2023

@cston I'd like a review pass on this before I add tests and check well-known members. The enumerator should probably be a nested type.. is there a sample I can follow to make it so? or it's ok for it to be top-level?

@CyrusNajmabadi
Copy link
Member

Awesome!!

@CyrusNajmabadi
Copy link
Member

@alrz will this cause conflicts with the work @jnm2 is doing in: #71110 ?

Just want to make sure you both are not colliding at the same time.

@cston
Copy link
Member

cston commented Dec 7, 2023 •
edited
Loading

Thanks @alrz, the changes so far LGTM!

The enumerator should probably be a nested type.. is there a sample I can follow to make it so? or it's ok for it to be top-level?

I agree, the enumerator type should probably be a nested type. I don't know if we have an example of a nested synthesized type, but perhaps the following changes are needed:

  • Use the list type as the enumerator type ContainingSymbol
  • Include the enumerator type in the list type GetTypeMembers()
  • Adjust enumerator type Arity and TypeParameters since the containing type is generic
  • Update PrivateImplementationDetails.GetTopLevelTypeMethods() to return the methods from type.GetNestedTypes(context) for each top-level type

@alrz
Copy link
Member Author

alrz commented Dec 8, 2023

@CyrusNajmabadi

will this cause conflicts with the work @jnm2 is doing in: #71110 ?

thanks for the heads up. I haven't started checking well-known members yet so it won't. I will sync once that PR goes in.

btw, would this interact with the planned yield-like codegen (from #68785)?

also I was wondering what would it take to remove SpecializedCollections; maybe synthesize empty types as well?

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Dec 8, 2023 •
edited
Loading

btw, would this interact with the planned yield-like codegen

Most likely yes :)

also I was wondering what would it take to remove SpecializedCollections; maybe synthesize empty types as well?

I would be fine with synthesizing empty singleton types. :)

But, frankly, we likely should just use the known Array.Empty singleton for these instead.

@alrz
Misc
8d16231
@alrz
Merge remote-tracking branch 'origin/main' into singleton
2ebc77a 
# Conflicts:
#	src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
#	src/Compilers/CSharp/Portable/Symbols/Synthesized/ReadOnlyListType/SynthesizedReadOnlyListTypeSymbol.cs
@alrz
Additional members
2de4a5e
@alrz
Misc
51f63a1
@alrz
Add test
9b80f87
@alrz alrz marked this pull request as ready for review December 12, 2023 21:27
@alrz alrz requested a review from a team as a code owner December 12, 2023 21:27
@alrz
Copy link
Member Author

alrz commented Dec 12, 2023

But, frankly, we likely should just use the known Array.Empty singleton for these instead.

I'm assuming there's a reason that both roslyn/runtime have a special type for this. Maybe saving a tiny bit of memory?
There's actually Enumerable.Empty<T>() for enumerables but not for ReadonlyList or ReadonlyCollection.

@CyrusNajmabadi
Copy link
Member

I'm assuming there's a reason that both roslyn/runtime have a special type for this.

Having one actual object exist and be pointed to by literally millions of locations in memory is good :)

@alrz alrz requested a review from cston December 15, 2023 11:00
@alrz
Copy link
Member Author

alrz commented Dec 15, 2023

@cston @dotnet/roslyn-compiler for review, thanks.

@cston cston requested review from RikkiGibson and a team December 21, 2023 20:00
@cston cston requested a review from a team December 28, 2023 17:56
@cston
Copy link
Member

cston commented Jan 2, 2024

@dotnet/roslyn-compiler for a second review, thanks.

1 similar comment
@cston
Copy link
Member

cston commented Jan 16, 2024

@dotnet/roslyn-compiler for a second review, thanks.

@cston
Copy link
Member

cston commented Jan 23, 2024

@333fred, @RikkiGibson, @dotnet/roslyn-compiler, for a second review, thanks.

333fred
333fred reviewed Jan 23, 2024
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done review pass

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
@@ -308,32 +308,36 @@ SpecialType.System_Collections_Generic_IReadOnlyCollection_T or
int numberIncludingLastSpread;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't name this variable, but... number of what? We need to rename this for clarity. If you want to do it now, go for it, if not we can do it in a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried lastSpreadIndex here 57dc299 but that's more than a rename. I'll leave it out of this PR.

...rs/CSharp/Portable/Symbols/Synthesized/ReadOnlyListType/SynthesizedReadOnlyListTypeSymbol.cs Outdated Show resolved Hide resolved
alrz
alrz commented Jan 25, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
// arrayOrList = Array.Empty<ElementType>();
arrayOrList = CreateEmptyArray(syntax, ArrayTypeSymbol.CreateSZArray(_compilation.Assembly, elementType));
}
else
{
var typeArgs = ImmutableArray.Create(elementType);
var synthesizedType = _factory.ModuleBuilderOpt.EnsureReadOnlyListTypeExists(syntax, hasKnownLength: useKnownLength, _diagnostics.DiagnosticBag).Construct(typeArgs);
var kind = useKnownLength
? numberIncludingLastSpread == 0 && elements.Length == 1 && SynthesizedReadOnlyListTypeSymbol.CanCreateSingleElement(_compilation)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CanCreateSingleElement

This runs for every single-element collection expr unlike SynthesizedReadOnlyListTypeSymbol.Create. Not sure if this matters but I think we can detect this in EnsureReadOnlyListTypeExists and downgrade instead of checking each time, although that would only optimize the happy path.

@alrz alrz changed the title Collection expressions: synthesize singleton list type for IEnumerable<T>, IReadOnlyCollection<T>, IReadOnlyList<T> Collection expressions: synthesize single-element list type for IEnumerable<T>, IReadOnlyCollection<T>, IReadOnlyList<T> Jan 25, 2024
@333fred 333fred enabled auto-merge (squash) January 25, 2024 18:34
@333fred 333fred merged commit e6f52d1 into dotnet:main Jan 25, 2024
25 checks passed
@ghost ghost added this to the Next milestone Jan 25, 2024
@cston
Copy link
Member

cston commented Jan 25, 2024

Thanks @alrz!

@Cosifne Cosifne modified the milestones: Next, 17.10 P1 Jan 30, 2024
@Cosifne Cosifne mentioned this pull request Feb 2, 2024
Closed
@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Apr 6, 2024
Merged
@eerhardt eerhardt mentioned this pull request May 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred approved these changes

@cston cston cston approved these changes

@RikkiGibson RikkiGibson Awaiting requested review from RikkiGibson

Assignees
No one assigned
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.10 P1
Development

Successfully merging this pull request may close these issues.
5 participants
@alrz @CyrusNajmabadi @cston @333fred @Cosifne