Annotate more public syntax APIs #44266

jcouv · 2020-05-14T19:09:28Z

No description provided.

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

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

src/Compilers/CSharp/Portable/Syntax/AnonymousMethodExpressionSyntax.cs

src/Compilers/CSharp/Portable/Syntax/DirectiveTriviaSyntax.cs

src/Compilers/CSharp/Portable/Syntax/SyntaxNodeRemover.cs

333fred · 2020-05-19T00:56:41Z

Done review pass (commit 2) #Resolved

jcouv · 2020-05-26T23:36:26Z

@333fred Please take another look when you get a chance. Thanks

333fred

LGTM (commit 3)

jcouv · 2020-05-27T05:45:28Z

Thanks Fred!

@dotnet/roslyn-compiler for a second review. Cheers

cston · 2020-06-17T23:48:56Z

src/Compilers/Core/Portable/Symbols/TypedConstant.cs

@@ -72,11 +72,11 @@ public bool IsNull
        /// <summary>
        /// The value for a non-array constant.
        /// </summary>
-        public object? Value
+        public object Value


public object Value [](start = 8, length = 19)

Isn't this null for the constant null? #Closed

Thanks. I'd misread the assertion in constructor.

In reply to: 441893234 [](ancestors = 441893234)

cston · 2020-06-17T23:49:08Z

    internal object ValueInternal

object?? #Closed

Refers to: src/Compilers/Core/Portable/Symbols/TypedConstant.cs:93 in d801f2a. [](commit_id = d801f2a, deletion_comment = False)

cston · 2020-06-18T00:12:59Z

src/Compilers/CSharp/Portable/Syntax/LambdaUtilities.cs

            var lambda = lambdaBody.Parent;
-            if (lambda.Kind() == SyntaxKind.ArrowExpressionClause)
+            if (lambda.IsKind(SyntaxKind.ArrowExpressionClause))


