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.

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

Support declaring and binding to methods with params Span<T> #59123

Merged
merged 10 commits into from
May 24, 2022

Conversation

cston
Copy link
Member

@cston cston commented Jan 27, 2022

Proposal: params-span.md

Initial feature changes:

  • Allow method declarations with params Span<T> and params ReadOnlySpan<T>
  • Allow calls in expanded form to methods declared with params Span<T> or params ReadOnlySpan<T>
  • Report -langversion errors
  • Allocate associated array at each callsite, with no sharing of array instances across calls
  • Prefer binding to overload with params Span<T> or params ReadOnlySpan<T> over params T[] in overload resolution; treat params Span<T> and params ReadOnlySpan<T> equally

Test plan #57049

@cston cston marked this pull request as ready for review January 28, 2022 00:53
@cston cston requested a review from a team as a code owner January 28, 2022 00:53
// PROTOTYPE: Test use-site diagnostics.
var useSiteInfo = new CompoundUseSiteInfo<AssemblySymbol>(_diagnostics, _compilation.Assembly);
var conversion = _compilation.Conversions.ClassifyImplicitConversionFromType(spanType, paramArrayType, ref useSiteInfo);
conversion.MarkUnderlyingConversionsCheckedRecursive();
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

MarkUnderlyingConversionsCheckedRecursive

It would be good to add a comment regarding why we think that checks performed by Binder do not have to be performed for this conversion. #Closed

