-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Make config binding gen incremental #89587
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
4f6f853
Make config binding gen incremental
layomia 0859bec
Iterate on implementation
layomia 82318e7
Add incremental tests & driver
layomia ff62dd4
Make incremental tests pass and revert functional regression
layomia 80c668f
Address failing tests
layomia 321d65a
Make tests pass
layomia 23abc0d
Suppress diagnostic
layomia 90be738
Address feedback on diag info creation
layomia 1b11a80
Refactor member access expr parsing to indicate assumptions
layomia 10f2579
Address feedback & do misc clean up
layomia de2e0e5
Adjust model to minimize baseline diff / misc clean up
layomia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
60 changes: 60 additions & 0 deletions
60
src/libraries/Common/src/SourceGenerators/DiagnosticInfo.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Linq; | ||
using System.Numerics.Hashing; | ||
using Microsoft.CodeAnalysis; | ||
|
||
namespace SourceGenerators; | ||
|
||
/// <summary> | ||
/// Descriptor for diagnostic instances using structural equality comparison. | ||
/// Provides a work-around for /~https://github.com/dotnet/roslyn/issues/68291. | ||
/// </summary> | ||
internal readonly struct DiagnosticInfo : IEquatable<DiagnosticInfo> | ||
{ | ||
public DiagnosticDescriptor Descriptor { get; private init; } | ||
public object?[] MessageArgs { get; private init; } | ||
public Location? Location { get; private init; } | ||
|
||
public static DiagnosticInfo Create(DiagnosticDescriptor descriptor, Location? location, object?[]? messageArgs) | ||
{ | ||
Location? trimmedLocation = location is null ? null : GetTrimmedLocation(location); | ||
|
||
return new DiagnosticInfo | ||
{ | ||
Descriptor = descriptor, | ||
Location = trimmedLocation, | ||
MessageArgs = messageArgs ?? Array.Empty<object?>() | ||
}; | ||
|
||
// Creates a copy of the Location instance that does not capture a reference to Compilation. | ||
static Location GetTrimmedLocation(Location location) | ||
=> Location.Create(location.SourceTree?.FilePath ?? "", location.SourceSpan, location.GetLineSpan().Span); | ||
} | ||
|
||
public Diagnostic CreateDiagnostic() | ||
=> Diagnostic.Create(Descriptor, Location, MessageArgs); | ||
|
||
public override readonly bool Equals(object? obj) => obj is DiagnosticInfo info && Equals(info); | ||
|
||
public readonly bool Equals(DiagnosticInfo other) | ||
{ | ||
return Descriptor.Equals(other.Descriptor) && | ||
MessageArgs.SequenceEqual(other.MessageArgs) && | ||
Location == other.Location; | ||
} | ||
|
||
public override readonly int GetHashCode() | ||
{ | ||
int hashCode = Descriptor.GetHashCode(); | ||
foreach (object? messageArg in MessageArgs) | ||
{ | ||
hashCode = HashHelpers.Combine(hashCode, messageArg?.GetHashCode() ?? 0); | ||
} | ||
|
||
hashCode = HashHelpers.Combine(hashCode, Location?.GetHashCode() ?? 0); | ||
return hashCode; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 changes: 2 additions & 1 deletion
3
...ies/System.Text.Json/gen/Model/TypeRef.cs → ...es/Common/src/SourceGenerators/TypeRef.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
84 changes: 84 additions & 0 deletions
84
src/libraries/Common/tests/SourceGenerators/GeneratorTestHelpers.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Collections; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Reflection; | ||
using Xunit; | ||
|
||
namespace SourceGenerators.Tests | ||
{ | ||
internal static class GeneratorTestHelpers | ||
{ | ||
/// <summary> | ||
/// Asserts for structural equality, returning a path to the mismatching data when not equal. | ||
/// </summary> | ||
public static void AssertStructurallyEqual<T>(T expected, T actual) | ||
{ | ||
CheckAreEqualCore(expected, actual, new()); | ||
static void CheckAreEqualCore(object expected, object actual, Stack<string> path) | ||
{ | ||
if (expected is null || actual is null) | ||
{ | ||
if (expected is not null || actual is not null) | ||
{ | ||
FailNotEqual(); | ||
} | ||
|
||
return; | ||
} | ||
|
||
Type type = expected.GetType(); | ||
if (type != actual.GetType()) | ||
{ | ||
FailNotEqual(); | ||
return; | ||
} | ||
|
||
if (expected is IEnumerable leftCollection) | ||
{ | ||
if (actual is not IEnumerable rightCollection) | ||
{ | ||
FailNotEqual(); | ||
return; | ||
} | ||
|
||
object?[] expectedValues = leftCollection.Cast<object?>().ToArray(); | ||
object?[] actualValues = rightCollection.Cast<object?>().ToArray(); | ||
|
||
for (int i = 0; i < Math.Max(expectedValues.Length, actualValues.Length); i++) | ||
{ | ||
object? expectedElement = i < expectedValues.Length ? expectedValues[i] : "<end of collection>"; | ||
object? actualElement = i < actualValues.Length ? actualValues[i] : "<end of collection>"; | ||
|
||
path.Push($"[{i}]"); | ||
CheckAreEqualCore(expectedElement, actualElement, path); | ||
path.Pop(); | ||
} | ||
} | ||
|
||
if (type.GetProperty("EqualityContract", BindingFlags.Instance | BindingFlags.NonPublic, null, returnType: typeof(Type), types: Array.Empty<Type>(), null) != null) | ||
{ | ||
// Type is a C# record, run pointwise equality comparison. | ||
foreach (PropertyInfo property in type.GetProperties(BindingFlags.Public | BindingFlags.Instance)) | ||
{ | ||
path.Push("." + property.Name); | ||
CheckAreEqualCore(property.GetValue(expected), property.GetValue(actual), path); | ||
path.Pop(); | ||
} | ||
|
||
return; | ||
} | ||
|
||
if (!expected.Equals(actual)) | ||
{ | ||
FailNotEqual(); | ||
} | ||
|
||
void FailNotEqual() => Assert.Fail($"Value not equal in ${string.Join("", path.Reverse())}: expected {expected}, but was {actual}."); | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, I believe the regex generator defines such a struct as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a glance this seems like a type that would be defined as a
record
if arrays were naturally equitable. Why wasn't this just defined as arecord
withImmutableEquatableArray<object?>
wrappingMessageArgs
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would force an intermediate allocation from
object[]
toImmutableEquatableArray
then back to anobject[]
when theDiagnostic
gets materialized. I think it's small enough to justify a bespoke type. Longer term though we should be looking at addressing dotnet/roslyn#68291 and useDiagnostic
in the equatable models directly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diagnostic
will never be ok to use in a model.Location
has a reference to theSyntaxTree
that it was created from, and that will change every time the file changes. This wrapper has similar issues: even if message args compared correctly, it would still be broken for equality.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per #89587 (comment) this bit is mitigated for this PR, follow up in #92509.