IsKind( [](start = 23, length = 7)

Just curious - why change these? #Resolved

The lambda.Kind() on line 63 would warn. Then I think I just aligned the check on line 57 for stylistic consistency.

In reply to: 441899787 [](ancestors = 441899787)

cston · 2020-06-18T00:21:48Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNodeRemover.cs

@@ -264,7 +268,7 @@ public override SeparatedSyntaxList<TNode> VisitList<TNode>(SeparatedSyntaxList<
                        }
                        else
                        {
-                            visited = this.VisitListElement((TNode)item.AsNode());
+                            visited = this.VisitListElement((TNode?)item.AsNode());


(TNode?)item.AsNode() [](start = 60, length = 21)

Can we use node here? #Closed

cston · 2020-06-19T22:10:21Z

src/Compilers/Core/Portable/Symbols/TypedConstant.cs

@@ -131,7 +132,7 @@ internal T DecodeValue<T>(SpecialType specialType)
            return value;
        }

-        internal bool TryDecodeValue<T>(SpecialType specialType, [MaybeNull][NotNullWhen(returnValue: true)] out T value)
+        internal bool TryDecodeValue<T>(SpecialType specialType, [MaybeNull][NotNullWhen(true)] out T value)


[NotNullWhen(true)] [](start = 76, length = 19)

What if value == null? #Closed

cston · 2020-06-19T22:10:48Z

src/Compilers/Core/Portable/Symbols/TypedConstant.cs

@@ -141,7 +142,8 @@ internal bool TryDecodeValue<T>(SpecialType specialType, [MaybeNull][NotNullWhen

            if (_type.SpecialType == specialType || (_type.TypeKind == TypeKind.Enum && specialType == SpecialType.System_Enum))
            {
-                value = (T)_value!;
+                Debug.Assert(_value is object);


Debug.Assert(_value is object); [](start = 16, length = 31)

Don't we get here with a constant null? #Closed

cston · 2020-06-19T22:13:16Z

src/Compilers/Core/Portable/Symbols/Attributes/CommonAttributeData.cs

@@ -285,11 +285,13 @@ private ObsoleteAttributeData DecodeObsoleteAttribute()
                // ObsoleteAttribute(string, bool)

                Debug.Assert(args.Length <= 2);
-                message = (string)args[0].ValueInternal;
+                Debug.Assert(args[0].ValueInternal is object);


Debug.Assert(args[0].ValueInternal is object); [](start = 16, length = 46)

Could the value be [Obsolete(null)]? #Closed

cston · 2020-06-19T22:15:49Z

src/Compilers/Core/Portable/Symbols/Attributes/CommonAttributeData.cs

@@ -341,8 +343,10 @@ private ObsoleteAttributeData DecodeDeprecatedAttribute()
                // DeprecatedAttribute(String, DeprecationType, UInt32, Platform) 
                // DeprecatedAttribute(String, DeprecationType, UInt32, String) 

-                message = (string)args[0].ValueInternal;
-                isError = ((int)args[1].ValueInternal == 1);
+                Debug.Assert(args[0].ValueInternal is object);


Debug.Assert(args[0].ValueInternal is object) [](start = 16, length = 45)

Could the value be [Deprecated(null, ...)]? #Closed

cston · 2020-06-19T22:19:07Z

src/Compilers/CSharp/Portable/Symbols/TypedConstantExtensions.cs


 namespace Microsoft.CodeAnalysis.CSharp
 {
    public static class TypedConstantExtensions
    {
        /// <summary>
        /// Returns the System.String that represents the current TypedConstant.
+        /// For testing and debugging only


For testing and debugging only [](start = 12, length = 30)

This method is part of the public API so I don't think we can state how this method is used. #Closed

Thanks

In reply to: 443063564 [](ancestors = 443063564)

cston · 2020-06-19T22:21:10Z

src/Compilers/CSharp/Portable/Symbols/TypedConstantExtensions.cs

@@ -140,7 +138,10 @@ private static string DisplayUnsignedEnumConstant(TypedConstant constant, Specia
            }

            // Unable to decode the enum constant, just display the integral value
-            return constant.ValueInternal.ToString();
+            Debug.Assert(constant.ValueInternal is object);


Debug.Assert(constant.ValueInternal is object) [](start = 12, length = 46)

Why assert rather than using !? #ByDesign

Because this is the result of a check in a calling method (if (constant.IsNull) in ToCSharpString), but not in the present method.

In reply to: 443063994 [](ancestors = 443063994)

cston · 2020-06-19T22:21:30Z

src/Compilers/CSharp/Portable/Symbols/TypedConstantExtensions.cs

@@ -213,7 +214,10 @@ private static string DisplaySignedEnumConstant(TypedConstant constant, SpecialT
            }

            // Unable to decode the enum constant, just display the integral value
-            return constant.ValueInternal.ToString();
+            Debug.Assert(constant.ValueInternal is object);


Debug.Assert(constant.ValueInternal is object); [](start = 12, length = 47)

Why assert rather than using !? #ByDesign

Same rationale

In reply to: 443064067 [](ancestors = 443064067)

cston · 2020-06-20T03:45:59Z

src/Compilers/Core/Portable/Symbols/Attributes/CommonAttributeData.cs

-                message = (string)args[0].ValueInternal;
-                isError = ((int)args[1].ValueInternal == 1);
+                Debug.Assert(args[1].ValueInternal is object);
+                message = (string?)args[0].ValueInternal!;


! [](start = 56, length = 1)

! is incorrect. #Closed

cston · 2020-06-20T19:39:38Z

src/Compilers/Core/Portable/XmlFileResolver.cs

@@ -48,7 +48,7 @@ public XmlFileResolver(string? baseDirectory)
        /// If <paramref name="baseFilePath"/> is relative <see cref="BaseDirectory"/> is used as the base path of <paramref name="baseFilePath"/>.
        /// </param>
        /// <returns>Normalized XML document file path or null if not found.</returns>
-        public override string? ResolveReference(string path, string? baseFilePath)
+        public override string? ResolveReference(string? path, string? baseFilePath)


string? path [](start = 49, length = 12)

Why is path optional? It looks like the uses of path support null values, but allowing null seems incorrect given the parameter description where it is a path to a source file. #Resolved

I'm trying to capture/document the existing behavior of this method.
The test I added does exercise this path. I can adjust the caller instead (don't call if path is null), but result is same.

In reply to: 443154834 [](ancestors = 443154834)

cston · 2020-06-20T19:40:03Z

src/Compilers/Core/Portable/Compilation/XmlReferenceResolver.cs

@@ -27,7 +27,7 @@ protected XmlReferenceResolver()
        /// <param name="path">The reference path to resolve. May be absolute or relative path.</param>
        /// <param name="baseFilePath">Path of the source file that contains the <paramref name="path"/> (may also be relative), or null if not available.</param>
        /// <returns>Path to the XML artifact, or null if the file can't be resolved.</returns>
-        public abstract string? ResolveReference(string path, string? baseFilePath);
+        public abstract string? ResolveReference(string? path, string? baseFilePath);


string? path [](start = 49, length = 12)

Why is path optional? #Resolved

Annotate more syntax APIs

c7daeb1

jcouv added Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types labels May 14, 2020

jcouv self-assigned this May 14, 2020

Tweaks

d8108db

jcouv marked this pull request as ready for review May 15, 2020 18:06

jcouv requested review from a team as code owners May 15, 2020 18:06