return CreateParamArrayArgument(syntax, paramArrayType, arrayArgs, _compilation, this);
if (paramArrayType.IsSZArray())
{
return CreateParamArrayArgument(syntax, paramArrayType, arrayArgs, _compilation, this);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

CreateParamArrayArgument

There are several more call sites for this method. Should they be adjusted as well? Also, would it make sense to move the logic that deals with param array to initial binding as we have done for default values? #Closed

System_Span_T__get_Item,
System_Span_T__get_Length,
System_Span_T__op_Implicit_SpanReadOnlySpan,
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

System_Span_T__op_Implicit_SpanReadOnlySpan

Is this still used? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as we discussed offline, we'll use the well-known member for conversion.

var parameter = ((BoundParameter)expr).ParameterSymbol;
// params Span<T> value cannot be returned since that would
// prevent sharing repeated allocations at the call-site.
if (parameter.IsParams && !parameter.Type.IsSZArray())
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

IsParams

If I remember correctly, it used to be that this property on its own wasn't sufficient for a parameter to be treated as a "real" params parameter. The type and position was important as well. Also, even if parameter is a "real" params, it doesn't mean that Span<T> was created by compiler, a user-created span could be passed. Another thing to consider is the fact, that removing params is not a binary breaking change, but it looks like it is going to disable the safety check. Perhaps, it would be better to move the safety check to the consumer, where we know exactly what is going on rather than doing this at the definition site? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated check for params and added a PROTOTYPE comment to move the error reporting to the caller.

{
Debug.Assert((object)final == final.ContainingSymbol.GetParameters().Last());
return final.IsParams && ((ParameterSymbol)final.OriginalDefinition).Type.IsSZArray();
return final.IsParams && final.Type.IsParamsType(Compilation);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

final.Type

This doesn't look equivalent, the OriginalDefinition is dropped. #Closed

}
if (type is NamedTypeSymbol { Arity: 1 } namedType)
{
return namedType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0];
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

namedType

Consider adding an assert at least for type's name. #Closed

@@ -1638,9 +1638,9 @@ private TypeSymbol GetParameterType(ParameterSymbol parameter, MemberAnalysisRes
{
var type = parameter.Type;
if (result.Kind == MemberResolutionKind.ApplicableInExpandedForm &&
parameter.IsParams && type.IsSZArray())
parameter.IsParams && type.IsParamsType(Compilation))
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

parameter.IsParams

It looks like we are missing the ordinal check. #Closed

}
if (type is NamedTypeSymbol { Arity: 1 } namedType)
{
var constructedFrom = namedType.ConstructedFrom;
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

ConstructedFrom

It feels like this should be OriginalDefinition #Closed

@@ -507,15 +506,23 @@ private static void CheckParameterModifiers(BaseParameterSyntax parameter, Bindi
// error CS1100: Method '{0}' has a parameter modifier 'this' which is not on the first parameter
diagnostics.Add(ErrorCode.ERR_BadThisParam, thisKeyword.GetLocation(), owner.Name);
}
else if (parameter.IsParams && owner.IsOperator())
else if (parameter.IsParams)
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

else if (parameter.IsParams)

The refactoring doesn't look equivalent. It is disabling the following checks even if no errors reported. Consider moving this if to the end of the chain, or restructuring the code to perform the following checks unless an error is reported (perhaps ignoring the language version error). #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the following checks are not applicable for a params parameter if no errors are reported here, although that may not be the case in the future, so I've moved this block to the end.

// for 'stackalloc' in LocalRewriter.VisitStackAllocArrayCreationBase().)

// For now, simply allocate the array on the heap: new Span<T>(new T[length])
var intType = _compilation.GetSpecialType(SpecialType.System_Int32);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

GetSpecialType

We should use helper that reports diagnostics #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

We should use helper that reports diagnostics

This is marked as resolved, but I do not see any changes around reporting diagnostics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.


// For now, simply allocate the array on the heap: new Span<T>(new T[length])
var intType = _compilation.GetSpecialType(SpecialType.System_Int32);
var array = new BoundArrayCreation(
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

BoundArrayCreation

It looks like we could use CreateParamArrayArgument helper to create this array. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

CreateParamArrayArgument() creates an array and initializes the values. Ideally, the Span<T> here will be created through a BCL helper method so initialization will likely be handled separately from allocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

the Span here will be created through a BCL helper method so initialization will likely be handled separately from allocation.

When and if this happens, then we will adjust implementation accordingly. At the moment, however, I see no good reason adding all this complexity when we have a perfectly good reusable helper.

// ...
// temp[n - 1] = arrayArgs[n - 1];
// temp
for (int i = 0; i < arrayArgs.Length; i++)
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

for (int i = 0; i < arrayArgs.Length; i++)

I am not sure why go through all this trouble when we can initialize the allocated array directly and we already have a helper for that. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

To allow for allocation to be handled by a runtime helper method.

{
assignment = _factory.AssignmentExpression(
_factory.Call(temp, spanGetItem, MakeLiteral(syntax, ConstantValue.Create(i), intType, this)),
_factory.Convert(elementType, arrayArgs[i]));
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

_factory.Convert

Shouldn't the type match? #Closed

sideEffects.Add(assignment);
}

var expr = MakeConversionNode(syntax, temp, conversion, temp.Type, @checked: false);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2022

Choose a reason for hiding this comment

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

temp.Type

Shouldn't this be paramArrayType? #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 2), tests are not looked at.

@AlekseyTs
Copy link
Contributor

@cston It looks like there are legitimate test failures.

// prevent sharing repeated allocations at the call-site.
if (parameter.IsParams &&
parameter.Ordinal == parameter.ContainingSymbol.GetParameterCount() - 1 &&
!parameter.Type.IsSZArray())
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 7, 2022

Choose a reason for hiding this comment

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

!parameter.Type.IsSZArray()

If we are complaining about a span type, then we probably should check for that rather than checking that the type is not an array. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in error situations the type could be neither and the error will be unexpected and confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added PROTOTYPE comment.

PropertySymbol p => p.GetOwnOrInheritedGetMethod() ?? p.GetOwnOrInheritedSetMethod(),
_ => throw ExceptionUtilities.UnexpectedValue(methodOrIndexer),
};
return CreateParamArrayArgument(syntax, parameterType, arrayArgs, compilation, method, BindingDiagnosticBag.Discarded);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 8, 2022

Choose a reason for hiding this comment

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

BindingDiagnosticBag.Discarded

It is not clear why we are discarding diagnostics and dependencies. #Closed

PropertySymbol p => p.GetOwnOrInheritedGetMethod() ?? p.GetOwnOrInheritedSetMethod(),
_ => throw ExceptionUtilities.UnexpectedValue(methodOrIndexer),
};
return CreateParamArrayArgument(syntax, parameterType, arrayArgs, compilation, method, BindingDiagnosticBag.Discarded);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 8, 2022

Choose a reason for hiding this comment

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

method

I am not sure what we are doing here, the value in this local doesn't look like a containing method. #Closed

Children(4):
ISimpleAssignmentOperation (OperationKind.SimpleAssignment, Type: System.Span<System.Int32>, IsImplicit) (Syntax: 'F(1, 2)')
Left:
ILocalReferenceOperation: (OperationKind.LocalReference, Type: System.Span<System.Int32>, IsImplicit) (Syntax: 'F(1, 2)')
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 8, 2022

Choose a reason for hiding this comment

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

ILocalReferenceOperation: (OperationKind.LocalReference, Type: System.Span<System.Int32>, IsImplicit) (Syntax: 'F(1, 2)')

What is this local? I do not see any locals in the body. IOperation tree shouldn't contain any locals synthesized by lowering. #Closed

null
Arguments(1):
IArgumentOperation (ArgumentKind.ParamArray, Matching Parameter: args) (OperationKind.Argument, Type: null, IsImplicit) (Syntax: 'F(1, 2)')
IOperation: (OperationKind.None, Type: System.Span<System.Int32>, IsImplicit) (Syntax: 'F(1, 2)')
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 8, 2022

Choose a reason for hiding this comment

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

OperationKind.None

OperationKind.None is unexpected. #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 6)

@cston
Copy link
Member Author

cston commented Mar 16, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

(byte)(MemberFlags.PropertyGet), // Flags
(byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_Span_T - WellKnownType.ExtSentinel), // DeclaringTypeId
0, // Arity
0, // Method Signature
(byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Int32, // Return Type

// System_ReadOnlySpan__ctor
// System_Span_T__op_Implicit_SpanReadOnlySpan
(byte)(MemberFlags.Method | MemberFlags.Static), // Flags
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 17, 2022

Choose a reason for hiding this comment

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

MemberFlags.Method

Consider adding a PROTOTYPE comment to add dedicated flags for operators/conversions #Closed


static BoundExpression createBadExpression(SyntaxNode syntax, TypeSymbol type)
{
return new BoundBadExpression(syntax, LookupResultKind.NotReferencable, ImmutableArray<Symbol?>.Empty, ImmutableArray<BoundExpression>.Empty, type)
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 17, 2022

Choose a reason for hiding this comment

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

ImmutableArray.Empty

arrayArgs? Even in a missing helper scenarios we want IOperation tree to include values from source. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

A PROTOTYPE comment will be fine for now, I think.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 7), modulo a couple of suggested PROTOTYPE comments

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 8), assuming CI is passing

// than on the definition. That would allow calling this method with an explicit Span<T>
// without an error. That's also important because changing the method defintion between
// params and non-params is not a binary breaking change.
Error(diagnostics, ErrorCode.ERR_EscapeParamsSpan, node, expr.Syntax);
Copy link
Member

@jcouv jcouv Mar 23, 2022

Choose a reason for hiding this comment

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

Added a note to test plan. Per offline conversation, we need to figure out safe escape rules first.
Would it be okay to remove this error in the meantime? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

var m1Original = m1.LeastOverriddenMember.OriginalDefinition.GetParameters();
var m2Original = m2.LeastOverriddenMember.OriginalDefinition.GetParameters();

// Prefer params Span<T> or ReadOnlySpan<T> over params T[].
Copy link
Member

@jcouv jcouv Mar 24, 2022

Choose a reason for hiding this comment

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

The spec says there's an order of preference: ReadOnlySpan<T> first, then Span<T>, T[] and IEnumerable<T> #Closed

using Roslyn.Test.Utilities;
using Xunit;

namespace Microsoft.CodeAnalysis.CSharp.UnitTests
Copy link
Member

@jcouv jcouv Mar 24, 2022

Choose a reason for hiding this comment

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

nit: consider file-scoped namespace while you can ;-) #ByDesign

compilation,
diagnostics);

var spanConstructor = (MethodSymbol)Binder.GetWellKnownTypeMember(compilation, WellKnownMember.System_Span_T__ctorArray, diagnostics, syntax.Location);
Copy link
Member

@jcouv jcouv Mar 24, 2022

Choose a reason for hiding this comment

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

nit: consider adding nullability annotation to GetWellKnownTypeMember and enabling around the new code #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is nullable enabled, but I've left GetWellKnownTypeMember() as is, since that's probably beyond the scope of this PR.

// A.F2("span", 3);
Diagnostic(ErrorCode.ERR_FeatureInPreview, @"A.F2(""span"", 3)").WithArguments("params Span<T>").WithLocation(8, 9));

compB = CreateCompilation(sourceB, references: new[] { refA }, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe);
Copy link
Member

@jcouv jcouv Mar 24, 2022

Choose a reason for hiding this comment

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

Should use RegularNext (instead of preview) in tests that use Regular10 or CSharp10 #Resolved


if (paramArrayType.IsReadOnlySpanType(compilation))
{
var spanToReadOnlySpanOperator = (MethodSymbol)Binder.GetWellKnownTypeMember(compilation, WellKnownMember.System_Span_T__op_Implicit_SpanReadOnlySpan, diagnostics, syntax.Location);
Copy link
Member

@jcouv jcouv Mar 24, 2022

Choose a reason for hiding this comment

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

Consider adding test with missing newly-added well-known members System_Span_T__op_Implicit_SpanReadOnlySpan and System_Span_T__ctorArray
Never mind, found it: MissingSpanConstructor #Closed

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 8)

@jcouv jcouv marked this pull request as draft May 6, 2022 17:17
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 10)

@cston
Copy link
Member Author

cston commented May 24, 2022

Tests pass locally but the feature branch is not up to date for CI.

@jaredpar
Copy link
Member

@cston confirmed all the tests are passing locally. The CI machines changed out from under us durin the lifetime of this PR. The next merge from main should fix. Force merging now (it's a feature branch) and next merge from main should show green else we'll dig into the failures

@jaredpar jaredpar merged commit 92e9909 into dotnet:features/params-span May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants