moved to List from ImmutableArray on json return type. #21395
Conversation
ImmutableArray is causing isssue on Json lib. use regular List since we don't actually need immutable array here.
|
@dotnet/roslyn-analysis @dotnet/roslyn-ide can I get code review? |
|
adding @jinujoseph and @mavasani |
|
What about: IRemoteAddImportFeatureService.GetFixesAsync? These all have APIs that return immutable arrays. |
|
@CyrusNajmabadi Do we have the ability to register a JSON deserializer for a generic type? |
I imagine we would, as long as json.net didn't overrule us. All we need to override is: http://www.newtonsoft.com/json/help/html/M_Newtonsoft_Json_JsonConverter_CanConvert.htm |
|
📝 I confirmed through WinDbg analysis of the crash that this was caused by JamesNK/Newtonsoft.Json#652. Possible solutions:
|
|
@AArnott @AlexEyler Is there a way we can get VS to prevent overriding of json.net? it seems like that may be a problem for many parts of VS that expect to be getting the redirected 9.0 version of json.net? |
|
unless there is real reason that we must use ImmutableArray, I think we should just make them to use regular collection for now. |
|
@sharwell I will remove ImmutableArray usage from json.net for now. we can consider using ImmutableArray later once we are sure that it is supported reliably. |
|
@heejaechang @sharwell I'm ok with that. @heejaechang Please take a look at teh full surface area of both CodeAnalysisService and RemoteSymbolSearchUpdateEngine |
|
@CyrusNajmabadi @AlexEyler I filed this to track on the VS side: Bug 476064: Protect certain platform assemblies from loading from extensions |
|
@AArnott @CyrusNajmabadi thanks - we were talking about this recently as well, others on the team (including myself) share the same concern. |
|
ping, can I get some code review? |
|
actually, IReadOnlyList is in 4.5 as well. so probably only choice I have is either IList or IEnumerable. |
| return SpecializedCollections.EmptyReadOnlyList<DesignerAttributeDocumentData>(); | ||
| } | ||
|
|
||
| return data.Values.ToList(); |
There was a problem hiding this comment.
I feel like i should be seeing more of this in the code. Basically, all the places where we return an immutable array (even casted to an IReadOnlyList) would need to actually be converted to a List. THat's because json.net isn't going to see or care that this is IReadOnlyList. It's going to look at the real underlying type and will decide what to do with it. If that underlying type is stilL IMmutableArray, things will still not work.
There was a problem hiding this comment.
I dont believe that's how it works.
https://github.com/JamesNK/Newtonsoft.Json/blob/b65807bbd2be51e8fa5bfebb50af5786b3eeb9ed/Src/Newtonsoft.Json/Serialization/JsonArrayContract.cs#L181
it will just create ReadOnlyCollection
|
@heejaechang Unless you plan to retarget this to 15.4 or earlier, I would recommend waiting until we establish a timeline on JamesNK/Newtonsoft.Json#652 getting fixed. If that gets Edit: It's already fixed. |
|
@sharwell is the fix going to be shipped in a 9.x release? If it's only for 10.x releases, that won't help you for a while yet, since VS ships 9.x. Even when VS ships 10.x, it will be a SXS deployment with 9.0 rather than a binding redirect to 10 since 10 has breaking changes. |
|
@sharwell I would rather be in safe side until things are settled down and verified. we can always go back to immutablearray when needed. it won't affect any public API or functionality. |
|
It will be a 10 release |
|
moved all interface that uses json.net to use IList (safest one to use for now). we will move it to immutable array once we are sure all issues around using immutable array in json.net in vs is resolved. |
|
can I get some code review? |
| { | ||
| return (succeeded: false, ImmutableArray<DocumentHighlights>.Empty); | ||
| } | ||
|
|
||
| return (true, result.SelectAsArray(h => h.Rehydrate(document.Project.Solution))); | ||
| return (true, result.Select(h => h.Rehydrate(document.Project.Solution)).ToImmutableArray()); |
There was a problem hiding this comment.
does this need to be SelectAsArray?
There was a problem hiding this comment.
yes. let me update.
|
@JamesNK A 10 release will mean we can't benefit for a while yet. |
|
You can update to 11 when it is eventually released. Alternatively you could write a JsonConverter to manually serialize/deserialize immutable collections if you want to fix the issue today. |
In my mind this is the most robust approach, because it ensures we won't hit this bug in the future. Currently |
|
Another feature team was just bitten by this bug as well yesterday. So having each individual feature have to learn that they need to write |
This would work, but IIRC the strong name key isn't public so either we public sign it (definitely keeps it out of the GAC), or we request a build of latest 9.x with assembly version set to 9.1 from @JamesNK, and only release the .NET 4.5 build (or 4.6, whichever one it is VS needs). This would be a "VS specific build" sent directly to MS and not published to NuGet. For StyleCop Analyzers we ended up switching to embedded LightJson to deal with various woes, but those were mostly related to distribution across multiple platforms (not always Windows). |
|
I just would like to use IList, there is no actual reason where we must use ImmutableArray here. we can move to using ImmutableArray when we are confident or figure out what to do here. before we moved to ImmutableArray all code was written to use either array or IList anyway. |
|
regardless of discussion where we should have ImmutableArray or not, I would like to get some code review on this change. basically it removes usage of ImmutableArray from json.net and use the safest collection (IList) in json.net. tagging @dotnet/roslyn-analysis @dotnet/roslyn-ide |
There was a problem hiding this comment.
This looks fine to me on the condition that it will ship to customers (referring to a stable update release, not a pre-release) prior to any alternative solution being made available by a VS platform team. ImmutableArray<T> makes for a better API, and we should exhaust our options related to its preservation prior to resorting to this approach.
If this does gets merged, a bug should be filed to revert it as soon as the VS platform offers a solution which no longer requires it.
| @@ -242,6 +244,19 @@ public static IEnumerable<T> WhereNotNull<T>(this IEnumerable<T> source) | |||
| return source.Where((Func<T, bool>)s_notNullTest); | |||
| } | |||
|
|
|||
| public static ImmutableArray<TResult> SelectAsArray<TSource, TResult>(this IEnumerable<TSource> source, Func<TSource, TResult> selector) | |||
There was a problem hiding this comment.
💭 I thought we already had this extension method. Maybe not accessible here though?
There was a problem hiding this comment.
that's between ImmutalbArray to ImmutableArray . this is IEnumerable to ImmutableArray
| public static T Last<T>(this IReadOnlyList<T> list) | ||
| { | ||
| return list[list.Count - 1]; | ||
| } | ||
|
|
||
| private class ReadOnlyList<T> : IReadOnlyList<T> |
There was a problem hiding this comment.
❓ Why not ReadOnlyCollection<T>?
There was a problem hiding this comment.
IReadOnlyList has indexer which is drop in replacement for ImmutebleArray, IreadOnlyCollection doesn't have indexer.
There was a problem hiding this comment.
@heejaechang I'm talking about the ReadOnlyCollection<T> class, not the IReadOnlyCollection<T> interface.
There was a problem hiding this comment.
are you talking about existing ReadOnlyCollection type. I dont know why it is using ICollection rather than IReadOnlyCollection.
|
@sharwell sure, I like using ImmutableArray as well. as long as It don't cause any issue. I will open an issue. |
|
Certainly I don't mean to hold up the PR. It's just a relevant and interesting discussion. :) |
|
tagging issue #21498 |
|
@AArnott they are not related to public API and IReadOnlyList is also only supported in 4.5 version. so for now, I am just going to use most safe collection. |
Is that a problem for you? Does Roslyn build to run on something older than that? |
|
@AArnott isn't the immutableArray issue we are having is due to json.net that doesn't support ImmutableArray is loaded? I believe IReadOnlyList has same issue? |
|
I don't think that would be an issue if your field is typed as |
|
@JamesNK we tried going with a private build of 9.0 with 9.0.0.153 as an assembly version, and that worked, except because we public signed it, it can't be ngen'd. Can you spin a build of 9.0.1 with an assembly version of 9.0.0.153 that is signed with your private key? |
|
Fixing bugs that could effect anyone is fine - I've fixed this issue so it won't be a problem in future versions of Json.NET - but I don't have enough time to spend it on creating custom builds for individual people. Json.NET is something I work on my limited own time. |
|
Understood, @JamesNK. To be fair, I'd be able to build it myself if you chose to expose your private key for strong name signing. But I'm sure you have your reasons. |
ImmutableArray is causing isssue on Json lib. use regular List since we don't actually need immutable array here.
fixing this
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/473979
no heap, but callstack shows that immutable array is default, but we are getting it from json.net lib. we could check isDefault, but I dont want to get into hassle until I know newston.json supports immutable array reliably. moving back to non immutable collection when crossing json.