Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make SourceClonedParameterSymbol abstract #54355

Merged
merged 13 commits into from
Jul 13, 2021
Merged

Make SourceClonedParameterSymbol abstract #54355

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
55b6ebe
Make SourceClonedParameterSymbol abstract
Youssef1313 Jun 24, 2021
034ef63
Fix implementation and add tests
Youssef1313 Jun 28, 2021
5bc7e09
Return -1 for now
Youssef1313 Jun 28, 2021
62f83f1
Merge remote-tracking branch 'upstream/features/caller-argument-expre…
Youssef1313 Jun 29, 2021
a53e766
Add test
Youssef1313 Jun 29, 2021
502df54
Delete PROTOTYPE comment
Youssef1313 Jun 29, 2021
25dd0ff
Add test
Youssef1313 Jun 30, 2021
8ee88e0
Two more tests
Youssef1313 Jun 30, 2021
e14db71
Address feedback
Youssef1313 Jul 1, 2021
0701455
Fix diagnostic comment
Youssef1313 Jul 1, 2021
d0fdc95
Address requested test change
Youssef1313 Jul 3, 2021
447c1ba
Update src/Compilers/CSharp/Portable/Symbols/Source/SourceDelegateClo…
Youssef1313 Jul 7, 2021
63531dc
Update src/Compilers/CSharp/Portable/Symbols/Source/SourceDelegateClo…
Youssef1313 Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 2 additions & 36 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceClonedParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,32 +4,27 @@

#nullable disable

using System;
using System.Collections.Immutable;
using System.Diagnostics;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{

// PROTOTYPE(caller-expr): Make SourceClonedParameterSymbol abstract and introduce new specialized types (same as being done in VB).

/// <summary>
/// Represents a source parameter cloned from another <see cref="SourceParameterSymbol"/>, when they must share attribute data and default constant value.
/// For example, parameters on a property symbol are cloned to generate parameters on accessors.
/// Similarly parameters on delegate invoke method are cloned to delegate begin/end invoke methods.
/// </summary>
internal sealed class SourceClonedParameterSymbol : SourceParameterSymbolBase
internal abstract class SourceClonedParameterSymbol : SourceParameterSymbolBase
{
// if true suppresses params-array and default value:
private readonly bool _suppressOptional;

private readonly SourceParameterSymbol _originalParam;
protected readonly SourceParameterSymbol _originalParam;

internal SourceClonedParameterSymbol(SourceParameterSymbol originalParam, Symbol newOwner, int newOrdinal, bool suppressOptional)
: base(newOwner, newOrdinal)
{
Debug.Assert((object)originalParam != null);

_suppressOptional = suppressOptional;
_originalParam = originalParam;
}
Expand Down Expand Up @@ -76,15 +71,6 @@ internal override ConstantValue DefaultValueFromAttributes
get { return _originalParam.DefaultValueFromAttributes; }
}

internal override ParameterSymbol WithCustomModifiersAndParams(TypeSymbol newType, ImmutableArray<CustomModifier> newCustomModifiers, ImmutableArray<CustomModifier> newRefCustomModifiers, bool newIsParams)
{
return new SourceClonedParameterSymbol(
_originalParam.WithCustomModifiersAndParamsCore(newType, newCustomModifiers, newRefCustomModifiers, newIsParams),
this.ContainingSymbol,
this.Ordinal,
_suppressOptional);
}

#region Forwarded

public override TypeWithAnnotations TypeWithAnnotations
Expand Down Expand Up @@ -142,26 +128,6 @@ internal override bool IsIUnknownConstant
get { return _originalParam.IsIUnknownConstant; }
}

internal override bool IsCallerFilePath
{
get { return _originalParam.IsCallerFilePath; }
}

internal override bool IsCallerLineNumber
{
get { return _originalParam.IsCallerLineNumber; }
}

internal override bool IsCallerMemberName
{
get { return _originalParam.IsCallerMemberName; }
}

internal override int CallerArgumentExpressionParameterIndex
{
get { return _originalParam.CallerArgumentExpressionParameterIndex; }
}

