Skip to content

Add nullable annotations to EnC impl#39195

Merged
tmat merged 1 commit intodotnet:masterfrom
tmat:Nullables
Oct 14, 2019
Merged

Add nullable annotations to EnC impl#39195
tmat merged 1 commit intodotnet:masterfrom
tmat:Nullables

Conversation

@tmat
Copy link
Member

@tmat tmat commented Oct 10, 2019

No description provided.

@tmat tmat marked this pull request as ready for review October 11, 2019 03:23
@tmat tmat requested a review from a team as a code owner October 11, 2019 03:23
@tmat tmat changed the base branch from master to darc-master-34a52118-29e2-475d-ab73-fe6377265200 October 11, 2019 03:24
@tmat tmat changed the base branch from darc-master-34a52118-29e2-475d-ab73-fe6377265200 to master October 11, 2019 03:24
@tmat tmat changed the title Nullables Add nullable annotations to EnC impl Oct 11, 2019
@tmat
Copy link
Member Author

tmat commented Oct 11, 2019

@ivanbasov PTAL

@tmat
Copy link
Member Author

tmat commented Oct 11, 2019

@sharwell FYI

Copy link
Contributor

@ivanbasov ivanbasov left a comment

Choose a reason for hiding this comment

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

:shipit:

@tmat tmat merged commit fad06a3 into dotnet:master Oct 14, 2019
@tmat tmat deleted the Nullables branch October 14, 2019 21:02
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Roslyn.Utilities;

#nullable enable
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 I've been putting this at the top (first item after copyright header). Placement here might make me miss it when looking through files in the future to see what work remains.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to move it up to unify.

}

private bool TryMapParameter(ValueTuple<SyntaxNode, int> parameterKey, IReadOnlyDictionary<SyntaxNode, SyntaxNode> map, out ValueTuple<SyntaxNode, int> mappedParameterKey)
private bool TryMapParameter((SyntaxNode? Node, int Ordinal) parameterKey, IReadOnlyDictionary<SyntaxNode, SyntaxNode> map, out (SyntaxNode? Node, int Ordinal) mappedParameterKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 The naming convention here deviates from the naming convention through the rest of this project.

Suggested change
private bool TryMapParameter((SyntaxNode? Node, int Ordinal) parameterKey, IReadOnlyDictionary<SyntaxNode, SyntaxNode> map, out (SyntaxNode? Node, int Ordinal) mappedParameterKey)
private bool TryMapParameter((SyntaxNode? node, int ordinal) parameterKey, IReadOnlyDictionary<SyntaxNode, SyntaxNode> map, out (SyntaxNode? node, int ordinal) mappedParameterKey)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we have actually agreed on a common convention for tuples.

else
{
topMatch.TryGetOldNode(typeSyntax, out partner);
_ = topMatch.TryGetOldNode(typeSyntax, out partner);
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

To make it explicit that we ignore the result. In this case the suggestion to be explicit that the analyzer reports makes sense.

{
GetDisplayName(syntax, EditKind.Insert),
GetDisplayName(TryGetContainingTypeDeclaration(syntax), EditKind.Update)
GetDisplayName(TryGetContainingTypeDeclaration(syntax)!, EditKind.Update)
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Why do we know this one is valid?

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 method is only called with syntax being a syntax of a field, property, or event of a type. So there will always be a containing type declaration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants