Use NRT by CyrusNajmabadi · Pull Request #53287 · dotnet/roslyn

CyrusNajmabadi · 2021-05-09T00:27:48Z

No description provided.

sharwell · 2021-05-10T14:59:04Z

...torParametersFromMembers/AddConstructorParametersFromMembersCodeRefactoringProvider.State.cs

+            [NotNull]
+            public INamedTypeSymbol? ContainingType { get; private set; }


This type is factory constructed. If you get an instance of it, the factory will have checked that this was not null.

Why not just public INamedTypeSymbol ContainingType { get; }?

i got warnings then that the constructor doesn't set it. Do we have a preferred idiom hre? It's a transitional value. but i would prefer it represent the value for the final consumer that passes this around and operates on it.

sharwell · 2021-05-10T15:02:09Z

...onstructorFromMembers/AbstractGenerateConstructorFromMembersCodeRefactoringProvider.State.cs

+            [NotNull]
+            public INamedTypeSymbol? ContainingType { get; private set; }


sharwell · 2021-05-10T15:03:57Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs

-        public bool IsIndexerMemberCRef(SyntaxNode node)
-            => node.Kind() == SyntaxKind.IndexerMemberCref;
+        public bool IsIndexerMemberCRef(SyntaxNode? node)
+            => node.IsKind(SyntaxKind.IndexerMemberCref);


💭 It's interesting that this doesn't required the parameter attributes to match. If this isn't just a compiler oversight, perhaps we should add a code style analyzer for it?

sharwell · 2021-05-10T15:07:33Z

...atures/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs

            if (index < 0)
            {
-                return (null, null);
+                return default;


The original form had a bunch of benefits here. If the original had used default I don't think this return would have even been flagged by NRT.

sharwell · 2021-05-10T15:08:52Z

...atures/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs

        }

-        private static (string pre, string post) GetPreAndPostTextParts(string text)
+        private static (string? pre, string? post) GetPreAndPostTextParts(string text)


💡 Based on use sites, (string pre, string post)? would be a less intrusive change here.

sharwell · 2021-05-10T15:09:46Z

...atures/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs

+                if (parameter == null)
+                    continue;


❔ Is this fixing a bug, or was it not reachable?

sharwell · 2021-05-10T15:13:54Z

...ures/Core/Portable/ConvertForToForEach/AbstractConvertForToForEachCodeRefactoringProvider.cs


                        if (!syntaxFacts.IsSimpleArgument(current.Parent) ||
-                            !syntaxFacts.IsElementAccessExpression(current.Parent.Parent.Parent))
+                            !syntaxFacts.IsElementAccessExpression(current.Parent?.Parent?.Parent))


📝 This only needed ?. for the last access:

current.Parent.Parent?.Parent

sharwell · 2021-05-10T15:14:37Z

...ures/Core/Portable/ConvertForToForEach/AbstractConvertForToForEachCodeRefactoringProvider.cs


            var collectionType = semanticModel.GetTypeInfo(collectionExpression, cancellationToken);
-            if (collectionType.Type == null && collectionType.Type.TypeKind == TypeKind.Error)
+            if (collectionType.Type == null || collectionType.Type.TypeKind == TypeKind.Error)


Can we add a test for this?

sharwell · 2021-05-10T15:18:06Z

src/Features/Core/Portable/AddParameter/AbstractAddParameterCodeFixProvider.cs

-            var conversion = compilation.ClassifyCommonConversion(argumentTypeInfo.Type, parameterType);
-            if (conversion.IsImplicit)
-                return true;
+            if (argumentTypeInfo.Type != null)


❔ Is this not an intended precondition of the method? If it is, we could just add the following at the start of the (private) method:

Contract.ThrowIfNull(argumentTypeInfo.Type);

Looking at call sites, it looks more complicated (i.e. null is allowed for this property if the conditions above produce an early return). We could still add a Contract.ThrowIfNull, but it would need to be at this location in the method.

sharwell · 2021-05-10T15:22:12Z

src/Features/Core/Portable/AddMissingReference/AddMissingReferenceCodeAction.cs

-                if (string.Equals(missingAssemblyIdentity.Name, candidateProject.AssemblyName, StringComparison.OrdinalIgnoreCase))
+                var candidateProject = project.Solution.GetRequiredProject(candidateProjectId);
+                if (candidateProject.SupportsCompilation &&
+                    string.Equals(missingAssemblyIdentity.Name, candidateProject.AssemblyName, StringComparison.OrdinalIgnoreCase))


@jasonmalinowski to verify if we need to add any new checks for target projects that do not support compilation

sharwell · 2021-05-10T15:24:12Z

...ertToInterpolatedString/AbstractConvertPlaceholderToInterpolatedStringRefactoringProvider.cs

-            var formatMethodsAcceptingParamsArray = formatMethods
-                    .Where(x => x.Parameters.Length > 1 && x.Parameters[1].Type.Kind == SymbolKind.ArrayType);
-            if (formatMethodsAcceptingParamsArray.Contains(invocationSymbol))
+            if (expression != null)


Can we add a test for this gap?

Use NRT

33bb163

CyrusNajmabadi requested a review from a team as a code owner May 9, 2021 00:27

ghost added the Area-IDE label May 9, 2021

CyrusNajmabadi added 27 commits May 8, 2021 17:28

use NRT

9f40172

use NRT

cf46523

use NRT

4946e58

use NRT

6798266

use NRT

99b9fea

use NRT

18d40f2

use NRT

cbbbaac

use NRT

7c285cf

use NRT

4914bb0

use NRT

518fa24

use NRT

7e38c1e

use NRT

95f9991

use NRT

8db0763

use NRT

aa27466

use NRT

bcf7f3e

use NRT

d1a2b79

use NRT

7444166

use NRT

7272bda

use NRT

a56494e

use NRT

88bfb21

use NRT

08d2cf7

use NRT

4cb0ecf

use NRT

13e66d3

use NRT

7f09f87

use NRT

4b3857c

use NRT

3cfc06e

use NRT

bb8bbf8

CyrusNajmabadi added 12 commits May 8, 2021 18:23

use NRT

e8e4318

use NRT

813ab00

use NRT

2fcf8a4

use NRT

57a8ea5

use NRT

2bef418

use NRT

2855131

use NRT

0d1799a

use NRT

dca9bc9

use NRT

a9422ab

use NRT

ce439a0

use NRT

c7554f0

Warning

609a704

davidwengier approved these changes May 9, 2021

View reviewed changes

CyrusNajmabadi merged commit 47fb229 into dotnet:main May 10, 2021

CyrusNajmabadi deleted the useNRT branch May 10, 2021 01:23

ghost added this to the Next milestone May 10, 2021

sharwell reviewed May 10, 2021

View reviewed changes

RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021

CyrusNajmabadi mentioned this pull request Feb 14, 2022

Possible NullReferenceException in AbstractConvertForToForEachCodeRefactoringProvider.cs #35320

Closed

		[NotNull]
		public INamedTypeSymbol? ContainingType { get; private set; }

Conversation

CyrusNajmabadi commented May 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sharwell May 10, 2021 •

edited

Loading