Annotate most of Microsoft.VisualStudio.ProjectSystem.Managed #5003

drewnoakes · 2019-07-09T03:16:27Z

No description provided.

drewnoakes · 2019-07-09T03:18:05Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/ImmutableCollectionLinqExtensions.cs

@@ -41,6 +40,7 @@ internal static class ImmutableCollectionLinqExtensions
            return false;
        }

+        [return: MaybeNull]
        public static T FirstOrDefault<T, TArg>(this ImmutableArray<T> immutableArray, Func<T, TArg, bool> predicate, TArg arg)


This is the recommended approach for indicating that an output with unconstrained generic type can be null.

Ugh - that is not great.

drewnoakes · 2019-07-09T03:19:58Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/ImmutableCollectionLinqExtensions.cs

@@ -49,7 +49,7 @@ internal static class ImmutableCollectionLinqExtensions
                    return obj;
            }

-            return default;
+            return default!;


Unfortunately the attribute above doesn't influence the member body, so we need to ! force this here. Tracked by dotnet/roslyn#36039.

drewnoakes · 2019-07-09T03:22:33Z

...tudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/ApplyChangesToWorkspaceContext.cs

-        private IWorkspaceProjectContext _context;
-        private ExportLifetimeContext<IWorkspaceContextHandler>[] _handlers;
+        private IWorkspaceProjectContext? _context;
+        private ExportLifetimeContext<IWorkspaceContextHandler>[] _handlers = Array.Empty<ExportLifetimeContext<IWorkspaceContextHandler>>();


In some cases it is easier to use singleton empty collections rather than null. Doing so can avoid lots of null checks or !, especially when there's an implicit relationship between being initialised and non-null (as happens a lot in this repo).

I agree in some cases, but in this case it would be a bug that this is null that is no longer caught.

drewnoakes · 2019-07-09T03:24:38Z

...tudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/ApplyChangesToWorkspaceContext.cs

            }
        }

        private void ProcessProjectBuildFailure(IProjectRuleSnapshot snapshot)
        {
+            Assumes.NotNull(_context);


Unfortunately these guard methods are not annotated to indicate that the argument is not null after this point. There is a DoesNotReturnIf(bool) attribute, but not a DoesNotReturnIfNull(string parameterName) attribute. It seems we have to ! the first usage after this point. I filed dotnet/roslyn#37071 to see if we could just ! the argument to the guard method directly, which would be nicer.

can we file an issue on the VSSDK folks for this?

Can do, but there's nothing they can do until the compiler supports it. As soon as there's compiler support, I'll send them a PR if they don't get to it first.

Actually it's possible to do this with the current attributes:

void AssertNotNull([NotNull] object? o) { } string M(object? o) { AssertNotNull(o); return o.ToString(); // no diagnostic here }

drewnoakes · 2019-07-09T03:25:52Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/BuildOptions.cs

 namespace Microsoft.VisualStudio.ProjectSystem.LanguageServices
 {
    internal class BuildOptions
    {
-        private static readonly PropertyInfo s_analyzerConfigPathsProperty = typeof(CommandLineArguments).GetProperty("AnalyzerConfigPaths");
+        private static readonly PropertyInfo? s_analyzerConfigPathsProperty = typeof(CommandLineArguments).GetProperty("AnalyzerConfigPaths");


Type.GetProperty is not annotated. I saw a later null test and annotated this. If the ? here was removed, it would still compile without warning.

I mention this as there are probably other places like this in the repo.

drewnoakes · 2019-07-09T03:33:16Z

...icrosoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/UnconfiguredProjectTasksService.cs

@@ -49,7 +46,7 @@ public Task OnProjectFactoryCompleted()
            }
            else
            {
-                _projectLoadedInHost.TrySetResult(false);
+                _projectLoadedInHost.TrySetResult(null);


We were boxing a bool as an object here.

drewnoakes · 2019-07-09T03:35:20Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/Text/LazyStringSplit.cs

@@ -69,7 +67,7 @@ internal Enumerator(in LazyStringSplit split)
                _index = 0;
                _input = split._input;
                _delimiter = split._delimiter;
-                Current = null;
+                Current = null!;


I think this makes sense for enumerators. Technically Current can be null, but 99.99% of consumers are foreach, in which case you won't see that state of the enumerator. You can still see nulls if the sequence has null elements though (which it won't here).

drewnoakes · 2019-07-09T03:35:57Z

...ctSystem.Managed/Tree/Dependencies/Subscriptions/RuleHandlers/DependenciesRuleHandlerBase.cs

@@ -75,7 +75,7 @@ void HandleChangesForRule(bool resolved, IProjectChangeDescription projectChange
                foreach (string removedItem in projectChange.Difference.RemovedItems)
                {
                    string dependencyId = resolved
-                        ? projectChange.Before.GetProjectItemProperties(removedItem).GetStringProperty(ResolvedAssemblyReference.OriginalItemSpecProperty) ?? removedItem
+                        ? projectChange.Before.GetProjectItemProperties(removedItem)!.GetStringProperty(ResolvedAssemblyReference.OriginalItemSpecProperty) ?? removedItem


Assuming the changed item has properties here, hence !.

drewnoakes · 2019-07-09T03:36:27Z

...ctSystem.Managed/Tree/Dependencies/Subscriptions/RuleHandlers/DependenciesRuleHandlerBase.cs


                    string originalItemSpec = resolved
-                        ? properties.GetStringProperty(ResolvedAssemblyReference.OriginalItemSpecProperty)
+                        ? properties.GetStringProperty(ResolvedAssemblyReference.OriginalItemSpecProperty) ?? itemSpec


Behaviour change. if somehow the original item spec property was null, use the item spec.

We should probably change these methods to return empty to behave identiical to MSBuild; if a property doesn't exist its "empty". Probably need to rationalize these extension methods with this ProjectRuleSnapshotExtensions.

drewnoakes · 2019-07-09T03:36:50Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/Utilities/MetadataExtensions.cs

@@ -37,7 +36,7 @@ internal static class MetadataExtensions
        /// <param name="key">The key that identifies the property to look up.</param>
        /// <param name="stringValue">The value of the string if found and non-empty, otherwise <see langword="null"/>.</param>
        /// <returns><see langword="true"/> if the property was found with a non-empty value, otherwise <see langword="false"/>.</returns>
-        public static bool TryGetStringProperty(this IImmutableDictionary<string, string> properties, string key, out string stringValue)
+        public static bool TryGetStringProperty(this IImmutableDictionary<string, string> properties, string key, [NotNullWhen(returnValue: true)] out string? stringValue)


TryGet* pattern.

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/FaultExtensions.cs

...rojectSystem.Managed/ProjectSystem/LanguageServices/FSharp/FSharpCommandLineParserService.cs

...isualStudio.ProjectSystem.Managed/ProjectSystem/OnceInitializedOnceDisposedUnderLockAsync.cs

...ctSystem/LanguageServices/WorkspaceProjectContextHost.WorkspaceProjectContextHostInstance.cs

This allows marking the fields as non-nullable.

...rojectSystem.Managed/ProjectSystem/LanguageServices/FSharp/FSharpCommandLineParserService.cs

Potentially masks bugs when the import fails.

davkean · 2019-07-11T03:08:20Z

We're want to merge this in quickly to avoid it fall behind. @jmarolf Are you good with the change?

jmarolf · 2019-07-11T03:47:10Z

Yep, lets not hold this one up

drewnoakes added 6 commits July 9, 2019 13:13

Update compiler version

b2f3d01

Suppress warnings

fe0123a

Typos

04128f4

Remove redundant return

ae28445

Add missing copyright header

5d15454

Make field readonly

4338762

drewnoakes requested a review from a team as a code owner July 9, 2019 03:16

drewnoakes commented Jul 9, 2019

View reviewed changes

Add nullability annotations

8f73d75

drewnoakes force-pushed the annotations branch from d62bbe9 to 8f73d75 Compare July 9, 2019 03:43

jmarolf reviewed Jul 9, 2019

View reviewed changes

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/FaultExtensions.cs Outdated Show resolved Hide resolved

jmarolf reviewed Jul 9, 2019

View reviewed changes

...rojectSystem.Managed/ProjectSystem/LanguageServices/FSharp/FSharpCommandLineParserService.cs Outdated Show resolved Hide resolved

jmarolf reviewed Jul 9, 2019

View reviewed changes

...isualStudio.ProjectSystem.Managed/ProjectSystem/OnceInitializedOnceDisposedUnderLockAsync.cs Show resolved Hide resolved

jmarolf reviewed Jul 9, 2019

View reviewed changes

...ctSystem/LanguageServices/WorkspaceProjectContextHost.WorkspaceProjectContextHostInstance.cs Show resolved Hide resolved

drewnoakes added 4 commits July 9, 2019 16:37

Use extension method form

20fe1ca

Typo

2dc5875

Initialise ImportMany fields with empty arrays

5903f41

This allows marking the fields as non-nullable.

Wrap parameter lists

7de85aa

davkean reviewed Jul 10, 2019

View reviewed changes

...rojectSystem.Managed/ProjectSystem/LanguageServices/FSharp/FSharpCommandLineParserService.cs Outdated Show resolved Hide resolved

davkean approved these changes Jul 10, 2019

View reviewed changes

drewnoakes added 2 commits July 10, 2019 20:31

Merge remote-tracking branch 'upstream/master' into annotations

18c9fde

Don't initialise ImportMany fields

32a3001

Potentially masks bugs when the import fails.

jmarolf approved these changes Jul 11, 2019

View reviewed changes

Merge branch 'master' into annotations

4cb5325

drewnoakes merged commit 80d2b6e into dotnet:master Jul 11, 2019

drewnoakes deleted the annotations branch July 11, 2019 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate most of Microsoft.VisualStudio.ProjectSystem.Managed #5003

Annotate most of Microsoft.VisualStudio.ProjectSystem.Managed #5003

drewnoakes commented Jul 9, 2019

drewnoakes Jul 9, 2019

davkean Jul 10, 2019

drewnoakes Jul 9, 2019

drewnoakes Jul 9, 2019

jmarolf Jul 9, 2019

davkean Jul 10, 2019

drewnoakes Jul 9, 2019

jmarolf Jul 9, 2019

drewnoakes Jul 9, 2019

drewnoakes Jul 10, 2019

drewnoakes Jul 9, 2019

drewnoakes Jul 9, 2019

drewnoakes Jul 9, 2019

drewnoakes Jul 9, 2019

drewnoakes Jul 9, 2019

davkean Jul 10, 2019

drewnoakes Jul 9, 2019

davkean commented Jul 11, 2019

jmarolf commented Jul 11, 2019

Annotate most of Microsoft.VisualStudio.ProjectSystem.Managed #5003

Annotate most of Microsoft.VisualStudio.ProjectSystem.Managed #5003

Conversation

drewnoakes commented Jul 9, 2019

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

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

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

davkean commented Jul 11, 2019

jmarolf commented Jul 11, 2019