Roslyn: "Make readonly" doesn't appear for some simple cases

[kuhlenh]

'Make readonly’ doesn't seem to work for a really simple case:

public static class Function1
    {

        private static HttpClient client = new HttpClient();

        private static IFaceServiceClient faceServiceClient = new FaceServiceClient("key", "https://westus.api.cognitive.microsoft.com/face/v1.0");

        public static IEnumerable<FaceAttributeType> faceAttributes = new FaceAttributeType[] { FaceAttributeType.Gender, FaceAttributeType.Age, FaceAttributeType.Smile, FaceAttributeType.Emotion, FaceAttributeType.Glasses, FaceAttributeType.Hair };

        [FunctionName("Function1")]
        public static async Task Run([BlobTrigger("samples-workitems/{name}", Connection = "")]Stream myBlob, string name, TraceWriter log)
        {
            try
            {
                Face[] faces = await faceServiceClient.DetectAsync(myBlob, returnFaceId: true, returnFaceLandmarks: false, returnFaceAttributes: faceAttributes);

                foreach (Face face in faces)
                {
                    var emotionScores = face.FaceAttributes.Emotion.Happiness;
                }
            }
            catch (FaceAPIException f)
            {
                log.Info($"{f.ErrorMessage}, {f.ErrorCode}");
            }
            catch (Exception e)
            {
                log.Info($"{e.Message}");
            }
        }
    }

• “Make readonly” appears for the HttpClient
• “Make readonly” does not appear for IFaceServiceClient

[CyrusNajmabadi]

I see it:

[sharwell]

@kuhlenh It seems the code snippet to reproduce this is incomplete.

[kuhlenh]

Figured it out. It's the try-catch. Updated the original repro

[CyrusNajmabadi]

Much simpler repro:

```c#
interface IFaceServiceClient
{
void DetectAsync();
}

public class Repro
{
private static IFaceServiceClient faceServiceClient = null;

public static void Run()
{
    faceServiceClient.DetectAsync();
}

}
```

[CyrusNajmabadi]

From looking at the source, i think the problem is:

```c#
if (!IsMemberOfThisInstance(descendant))
{
unassignedSymbols.Remove(symbol);
}

...
protected override bool IsMemberOfThisInstance(SyntaxNode node)
{
// if it is a qualified name, make sure it is this.name
if (node.Parent is MemberAccessExpressionSyntax memberAccess)
{
return memberAccess.Expression is ThisExpressionSyntax;
}
```

This is clearly bogus. We only want to do this check if we're the name of the member access expression. The way the code is written now, if the field is on the left of a dot, it won't ever be made readonly.

[CyrusNajmabadi]

Most of the code here seems completely wrong. There's also this check:

c# // make sure it isn't in an object initializer if (node.Parent.Parent is InitializerExpressionSyntax) { return false; }

This seems like it may also be wrong if you're on the initializer, and not the name, of an object initializer.

[Neme12]

I would just wait to fix this as part of #25652

[sharwell]

@Neme12 There are some major hurdles (performance related) that could stall #25652. If this is an easy fix it could certainly be relevant in the 15.8 time frame.

[CyrusNajmabadi]

@sharwell What are the major hurdles? Is it IOperation related? I used IOperation in several recent PRs and I'm curious if htat will be a problem.

[Neme12]

Ok, I thought #25652 was for 15.8