Create attribute default arguments during binding

[RikkiGibson]

Related to #47352

Before this change we had a dedicated code path for creating default arguments in attributes, separate from the path that creates default arguments everywhere else. This change consolidates us to the single path which creates BoundExpressions instead of TypedConstants.

#53618 becomes a lot simpler to implement and more similar to operation creation for calls after this change.

[Youssef1313]

Symbol? attributedMember = null

Are we sure that callers not passing this parameter work properly? For example CSharpSemanticModel.GetSpeculativeSymbolInfo(int position, AttributeSyntax attribute) comes across this code path. This parameter as far as I understand affects CallerMemberName which I think there is no speculative semantic model tests for.

There is a second caller in AttributeSemanticModel that needs to be revised, I think.

---

e.g:

            var tree = SyntaxFactory.ParseSyntaxTree(@"
using System;
using System.Runtime.CompilerServices;

public class MyAttribute : Attribute
{
    public MyAttribute([CallerMemberName] string caller = null)
    {
    }
}

[Obsolete]
public class C
{
}
");
            // Writing in a clean console app so CreateCompilation helper of unit tests not available.
            var comp = CSharpCompilation.Create("", new[] { tree }, new[] { MetadataReference.CreateFromFile(typeof(CallerMemberNameAttribute).Assembly.Location) });
            var model = comp.GetSemanticModel(tree);
            var newAttributeSyntax = SyntaxFactory.ParseCompilationUnit("[My]class C { }").DescendantNodes().OfType<AttributeSyntax>().Single();
            var speculativeInfo = model.GetSpeculativeSymbolInfo(tree.GetRoot().DescendantNodes().First(n => n.ToString() == "[Obsolete]").SpanStart, newAttributeSyntax);
            Console.WriteLine(speculativeInfo.Symbol.ToDisplayString()); // prints MyAttribute.MyAttribute(string)

More extensive tests may be needed

[RikkiGibson]

I wasn't able to come up with a speculative test which observes the value of implicit default arguments before this change. After this change, we pass the following test:

        [Fact]
        public void TestAttributeCallerInfoSemanticModel_Speculative()
        {
            var source = @"
using System;
using System.Runtime.CompilerServices;

class Attr : Attribute { public Attr([CallerMemberName] string s = null) { } }

class C
{
    [Attr(""a"")]
    void M0() { }
}
";
            var comp = CreateCompilation(source);
            comp.VerifyDiagnostics();

            var tree = comp.SyntaxTrees[0];
            var root = tree.GetRoot();
            var attrSyntax = root.DescendantNodes().OfType<AttributeSyntax>().Last();

            var semanticModel = comp.GetSemanticModel(tree);

            var newRoot = root.ReplaceNode(attrSyntax, attrSyntax.WithArgumentList(SyntaxFactory.ParseAttributeArgumentList("()")));
            var newAttrSyntax = newRoot.DescendantNodes().OfType<AttributeSyntax>().Last();

            Assert.True(semanticModel.TryGetSpeculativeSemanticModel(attrSyntax.ArgumentList.Position, newAttrSyntax, out var speculativeModel));

            var operation = speculativeModel.GetOperation(newAttrSyntax);
            VerifyOperationTree(comp, operation, @"
IOperation:  (OperationKind.None, Type: Attr) (Syntax: 'Attr()')
  Children(1):
      IDefaultValueOperation (OperationKind.DefaultValue, Type: System.String, Constant: null, IsImplicit) (Syntax: 'Attr()')");
        }

Before this PR, the 'None' operation has no children.
In other words, this implementation doesn't propagate the caller info properly. To do that, we would actually need to determine the attributedMember. Perhaps the implementation of IAttributeOperation would be a reasonable time to solve that.

[Youssef1313]

Getting IOperartion from speculative semantic model sounds interesting!

[RikkiGibson]

FWIW, I think it could be difficult to do properly. I don't know how the rest of the team feels about it but it's the kind of thing I think would be reasonable to file as a follow-up bug after initial implementation of IAttributeOperation. It seems like the use case for CallerInfo is at runtime. If you are writing a tool that uses IOperation, etc. there's no reason to dig into the default arguments to get member name, line number, etc. values, just use the compiler APIs directly at that point.

[cston]

(this as ContextualAttributeBinder)

Are there valid cases where the ContextualAttributeBinder is in the Next chain rather than this?

[RikkiGibson]

No, if a ContextualAttributeBinder is present, it will be at the top of the chain here. I did actually consider eliminating the type entirely but I was concerned it could result in too much additional churn.

[cston]

if a ContextualAttributeBinder is present, it will be at the top of the chain here

Consider asserting that.

Debug.Assert(this is ContextualAttributeBinder || !GetAllBinders().Any(this is ContextualAttributeBinder));

[RikkiGibson]

I wasn't able to write a reasonable assertion in a scenario like the following.

roslyn/src/Compilers/CSharp/Test/Semantic/Semantics/LambdaTests.cs

Lines 3736 to 3754 in 777f4e0

	[Fact]
	public void LambdaAttributes_06()
	{
	var source =
	@"using System;
	class AAttribute : Attribute
	{
	public AAttribute(Action a) { }
	}
	[A([B] () => { })]
	class BAttribute : Attribute
	{
	}";
	var comp = CreateCompilation(source, parseOptions: TestOptions.RegularPreview);
	comp.VerifyDiagnostics(
	// (6,2): error CS0181: Attribute constructor parameter 'a' has type 'Action', which is not a valid attribute parameter type
	// [A([B] () => { })]
	Diagnostic(ErrorCode.ERR_BadAttributeParamType, "A").WithArguments("a", "System.Action").WithLocation(6, 2));
	}

In this case, the BoundAttribute for [B] doesn't have errors, so there's not something conveniently available in scope for us to grab onto that lets us skip asserting that "ContextualAttributeBinder is only found on the top".

[RikkiGibson]

Also, I did try eliminating ContextualAttributeBinder, but I found we still depend on it in Binder.GetForwardedToAssembly to break cycles.

[cston]

rewrittenArguments = constructorArgsArray

Is it possible to get here with an attribute constructor with params argument and where the length of rewrittenArguments does not match the parameter length (say sharplab.io)? Do consumers of the SourceAttributeData expect the lengths to match?

[RikkiGibson]

I'll write a test against the compiler prior to this PR and ensure we haven't changed the behavior.

[RikkiGibson]

I added the test AttributeTests.TestBadParamsCtor to answer this. It looks like the arguments and parameters length are only required to match when the attribute doesn't have errors.

[cston]

&& Conversions.ClassifyBuiltInConversion(Compilation.GetSpecialType(SpecialType.System_String), parameterType, ref discardedUseSiteInfo).Exists

Are we testing this case?

[RikkiGibson]

IIRC, I added this bit to satisfy some existing CallerArgumentExpression tests--the attribute is not expected to work when the attributed parameter only has a user-defined conversion from string.

[cston]

&& containingMember is not null

Are we silently ignoring [CallerMemberName] for attributes?

[RikkiGibson]

We are not ignoring them in batch compilation scenarios, but we are in semantic model (IOperation) scenarios. See #59750 (comment)

[RikkiGibson]

@cston Thank you for the review. I believe I've addressed all your feedback.

[RikkiGibson]

Reviewers should be aware that the diff gets quite hard to read in this file from here on. You might be better off just separately reviewing the lines deleted and lines added due to large gaps in the regular split view.