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

Fix a number of accessibility resolution issues in the source generator. #87136

Merged

Conversation

eiriktsarpalis
Copy link
Member

Updates the source generator to use the built-in Roslyn IsSymbolAccessibleWithin method when resolving accessibility for symbols. This addresses the following issues:

@ghost
Copy link

ghost commented Jun 5, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Updates the source generator to use the built-in Roslyn IsSymbolAccessibleWithin method when resolving accessibility for symbols. This addresses the following issues:

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Jun 5, 2023
@@ -589,179 +591,41 @@ private TypeGenerationSpec ParseTypeGenerationSpec(TypeToGenerate typeToGenerate
{
classType = ClassType.Enum;
}
else if (type.GetCompatibleGenericBaseType(_knownSymbols.IAsyncEnumerableOfTType) is INamedTypeSymbol iasyncEnumerableType)
Copy link
Member Author

Choose a reason for hiding this comment

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

The deleted code has been extracted into the TryResolveCollectionType method .

@@ -426,8 +426,10 @@ public record AppRecord(int Id)
CheckCompilationDiagnosticsErrors(result.Diagnostics);
CheckCompilationDiagnosticsErrors(result.NewCompilation.GetDiagnostics());

Assert.Equal(4, result.AllGeneratedTypes.Count());
Assert.Equal(3, result.AllGeneratedTypes.Count());
Copy link
Member Author

Choose a reason for hiding this comment

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

Generated type number changed because we no longer include the compiler-generated EqualityContract property.

@eiriktsarpalis
Copy link
Member Author

cc @Sergio0694 it would be interesting to see how this impacts your application size.

@Sergio0694
Copy link
Contributor

@eiriktsarpalis will share binary size deltas in the Store and in the NAOT sample with the new package from this PR! 👍

@Sergio0694
Copy link
Contributor

Sergio0694 commented Jun 6, 2023

@eiriktsarpalis size diffs for the Microsoft Store below:

System.Text.Json version Package size x64 binary size
7.0.2 67.277 KB 49.735 KB
8.0.0-preview.5.23275.6 65.605 KB 46.998 KB
8.0.0-preview.6.23305.2 65.605 KB 46.999 KB
8.0.0-preview.6.23305.5 65.571 KB 46.975 KB

Deltas:

  • Package: -1.706 MB (-2.53%)
  • x64 binary: -2.760 MB (-5.55%)

Pretty nice win overall, and it seems this PR saved another ~35 KB per arch. Not groundbreaking, but hey, it's free 😄

@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants