IFixedStatement API Review

[jinujoseph]

https://github.com/dotnet/roslyn/blob/features/ioperation/src/Compilers/Core/Portable/Operations/IFixedStatement.cs

We should remove this for V1

[333fred]

I'm going to make the existing IFixedStatement APIs internal, and make the OperationFactories return NoneOperation for FixedStatements. I'm leaving them in place, however, as the API will likely be fine when we get to unsafe code. This bug will track reviewing the API and re-publicizing IFixedStatement.

[siegfriedpammer]

We should remove this for V1

Now three major releases have passed and still there's no sign of this being made available publicly. Is there anything contributors could do to make this happen?

[CyrusNajmabadi]

@siegfriedpammer Designing a suitable API for review would be helpful. Thanks!

[333fred]

Yes: while I am not opposed to reviewing such a proposal, the general thing is that we've gone 3 major releases and you are the first person that has even asked us for the API. We haven't prioritized it for that reason :).

[siegfriedpammer]

To be honest, I haven't had a use-case for the IFixedOperation yet, so I am not the right person to design an API. However, I am using OperationVisitor and I currently have to do some dirty hacks (going back to the AST and forth) to handle IOperation trees that contain fixed statements and the "address of" operator (IAddressOfOperation). Currently I am seeing the behavior that, even though I have a VisitChildren method that loops through all items in the IOperation.Children collection, this seems to not work as soon as a NoneOperation is involved.

The more I think about it, I guess for me a VisitNoneOperation method in the OperationVisitor would suffice to be able to normally traverse the full tree. Of course, I will open a new issue with my proposal, if necessary.

[siegfriedpammer]

Addendum, I just discovered that there already is such a method, but it's internal. Any chance, this could be changed?

[333fred]

Addendum, I just discovered that there already is such a method, but it's internal. Any chance, this could be changed?

I don't think this is super likely. We could, but the general goal of NoneOperations is to remove them. However, I wouldn't be opposed to discussing it in API review and seeing what @AlekseyTs or @mavasani thinks.

[AlekseyTs]

@siegfriedpammer

Currently I am seeing the behavior that, even though I have a VisitChildren method that loops through all items in the IOperation.Children collection, this seems to not work as soon as a NoneOperation is involved.

Could you please elaborate on this? Perhaps with specific example?

[siegfriedpammer]

@AlekseyTs here you go:

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.CodeAnalysis.Text;
using System.Diagnostics;

string source = @"

unsafe class C
{
    int field;

    void Use()
    {
        fixed (int *ptr = &field)
        {

        }
    }
}
";

var ast = CSharpSyntaxTree.ParseText(SourceText.From(source));
var options = new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary, allowUnsafe: true);
var compilation = CSharpCompilation.Create("MyAssembly").AddSyntaxTrees(ast).AddReferences(MetadataReference.CreateFromFile(typeof(object).Assembly.Location)).WithOptions(options);

var diags = compilation.GetDiagnostics();
Debug.Assert(diags.Length == 0);

var method = ast.GetRoot()
    .FindNode(TextSpan.FromBounds(source.IndexOf("fixed"), source.IndexOf("fixed") + 10))
    .FirstAncestorOrSelf<MethodDeclarationSyntax>();
var operationTree = compilation.GetSemanticModel(ast).GetOperation(method!.Body!)!;

operationTree.Accept(new TestOperationVisitor(), null);

class TestOperationVisitor : OperationVisitor<object?, bool>
{
    public override bool DefaultVisit(IOperation operation, object? argument)
    {
        Console.WriteLine("Visiting " + operation.GetType().FullName);
        return VisitChildren(operation, argument);
    }

    private bool VisitChildren(IOperation operation, object? argument)
    {
        foreach (var child in operation.Children)
        {
            Console.WriteLine("\tTrying to visit " + child.GetType().FullName);
            child.Accept(this, argument);
        }
        return false;
    }

    public override bool VisitFieldReference(IFieldReferenceOperation operation, object? argument)
    {
        // this is never executed, because the internal VisitFixedOperation/VisitNoneOperation does not call DefaultVisit!
        Console.WriteLine("Found field reference " + operation.Field); 
        return base.VisitFieldReference(operation, argument);
    }
}

I would expect VisitFieldReference to be called for the use of field inside the fixed statement's "variable initializer". This assumption is simply derived from the nature of the visitor pattern.

I am sure you will agree that relying on the IOperation tree has its benefits and not having to switch back and forth between AST and IOperation makes the code easier to understand.

you are the first person that has even asked us for the API.

@333fred A similar request has been made in #27036...

The main motivation for using the IOperation tree instead of the AST is that the IOperation tree combines some of the syntax node into a single operation, such as the different kinds of anonymous function/lambda syntaxes or if statements and ternary operators and the like. Also there is a IDelegateCreationOperation which is syntactically not different from an ObjectCreationExpression.

If you want me to make an API proposal for IFixedOperation, then I will have to come up with something, but as I don't have a proper use case to validate the API against, this is very difficult for me.

Thank you for your understanding!

[AlekseyTs]

I didn't dig deep, but it looks to me that the problem with visiting NoneOperation is the fact that VisitNoneOperation does nothing by default and cannot be overridden by external consumers:

    public abstract partial class OperationVisitor<TArgument, TResult>
    {
        public virtual TResult? DefaultVisit(IOperation operation, TArgument argument) => default(TResult);
        internal virtual TResult? VisitNoneOperation(IOperation operation, TArgument argument) => default(TResult);
    }

The specific problem with visiting IFixedOperation is that we delegate its visiting to VisitNoneOperation:

    public abstract partial class OperationVisitor<TArgument, TResult>
    {
        internal virtual TResult? VisitFixed(IFixedOperation operation, TArgument argument) =>
            VisitNoneOperation(operation, argument);
    }

Note that we also expose this walker that overrides VisitNoneOperation as follows:

    public abstract class OperationWalker<TArgument> : OperationVisitor<TArgument, object?>
    {
        internal override object? VisitNoneOperation(IOperation operation, TArgument argument)
        {
            VisitChildrenOperations(operation, argument);
            return null;
        }
    }

Perhaps we could consider changing default behavior of VisitNoneOperation to calling DefaultVisit? If this change feels too big/risky, we could consider adjusting implementation of VisitFixed to do that.

If you want me to make an API proposal for IFixedOperation, then I will have to come up with something, but as I don't have a proper use case to validate the API against, this is very difficult for me.

It looks like we already have a preliminary shape for IFixedOperation and even tests for it. We probably could simply go through reviewing the shape. Before doing that though, it would be good if someone could confirm that the shape is able to represent all flavors of fixed statement that C# supports right now.

[siegfriedpammer]

Note that we also expose this walker that overrides VisitNoneOperation as follows:

Would this solution be applicable to the other implementations of the visitor? I am using OperationVisitor<GeneratorContext, ISymbol?>. I realize that this is potentially a breaking change. how about introducing a new abstract class "DefaultOperationVisitor" that calls DefaultVisit in all Visit-overrides?

Before doing that though, it would be good if someone could confirm that the shape is able to represent all flavors of fixed statement that C# supports right now.

I am probably not the right person for this task, because I am not yet up to date with all the different flavors of the fixed statement added in more recent versions of the language. If I have the time, I will take a look.