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.

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

Create an internal sorted equatable collection type for incremental source generators #89318

Open
layomia opened this issue Jul 21, 2023 · 9 comments
Labels
area-Extensions-Configuration source-generator Indicates an issue with a source generator feature
Milestone

Comments

@layomia
Copy link
Contributor

layomia commented Jul 21, 2023

Based on feedback from @eiriktsarpalis.

ImmutableEquatableArray<T> implements sequence equality. The JSON generator uses it in its specs to make incremental generation possible. However, spec collections often need to have set-like semantics to retain equality & avoid regeneration when there are compilation diffs that don't affect the effective inputs to the generator. IOW we want to avoid false negatives in the incremental cache.

An upcoming PR to enable incremental generation for the config binding generator (#83534) will share this type. As a follow up to that PR, we should create a new type (say ImmutableEquatableSet<T>) that guarantees set semantics by construction, following the implementation of SortedList<T>. This would avoid false negatives in the incremental cache.

@layomia layomia added area-Extensions-Configuration source-generator Indicates an issue with a source generator feature labels Jul 21, 2023
@layomia layomia added this to the 8.0.0 milestone Jul 21, 2023
@ghost
Copy link

ghost commented Jul 21, 2023

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Based on feedback from @eiriktsarpalis.

ImmutableEquatableArray<T> implements sequence equality. The JSON generator uses it in its specs to make incremental generation possible. However, the lists often need to have set-like semantics to retain equality & avoid regeneration when there are compilation diffs that don't affect the effective inputs to the generator. Currently, parsed elements (cached in intermediary data structures) are sorted before construction of the incremental lists / containing specs.

An upcoming PR to enable incremental generation for the config binding generator (#83534) will share this type, and also needs to sort elements before construction. As a follow up to that PR, we should create a new type (say ImmutableEquatableSortedList<T>) that guarantees set semantics by construction following the implementation of SortedList<T>. This would avoid intermediate lists, pre-sorting, and avoiding false negatives in the incremental cache.

Author: layomia
Assignees: -
Labels:

area-Extensions-Configuration, source-generator

Milestone: 8.0.0

@eiriktsarpalis
Copy link
Member

For simplicity we might just call it ImmutableEquatableSet<T> and I suspect we might eventually need an ImmutableEquatableDictionary too.

@Tornhoof
Copy link
Contributor

Can we get an oob nuget package with the sources of such collections as content files?

At the moment most src gens simply copy the Array type around or fail to understand that they are practically necessary for proper caching.

Why Nuget with source files as content? Binary Nugets with src gens are quite complicated to get them working properly.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jul 24, 2023

Can we get an oob nuget package with the sources of such collections as content files?

We should definitely consider something like this eventually, once the proposed new types have been tried and tested internally.

@layomia layomia modified the milestones: 8.0.0, Future Jul 25, 2023
@AaronRobinsonMSFT
Copy link
Member

@jkoritzinsky Any possibility to share with the interop source generators?

@jkoritzinsky
Copy link
Member

We have a collection today that has this behavior, SequentialEqualImmutableArray<T>. We also had one for dictionary at one point, but dropped it as we were only using it in cases where some of the types would never be equal anyway. @Sergio0694 also has an implementation with a better API surface in the .NET Community Toolkit.

@Sergio0694
Copy link
Contributor

Yeah I've been usig my own EquatableArray<T> type (here) across all of my repositories (.NET Community Toolkit, ComputeSharp, PolySharp, a whole bunch of generators I wrote for the Microsoft Store, etc.). It's effectively one of the absolute must have building blocks when writing a new source generator. I will say — I love the idea of adding something like this to the BCL (provided it has an equivalent API surface), though along with that of course we'd have to make this available through some OOB package (eg. System.Collections.Immutable?) so that you'd automatically get this as well in every source generator project as a transitive dependency from Roslyn. That would be pretty nice.

Worth noting — that's just one of the many missing APIs you have to manually polyfill every single time you're writing a generator, but this one in particular feels like something should really just be built-in, as it's also perf-critical 😅

@tarekgh tarekgh modified the milestones: Future, 9.0.0 Nov 20, 2023
@ericstj ericstj modified the milestones: 9.0.0, 10.0.0 Aug 6, 2024
@ericstj
Copy link
Member

ericstj commented Aug 6, 2024

Moving out all the incremental source gen work to 10.0

@eiriktsarpalis
Copy link
Member

FWIW I've been using equatable sets and dictionaries with success in typeshape-csharp:

https://github.com/eiriktsarpalis/typeshape-csharp/tree/6a6c68c76829fb26b8aaec42b8a3bdff02018423/src/TypeShape.Roslyn/IncrementalTypes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Extensions-Configuration source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

No branches or pull requests

8 participants