Added null checks on F# external access services#36469
Merged
TIHan merged 2 commits intodotnet:masterfrom Jun 14, 2019
Merged
Conversation
sharwell
approved these changes
Jun 14, 2019
heejaechang
approved these changes
Jun 14, 2019
brettfo
suggested changes
Jun 14, 2019
| if (mappedSignatureHelpItems != null) | ||
| { | ||
| return new SignatureHelpItems( | ||
| mappedSignatureHelpItems.Items?.Select(x => |
Member
There was a problem hiding this comment.
This will throw if it's null and also if it's empty. The whole function should return null instead.
Contributor
Author
There was a problem hiding this comment.
Sure, but at least it will throw inside SignatureHelpItems. It's going to throw no matter what if that is null. This just adds another guard check.
| FSharpGlyphHelpers.ConvertTo(item.Glyph), | ||
| item.Spans, | ||
| item.ChildItems.Select(x => ConvertToNavigationBarItem(x)).ToList(), | ||
| item.ChildItems?.Select(x => ConvertToNavigationBarItem(x)).ToList(), |
Member
There was a problem hiding this comment.
This might throw, it may be preferred to return null from this function.
Contributor
Author
There was a problem hiding this comment.
This is just a conversion and private. item really should never be null because the mapping happens on a collection.
333fred
added a commit
to 333fred/roslyn
that referenced
this pull request
Jun 18, 2019
…-types * dotnet/master: (63 commits) Fix stack overflow in requesting syntax directives (dotnet#36347) crash on ClassifyUpdate for EventFields (dotnet#35962) Disable move type when the options service isn't present (dotnet#36334) Fix crash where type inference doing method inference needs to drop nullability Fix parsing bug in invalid using statements (dotnet#36428) Do not suggest or diagnose use compound assignment when right hand of binary operator is a throw expression Add option to emit nullable metadata for public members only (dotnet#36398) Added null checks on F# external access services (dotnet#36469) Deal with discovering extra .editorconfig files Re-enable MSBuildWorkspaceTests.TestEditorConfigDiscovery Add support to VisualStudioMSBuildInstalled to support minimum versions Fix configuration of accessibilities in editorconfig Shorten a resource ID Revert "Extract the RDT implementation for Misc files and VS open file tracker" Add nullability support to use local function Add EditorFeatures.WPF dependency to F# ExternalAccess Ensure NullableWalker.AsMemberOfType locates the right new container for the member. (dotnet#36406) Replace `dynamic` with `object` when substituting constraints. (dotnet#36379) Add some string descriptions Adjust type of out var based on parameter state (dotnet#36284) ...
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
We need to check for nulls when we map F# types to Roslyn types, vice versa, for the shim work. We got a crash in
FSharpSignatureHelpProviderfor this reason. I fixed the sig help provider, but I also put more null checks in the other places that potentially could need it.