Resolve ILLink warnings in System.Text.Json (Round 1) #51886
Conversation
eerhardt
requested review from
krwq,
eiriktsarpalis,
steveharter,
layomia and
joperezr
|
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue DetailsAlso resolve ILLink warnings in System.Net.Http.Json. Contributes to #45623 There is one last ILLink warning in
|
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
eerhardt
added
linkable-framework
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer Issue DetailsAlso resolve ILLink warnings in System.Net.Http.Json. Contributes to #45623 There is one last ILLink warning in
|
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
Outdated
Show resolved
Hide resolved
| @@ -13,6 +13,11 @@ | |||
| using System.Threading; | |||
| using System.Threading.Tasks; | |||
|
|
|||
| [assembly: UnconditionalSuppressMessage("ReflectionAnalysis", "IL2091:UnrecognizedReflectionPattern", | |||
| Target = "M:System.Text.Json.JsonSerializer.<<DeserializeAsyncEnumerable>g__CreateAsyncEnumerableDeserializer|27_0>d`1.MoveNext()", | |||
There was a problem hiding this comment.
Appears to be based on C# generated types which in theory could change in the future
There was a problem hiding this comment.
Yes, unfortunately this is a current limitation of the trimmer with async methods. I'm open to other suggestions, because this is the only way I know how to resolve these warnings.
There was a problem hiding this comment.
No great answers here, unfortunately. I think the C# compiler should be involved in brainstorming a solution. The same problem would appear if you wanted to, say, put MethodImplOptions on an async method.
There was a problem hiding this comment.
The mapping from the kickoff method to the MoveNext method is available in the debug info - when illinker is running in the "library mode", it could propagate the attribute (simply write it into the trimmed library). It shouldn't be too hard and it would work as a stopgap for us.
Of course this would only work if one has debug info and one runs illink in this special mode to propagate the attribute.
There was a problem hiding this comment.
Of course this would only work if one has debug info
Which will be a problem for our customers trimming their apps that use System.Text.Json, since they won't have .pdbs for System.Text.Json at build time. So they would see these warnings.
There was a problem hiding this comment.
Yes, that's why I was suggesting we would propagate it when illink is running as part of building this repo (i.e. when it runs in the library mode, it would just look at the debug info and add a physical attribute on the MoveNext method). It would solve the problem for us right now and I don't think it's very expensive to do.
Also resolve ILLink warnings in System.Net.Http.Json. Contributes to dotnet#45623
src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs
Outdated
Show resolved
Hide resolved
eerhardt
force-pushed
the
JsonILLinkWarnings
branch
from
45d8bc1
to
a0ae5f6
Compare
|
Hello @eerhardt! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
ghost
locked as resolved and limited conversation to collaborators
Also resolve ILLink warnings in System.Net.Http.Json.
Contributes to #45623
There is one last ILLink warning in
JsonValue<T>that I intend to address in a separate PR, since this change is big enough as it is.