Address NRT issues found by targeting net5.0 #50895

huoyaoyuan · 2021-01-29T08:30:19Z

Moving target is currently blocked by #50893 . Cherry-picking NRT commits. Part of #50822.

Mostly caused by LINQ, Path/Directory and xml dom.

I'm trying to use asset patterns in the same file.

huoyaoyuan · 2021-01-29T08:31:19Z

src/Compilers/Core/Portable/Syntax/SyntaxNodeExtensions_Tracking.cs

@@ -89,7 +89,7 @@ public static IEnumerable<TNode> GetCurrentNodes<TNode>(this SyntaxNode root, TN
        /// </summary>
        /// <param name="root">The root of the subtree containing the current node corresponding to the original tracked node.</param>
        /// <param name="node">The node instance originally tracked.</param>
-        public static TNode GetCurrentNode<TNode>(this SyntaxNode root, TNode node)
+        public static TNode? GetCurrentNode<TNode>(this SyntaxNode root, TNode node)


This one is public API bug, and causing bug in Features layer.

huoyaoyuan · 2021-01-29T08:32:23Z

src/Features/Core/Portable/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs

@@ -175,7 +175,7 @@ private static void ReportTelemetryIfNecessary(ImmutableArray<(TExpressionSyntax
            var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
            var spanNodes = diagnostics.SelectAsArray(
                d => root.FindNode(d.Location.SourceSpan, getInnermostNodeForTie: true)
-                         .GetAncestorsOrThis<TExpressionSyntax>().FirstOrDefault());
+                         .GetAncestorsOrThis<TExpressionSyntax>().First());


The code looks very confidential about it (defensive in outer code but not this), so I changed to First.

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

...orkspaces/Core/Portable/FindSymbols/FindReferences/Finders/ParameterSymbolReferenceFinder.cs

src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/SyntaxTreeExtensions.cs

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/SyntaxNodeExtensions.cs

src/Compilers/Core/Portable/AssemblyUtilities.cs

src/Compilers/Core/Portable/InternalUtilities/IncrementalHashExtensions.cs

jcouv · 2021-01-29T18:45:41Z

src/Compilers/Core/Portable/InternalUtilities/RoslynLazyInitializer.cs

@@ -19,11 +19,11 @@ internal static class RoslynLazyInitializer
            => LazyInitializer.EnsureInitialized<T>(ref target!, valueFactory);

        /// <inheritdoc cref="LazyInitializer.EnsureInitialized{T}(ref T, ref bool, ref object)"/>
-        public static T EnsureInitialized<T>([NotNull] ref T? target, ref bool initialized, [NotNull] ref object? syncLock)
+        public static T EnsureInitialized<T>([NotNull] ref T? target, ref bool initialized, [NotNullIfNotNull("syncLock")] ref object? syncLock)


That's clever ;-)
Do we still need RoslynLazyInitializer now that LazyInitializer is annotated? If it's not a trivial change, please file an issue and assign it to me.

I copied the annotation from net5.0.
Lack of [NotNull] should have impact on netstandard2.0

Makes sense. Thanks

jcouv

Done with review pass of compiler changes (iteration 3)

RikkiGibson · 2021-01-29T19:22:00Z

It's necessary to merge the latest master into this branch to fix the build.

jcouv

Compiler changes LGTM Thanks (iteration 5)

jcouv · 2021-02-09T00:57:02Z

@dotnet/roslyn-ide for review of the IDE changes. Thanks

cston

Compiler changes LGTM, thanks.

jcouv · 2021-02-10T23:42:32Z

@dotnet/roslyn-ide @jinujoseph for review of the IDE changes. Thanks

sharwell · 2021-02-11T16:48:19Z

src/Compilers/Core/Portable/AssemblyUtilities.cs

@@ -106,6 +106,7 @@ public static ImmutableArray<string> FindSatelliteAssemblies(string filePath)
            var builder = ImmutableArray.CreateBuilder<string>();

            string? directory = Path.GetDirectoryName(filePath);
+            RoslynDebug.AssertNotNull(directory);


❔ Is it ever possible for PathUtilities.IsAbsolute(filePath) to return true but Path.GetDirectoryName(filePath) to return null? If not, I almost prefer this form:

Debug.Assert(PathUtilities.IsAbsolute(filePath)); string directory = Path.GetDirectoryName(filePath)!;

If path happens to be C:\?

Yeah, assert seems wise so I think this looks good -- there shouldn't ever be a directory name versus a file path being passed here, but if that were to happen...

src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs

sharwell

I made it partly through. It'll take a while to finish all cases since I really have to think about each one :)

jasonmalinowski

Generally looks good, but a few specific questions.

jasonmalinowski · 2021-02-25T23:27:50Z

src/Compilers/Core/Portable/AssemblyUtilities.cs

@@ -106,6 +106,7 @@ public static ImmutableArray<string> FindSatelliteAssemblies(string filePath)
            var builder = ImmutableArray.CreateBuilder<string>();

            string? directory = Path.GetDirectoryName(filePath);
+            RoslynDebug.AssertNotNull(directory);


Yeah, assert seems wise so I think this looks good -- there shouldn't ever be a directory name versus a file path being passed here, but if that were to happen...

jasonmalinowski · 2021-02-25T23:36:52Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOption2`1.cs

@@ -82,7 +82,7 @@ public NotificationOption2 Notification
            new("CodeStyleOption", // Ensure that we use "CodeStyleOption" as the name for back compat.
                new XAttribute(nameof(SerializationVersion), SerializationVersion),
                new XAttribute("Type", GetTypeNameForSerialization()),
-                new XAttribute(nameof(Value), GetValueForSerialization()),
+                new XAttribute(nameof(Value), GetValueForSerialization()!),


What's going on here? GetValueForSerialization is pretty directly marked as null-returning; can we revise that and push the suppression somewhere closer if possible?

It's only nullable when using uninitialized value with T=string. There could be a later refactor on that.

I guess I'd say in that case we should:

annotate GetValueForSerialization to return not-null.

In the branch for string, do a Contract.ThrowIfNull().

In the branch for bool, just ! since that's more obvious what's going on here.

Now I'm also a bit confused because the docs for the type say that T is only allowed to be bool or an enum, so clearly we are breaking that rule somewhere...? @mavasani or @jmarolf any idea if nulls are supposed to be supported here?

@jmarolf confirmed that we don't expect to get nulls here, so current code is good.

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOption2`1.cs

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/TaskExtensions.cs

src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs

...orkspaces/Core/Portable/FindSymbols/FindReferences/Finders/ParameterSymbolReferenceFinder.cs

src/Compilers/Core/Portable/PEWriter/SigningUtilities.cs

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

RikkiGibson · 2021-02-26T19:20:51Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/TaskExtensions.cs

        public static T WaitAndGetResult<T>(this Task<T> task, CancellationToken cancellationToken)
        {
 #if DEBUG
-            if (IsThreadPoolThread(Thread.CurrentThread))
+            if (Thread.CurrentThread.IsThreadPoolThread)


jasonmalinowski

Comments left; this is looking pretty good from my perspective although do want some of the feature area owners to comment on their own code.

jasonmalinowski · 2021-03-01T20:29:43Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOption2`1.cs

@@ -82,7 +82,7 @@ public NotificationOption2 Notification
            new("CodeStyleOption", // Ensure that we use "CodeStyleOption" as the name for back compat.
                new XAttribute(nameof(SerializationVersion), SerializationVersion),
                new XAttribute("Type", GetTypeNameForSerialization()),
-                new XAttribute(nameof(Value), GetValueForSerialization()),
+                new XAttribute(nameof(Value), GetValueForSerialization()!),


I guess I'd say in that case we should:

annotate GetValueForSerialization to return not-null.

In the branch for string, do a Contract.ThrowIfNull().

In the branch for bool, just ! since that's more obvious what's going on here.

Now I'm also a bit confused because the docs for the type say that T is only allowed to be bool or an enum, so clearly we are breaking that rule somewhere...? @mavasani or @jmarolf any idea if nulls are supposed to be supported here?

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

This reverts commit a0496e9.

jasonmalinowski · 2021-03-05T00:53:10Z

Thanks @huoyaoyuan for the fixes!

huoyaoyuan added 3 commits January 29, 2021 16:21

Resolve NRT warnings in Compiler layer.

b844dc6

Resolve NRT warnings in workspace layer core.

bd953a3

Resolve NRT warnings in Features.

3d29d69

huoyaoyuan requested review from a team as code owners January 29, 2021 08:30

dotnet-issue-labeler bot added the Area-Compilers label Jan 29, 2021

huoyaoyuan commented Jan 29, 2021

View reviewed changes

jcouv added the Concept-Null Annotations The issue involves annotating an API for nullable reference types label Jan 29, 2021

jcouv reviewed Jan 29, 2021

View reviewed changes

src/Compilers/Core/Portable/AssemblyUtilities.cs Outdated Show resolved Hide resolved

jcouv reviewed Jan 29, 2021

View reviewed changes

src/Compilers/Core/Portable/InternalUtilities/IncrementalHashExtensions.cs Outdated Show resolved Hide resolved

jcouv reviewed Jan 29, 2021

View reviewed changes

jcouv self-assigned this Jan 29, 2021

jcouv reviewed Jan 29, 2021

View reviewed changes

huoyaoyuan added 2 commits January 31, 2021 01:37

Merge branch 'master' into net5.0-nrt

fa95138

Add assertion.

00fb401

jcouv approved these changes Jan 30, 2021

View reviewed changes

jaredpar added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 1, 2021

cston approved these changes Feb 9, 2021

View reviewed changes

jinujoseph assigned jasonmalinowski Feb 10, 2021

sharwell reviewed Feb 11, 2021

View reviewed changes

src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs Outdated Show resolved Hide resolved

sharwell reviewed Feb 11, 2021

View reviewed changes

jcouv removed the Area-Compilers label Feb 16, 2021

jasonmalinowski reviewed Feb 26, 2021

View reviewed changes

Remove reflection of IsThreadPoolThread.

4ecfa02

RikkiGibson reviewed Feb 26, 2021

View reviewed changes

huoyaoyuan added 3 commits February 27, 2021 19:08

Merge branch 'master' into net5.0-nrt

012966c

Fix PublicAPI annotation for GetCurrentNode.

910f407

Address review comments.

d837ee9

jasonmalinowski reviewed Mar 1, 2021

View reviewed changes

Base automatically changed from master to main March 3, 2021 23:53

huoyaoyuan added 4 commits March 4, 2021 20:40

Add notnull for CodeStyleOption.

a0496e9

Throw instead of supress.

2752737

Revert "Add notnull for CodeStyleOption."

6a48567

This reverts commit a0496e9.

Make GetValueForSerialization non-nullable.

cf72285

jasonmalinowski approved these changes Mar 5, 2021

View reviewed changes

jasonmalinowski merged commit 0aaa2eb into dotnet:main Mar 5, 2021

huoyaoyuan deleted the net5.0-nrt branch March 5, 2021 04:20

allisonchou added this to the Next milestone Mar 12, 2021

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address NRT issues found by targeting net5.0 #50895

Address NRT issues found by targeting net5.0 #50895

huoyaoyuan commented Jan 29, 2021

huoyaoyuan Jan 29, 2021

huoyaoyuan Jan 29, 2021

jcouv Jan 29, 2021

huoyaoyuan Jan 29, 2021

jcouv Jan 29, 2021

jcouv left a comment

RikkiGibson commented Jan 29, 2021

jcouv left a comment

jcouv commented Feb 9, 2021

cston left a comment

jcouv commented Feb 10, 2021

sharwell Feb 11, 2021

huoyaoyuan Feb 12, 2021

jasonmalinowski Feb 25, 2021 •

edited

Loading

sharwell left a comment

jasonmalinowski left a comment

jasonmalinowski Feb 25, 2021 •

edited

Loading

jasonmalinowski Feb 25, 2021

huoyaoyuan Feb 27, 2021

jasonmalinowski Mar 1, 2021

jasonmalinowski Mar 5, 2021

RikkiGibson Feb 26, 2021

jasonmalinowski left a comment

jasonmalinowski Mar 1, 2021

jasonmalinowski commented Mar 5, 2021

Address NRT issues found by targeting net5.0 #50895

Address NRT issues found by targeting net5.0 #50895

Conversation

huoyaoyuan commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

RikkiGibson commented Jan 29, 2021

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Feb 9, 2021

cston left a comment

Choose a reason for hiding this comment

jcouv commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

sharwell left a comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski commented Mar 5, 2021

jasonmalinowski Feb 25, 2021 •

edited

Loading

jasonmalinowski Feb 25, 2021 •

edited

Loading