internal override FlowAnalysisAnnotations FlowAnalysisAnnotations
{
get { return FlowAnalysisAnnotations.None; }
Expand Down
36 changes: 36 additions & 0 deletions ...CSharp/Portable/Symbols/Source/SourceDelegateClonedParameterSymbolForBeginAndEndInvoke.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Immutable;
using System.Diagnostics;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
internal sealed class SourceDelegateClonedParameterSymbolForBeginAndEndInvoke : SourceClonedParameterSymbol
{
internal SourceDelegateClonedParameterSymbolForBeginAndEndInvoke(SourceParameterSymbol originalParam, SourceDelegateMethodSymbol newOwner, int newOrdinal)
: base(originalParam, newOwner, newOrdinal, suppressOptional: true)
{
333fred marked this conversation as resolved.
Show resolved Hide resolved
}

internal override bool IsCallerFilePath => _originalParam.IsCallerFilePath;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand why properties that are implemented the same way in SourceDelegateClonedParameterSymbolForBeginAndEndInvoke and SourcePropertyClonedParameterSymbolForAccessors were moved out of the base type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcouv This was requested by @AlekseyTs. The reasoning is to force new types to implement it. i.e, avoid future mistakes if a new type should implement it another way.


internal override bool IsCallerLineNumber => _originalParam.IsCallerLineNumber;

internal override bool IsCallerMemberName => _originalParam.IsCallerMemberName;

// From code review: Could we just return -1 for now and see if anyone would ask us to support the scenarios.
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
internal override int CallerArgumentExpressionParameterIndex => -1;
333fred marked this conversation as resolved.
Show resolved Hide resolved

internal override ParameterSymbol WithCustomModifiersAndParams(TypeSymbol newType, ImmutableArray<CustomModifier> newCustomModifiers, ImmutableArray<CustomModifier> newRefCustomModifiers, bool newIsParams)
{
return new SourceDelegateClonedParameterSymbolForBeginAndEndInvoke(
_originalParam.WithCustomModifiersAndParamsCore(newType, newCustomModifiers, newRefCustomModifiers, newIsParams),
(SourceDelegateMethodSymbol)ContainingSymbol,
Ordinal);
}
}
}
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceDelegateMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ internal BeginInvokeMethod(
var parameters = ArrayBuilder<ParameterSymbol>.GetInstance();
foreach (SourceParameterSymbol p in invoke.Parameters)
{
var synthesizedParam = new SourceClonedParameterSymbol(originalParam: p, newOwner: this, newOrdinal: p.Ordinal, suppressOptional: true);
var synthesizedParam = new SourceDelegateClonedParameterSymbolForBeginAndEndInvoke(originalParam: p, newOwner: this, newOrdinal: p.Ordinal);
parameters.Add(synthesizedParam);
}

Expand Down Expand Up @@ -403,7 +403,7 @@ internal EndInvokeMethod(
{
if (p.RefKind != RefKind.None)
{
var synthesizedParam = new SourceClonedParameterSymbol(originalParam: p, newOwner: this, newOrdinal: ordinal++, suppressOptional: true);
var synthesizedParam = new SourceDelegateClonedParameterSymbolForBeginAndEndInvoke(originalParam: p, newOwner: this, newOrdinal: ordinal++);
parameters.Add(synthesizedParam);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,7 @@ private ImmutableArray<ParameterSymbol> ComputeParameters(BindingDiagnosticBag d
// since the ContainingSymbol needs to be set to the accessor.
foreach (SourceParameterSymbol propertyParam in propertyParameters)
{
parameters.Add(new SourceClonedParameterSymbol(propertyParam, this, propertyParam.Ordinal, suppressOptional: false));
parameters.Add(new SourcePropertyClonedParameterSymbolForAccessors(propertyParam, this));
}

if (!isGetMethod)
Expand Down
32 changes: 32 additions & 0 deletions ...mpilers/CSharp/Portable/Symbols/Source/SourcePropertyClonedParameterSymbolForAccessors.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
internal sealed class SourcePropertyClonedParameterSymbolForAccessors : SourceClonedParameterSymbol
{
internal SourcePropertyClonedParameterSymbolForAccessors(SourceParameterSymbol originalParam, Symbol newOwner)
: base(originalParam, newOwner, originalParam.Ordinal, suppressOptional: false)
{
}

internal override bool IsCallerFilePath => _originalParam.IsCallerFilePath;

internal override bool IsCallerLineNumber => _originalParam.IsCallerLineNumber;

internal override bool IsCallerMemberName => _originalParam.IsCallerMemberName;

internal override int CallerArgumentExpressionParameterIndex => _originalParam.CallerArgumentExpressionParameterIndex;

internal override ParameterSymbol WithCustomModifiersAndParams(TypeSymbol newType, ImmutableArray<CustomModifier> newCustomModifiers, ImmutableArray<CustomModifier> newRefCustomModifiers, bool newIsParams)
{
return new SourcePropertyClonedParameterSymbolForAccessors(
_originalParam.WithCustomModifiersAndParamsCore(newType, newCustomModifiers, newRefCustomModifiers, newIsParams),
this.ContainingSymbol);
}
}
}
203 changes: 203 additions & 0 deletions src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,209 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests
{
public class AttributeTests_CallerInfoAttributes : WellKnownAttributesTestBase
{
[ConditionalFact(typeof(CoreClrOnly))]
public void TestBeginInvoke()
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestBeginInvoke

Do we have similar tests for EndInvoke? Even though they are likely going to be error scenarios, we want to make sure that binding doesn't fail in an unexpected way (a crash/assert, etc. ) #Closed

{
string source = @"
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System.Runtime.InteropServices
{
[AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
public sealed class OptionalAttribute : Attribute
{
public OptionalAttribute()
{
}
}

[AttributeUsage(AttributeTargets.Parameter)]
public sealed class DefaultParameterValueAttribute : Attribute
{
public DefaultParameterValueAttribute(object value)
{
Value = value;
}
public object Value { get; }
}
}

class Program
{
const string s1 = nameof(s1);
delegate void D(string s1, [CallerArgumentExpression(s1)] [Optional] [DefaultParameterValue(""default"")] string s2);

static void M(string s1, string s2)
{
}

static string GetString() => null;

public static void Main()
{
D d = M;
d.BeginInvoke(GetString(), callback: null, @object: null);
}
}
";

var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
// Begin/EndInvoke are not currently supported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supported

Do you mean at runtime? Supported where?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@333fred No, I meant "supported by the compiler". ie, the compiler doesn't do anything for caller argument expressions for cloned begin/end invoke parameters.

CompileAndVerify(compilation).VerifyDiagnostics().VerifyIL("Program.Main", @"
{
// Code size 31 (0x1f)
.maxstack 5
IL_0000: ldnull
IL_0001: ldftn ""void Program.M(string, string)""
IL_0007: newobj ""Program.D..ctor(object, System.IntPtr)""
IL_000c: call ""string Program.GetString()""
IL_0011: ldstr ""default""
IL_0016: ldnull
IL_0017: ldnull
IL_0018: callvirt ""System.IAsyncResult Program.D.BeginInvoke(string, string, System.AsyncCallback, object)""
IL_001d: pop
IL_001e: ret
}
");
}

[ConditionalFact(typeof(CoreClrOnly))]
public void TestEndInvoke()
{
string source = @"
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System.Runtime.InteropServices
{
[AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
public sealed class OptionalAttribute : Attribute
{
public OptionalAttribute()
{
}
}

[AttributeUsage(AttributeTargets.Parameter)]
public sealed class DefaultParameterValueAttribute : Attribute
{
public DefaultParameterValueAttribute(object value)
{
Value = value;
}
public object Value { get; }
}
}

class Program
{
const string s1 = nameof(s1);
delegate void D(ref string s1, [CallerArgumentExpression(s1)] [Optional] [DefaultParameterValue(""default"")] string s2);
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CallerArgumentExpression(s1)] [Optional] [DefaultParameterValue(""default"")] string s2

The interesting scenario is when this parameter is getting cloned and the one that it references isn't and probably follows it. Is this the case in this unit-test. We want to test scenario where simply reusing the index from the original parameter will be observably wrong. #Closed

Copy link
Member Author

@Youssef1313 Youssef1313 Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interesting scenario is when this parameter is getting cloned and the one that it references isn't

For the parameter to get cloned, it must be ref if I understand correctly. ref parameters seem to have a false IsOptional. Hence, the test won't hit calling CallerArgumentExpressionIndex. This is regardless we're in error case or not.

I think the test you're asking for should hit CallerArgumentExpressionIndex. I'm unable to find a test case that fulfills all the following:

  1. The CallerArgumentExpression parameter is cloned.
  2. The parameter it refers to isn't cloned.
  3. CallerArgumentExpressionIndex gets hit.

I added a test that fulfills 1 and 2.

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the parameter to get cloned, it must be ref if I understand correctly. ref parameters seem to have a false IsOptional. Hence, the test won't hit calling CallerArgumentExpressionIndex. This is regardless we're in error case or not.

We still can have a test attempting to do that. Trying all possible ways marking the parameter optional, attempting to ommit it at the call-site. It is fine if we will be unable to hit the API. We still need tests that attempt to do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlekseyTs I added a test in 25dd0ff, and added two more now in 8ee88e0. Let me know if there are any other scenarios :)


static void M(ref string s1, string s2)
{
}

public static void Main()
{
D d = M;
string s = string.Empty;
d.EndInvoke(ref s, null);
}
}
";

var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
// Begin/EndInvoke are not currently supported.
CompileAndVerify(compilation).VerifyDiagnostics().VerifyIL("Program.Main", @"
{
// Code size 27 (0x1b)
.maxstack 3
.locals init (string V_0) //s
IL_0000: ldnull
IL_0001: ldftn ""void Program.M(ref string, string)""
IL_0007: newobj ""Program.D..ctor(object, System.IntPtr)""
IL_000c: ldsfld ""string string.Empty""
IL_0011: stloc.0
IL_0012: ldloca.s V_0
IL_0014: ldnull
IL_0015: callvirt ""void Program.D.EndInvoke(ref string, System.IAsyncResult)""
IL_001a: ret
}
");
}

[ConditionalFact(typeof(CoreClrOnly))]
public void TestBeginInvoke_ReferringToCallbackParameter()
{
string source = @"
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System.Runtime.InteropServices
{
[AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
public sealed class OptionalAttribute : Attribute
{
public OptionalAttribute()
{
}
}

[AttributeUsage(AttributeTargets.Parameter)]
public sealed class DefaultParameterValueAttribute : Attribute
{
public DefaultParameterValueAttribute(object value)
{
Value = value;
}
public object Value { get; }
}
}

class Program
{
const string callback = nameof(callback);
delegate void D(string s1, [CallerArgumentExpression(callback)] [Optional] [DefaultParameterValue(""default"")] string s2);

static void M(string s1, string s2)
{
}

static string GetString() => null;

public static void Main()
{
D d = M;
d.BeginInvoke(GetString(), callback: null, @object: null);
}
}
";

var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
CompileAndVerify(compilation).VerifyDiagnostics(
// (29,33): warning CS9005: The CallerArgumentExpressionAttribute applied to parameter 's2' will have no effect. It is applied with an invalid parameter name.
// delegate void D(string s1, [CallerArgumentExpression(callback)] [Optional] [DefaultParameterValue("default")] string s2);
Diagnostic(ErrorCode.WRN_CallerArgumentExpressionAttributeHasInvalidParameterName, "CallerArgumentExpression").WithArguments("s2").WithLocation(29, 33)
).VerifyIL("Program.Main", @"
{
// Code size 31 (0x1f)
.maxstack 5
IL_0000: ldnull
IL_0001: ldftn ""void Program.M(string, string)""
IL_0007: newobj ""Program.D..ctor(object, System.IntPtr)""
IL_000c: call ""string Program.GetString()""
IL_0011: ldstr ""default""
IL_0016: ldnull
IL_0017: ldnull
IL_0018: callvirt ""System.IAsyncResult Program.D.BeginInvoke(string, string, System.AsyncCallback, object)""
IL_001d: pop
IL_001e: ret
}
");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the added tests cover delegate scenarios. Do we already have scenarios for properties/indexers, since that's another scenario that uses SourceClonedParameterSymbol?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcouv There is a test for indexer:

roslyn/src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs

Line 1685 in 63531dc

public void TestIndexers()

Let me know if you want to extend the tests with any scenarios.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks


#region CallerArgumentExpression - Invocations
[ConditionalFact(typeof(CoreClrOnly))]
public void TestGoodCallerArgumentExpressionAttribute()
Expand Down