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

Use single argument overload for [start..] of string or (ReadOnly)Span #74643

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Use single argument overload for [start..] of string or (ReadOnly)Span #74643

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7324eeb
Use single argument overload for `[start..]` of `string` or `Span`
tats-u Aug 4, 2024
7abaa8e
Don't continue to try simplifying if overload is missing
tats-u Aug 4, 2024
e53df10
Fix for 1st review and improve
tats-u Aug 13, 2024
6bd5239
Add member missing tests
tats-u Aug 13, 2024
bdf17b9
Use only`TestSource`
tats-u Aug 13, 2024
7385490
Fix VB test
tats-u Aug 14, 2024
e2b3f40
Update generated ILs in some existing tests
tats-u Aug 14, 2024
594ee24
Revert unintended replacements
tats-u Aug 15, 2024
77d5130
Use `CombinatorialData`
tats-u Aug 15, 2024
23455d1
Fix Debug.Assert
tats-u Aug 15, 2024
eb7410b
Fix `Span.ToString`
tats-u Aug 17, 2024
57382f3
Improve tests
tats-u Aug 17, 2024
2bd0e6c
Fix tests
tats-u Aug 17, 2024
83f315e
Revert unintended change
tats-u Aug 17, 2024
19aba1b
Fix tests
tats-u Aug 17, 2024
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
54 changes: 53 additions & 1 deletion src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,7 +984,59 @@ private BoundExpression VisitRangePatternIndexerAccess(BoundImplicitIndexerAcces
AddPlaceholderReplacement(node.ArgumentPlaceholders[1], rangeSizeExpr);

var sliceCall = (BoundCall)node.IndexerOrSliceAccess;
var rewrittenIndexerAccess = VisitExpression(sliceCall);

// [start..] can be simplified to .Substring(start) or .Slice(start) for some built-in types
// e.g. string and (ReadOnly)Span

BoundExpression? moreEfficientIndexerAccess = null;
if (endMakeOffsetInput is null)
{
MethodSymbol? singleArgumentOverload = null;
NamedTypeSymbol? typeContainingElementType = null;

if (
TryGetSpecialTypeMethod(node.Syntax, SpecialMember.System_String__Substring, out var stringSubstring, isOptional: true)
&& sliceCall.Method.Equals(stringSubstring)
Copy link
Member

Choose a reason for hiding this comment

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

Please use the Equals that takes a comparison kind for these calls.

Copy link
Author

Choose a reason for hiding this comment

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

Do you think TypeCompareKind.ConsiderEverything is correct?

Copy link
Member

Choose a reason for hiding this comment

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

It is for string. Span is more interesting: we probably don't want to take tuple names into account, for example. You'll need to go through the various ignore options and add some tests around it.

Copy link
Author

Choose a reason for hiding this comment

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

tuple names

Do you mean the element type inside <...>?
For Span, I use its UnderlyingType to compare. I think we don't have to care about it and related ones.
Span is a value type, so ? shouldn't be ignored. I found CLRSignatureCompareOptions and it looks great.

Copy link
Member

Choose a reason for hiding this comment

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

As an example, what if the argument to Slice as a modopt on a parameter or return? That's something that would be affected by the comparison option chosen.

Copy link
Author

@tats-u tats-u Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Notice: This hasn't been fixed yet.

I've never heard modopt before and Google provided me little information for it.

https://stackoverflow.com/questions/69385708/what-is-a-custommodifier
https://learn.microsoft.com/ja-jp/dotnet/api/microsoft.codeanalysis.custommodifier?view=roslyn-dotnet-4.9.0

I'm afraid the above literature didn't remind me any example C# code or the correct value for the TypeCompareKind enum.

)
tats-u marked this conversation as resolved.
Show resolved Hide resolved
{
// If the overload is missing, we won't try the other `else if`
tats-u marked this conversation as resolved.
Show resolved Hide resolved
if (TryGetSpecialTypeMethod(node.Syntax, SpecialMember.System_String__SubstringInt, out var stringOverload, isOptional: true))
singleArgumentOverload = stringOverload;
tats-u marked this conversation as resolved.
Show resolved Hide resolved
}
// with single type parameter ((ReadOnly)Span)
else if (sliceCall is { Method: SubstitutedMethodSymbol { UnderlyingMethod: var generalizedMethod }, Type: NamedTypeSymbol typeWithElementType })
{
// We use the return type of the current overload to simplify the logic
typeContainingElementType = typeWithElementType;
tats-u marked this conversation as resolved.
Show resolved Hide resolved
if (
TryGetWellKnownTypeMember(node.Syntax, WellKnownMember.System_Span_T__Slice_Int_Int, out MethodSymbol? spanSlice, isOptional: true)
&& generalizedMethod.Equals(spanSlice)
)
{
if (TryGetWellKnownTypeMember(node.Syntax, WellKnownMember.System_Span_T__Slice_Int, out MethodSymbol? spanOverload, isOptional: true))
singleArgumentOverload = spanOverload;
}
else if (
TryGetWellKnownTypeMember(node.Syntax, WellKnownMember.System_ReadOnlySpan_T__Slice_Int_Int, out MethodSymbol? readOnlySpanSlice, isOptional: true)
&& generalizedMethod.Equals(readOnlySpanSlice)
)
{
if (TryGetWellKnownTypeMember(node.Syntax, WellKnownMember.System_ReadOnlySpan_T__Slice_Int, out MethodSymbol? readOnlySpanOverLoad, isOptional: true))
singleArgumentOverload = readOnlySpanOverLoad;

}
}

if (singleArgumentOverload is not null)
{
var overloadWithMaybeT = typeContainingElementType is not null
? new SubstitutedMethodSymbol(typeContainingElementType, singleArgumentOverload)
tats-u marked this conversation as resolved.
Show resolved Hide resolved
: singleArgumentOverload;
moreEfficientIndexerAccess = F.Call(receiver, overloadWithMaybeT, startExpr);
}
}

var rewrittenIndexerAccess = moreEfficientIndexerAccess ?? VisitExpression(sliceCall);

RemovePlaceholderReplacement(node.ArgumentPlaceholders[0]);
RemovePlaceholderReplacement(node.ArgumentPlaceholders[1]);
Expand Down
89 changes: 89 additions & 0 deletions src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs
View file
Open in desktop
tats-u marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -4236,5 +4236,94 @@ static void M(Span<int> s)
var comp = CreateCompilation(source, targetFramework: TargetFramework.Net70);
comp.VerifyDiagnostics();
}
[ConditionalFact(typeof(CoreClrOnly))]
tats-u marked this conversation as resolved.
Show resolved Hide resolved
public void SingleOverloadReadOnlySpan()
{
string source = """
using System;

ReadOnlySpan<char> s = "0123";
Console.Write(s[1..].ToString());
""";
var comp = CompileAndVerify(source, targetFramework: TargetFramework.Net70, expectedOutput: "123");
comp.VerifyDiagnostics();
comp.VerifyIL("<top-level-statements-entry-point>", """
{
// Code size 39 (0x27)
.maxstack 2
.locals init (System.ReadOnlySpan<char> V_0, //s
System.ReadOnlySpan<char> V_1)
IL_0000: ldstr "0123"
IL_0005: call "System.ReadOnlySpan<char> string.op_Implicit(string)"
IL_000a: stloc.0
IL_000b: ldloca.s V_0
IL_000d: ldc.i4.1
IL_000e: call "System.ReadOnlySpan<char> System.ReadOnlySpan<char>.Slice(int)"
IL_0013: stloc.1
IL_0014: ldloca.s V_1
IL_0016: constrained. "System.ReadOnlySpan<char>"
IL_001c: callvirt "string object.ToString()"
IL_0021: call "void System.Console.Write(string)"
IL_0026: ret
}
""");
}

[ConditionalFact(typeof(CoreClrOnly))]
public void SingleOverloadSpan()
{
string source = """
using System;

Console.Write("0123".ToCharArray().AsSpan()[1..].ToString());
tats-u marked this conversation as resolved.
Show resolved Hide resolved
""";
var comp = CompileAndVerify(source, targetFramework: TargetFramework.Net70, expectedOutput: "123");
comp.VerifyDiagnostics();
comp.VerifyIL("<top-level-statements-entry-point>", """
{
// Code size 44 (0x2c)
.maxstack 2
.locals init (System.ReadOnlySpan<char> V_0, //s
)
IL_0000: ldstr "0123"
IL_0005: call "char[] string.ToCharArray()"
IL_000a: call "System.ReadOnlySpan<char> string.op_Implicit(string)"
IL_000f: stloc.0 V_0
IL_0010: ldloca.s 0
IL_0012: ldc.i4.1
IL_0013: call "System.Span<char> System.Span<char>.Slice(int)"
IL_0018: stloc.0
IL_0019: ldloca.s V_0
IL_001b: constrained. "System.Span<char>"
IL_0021: callvirt "string object.ToString()"
IL_0026: call "void System.Console.Write(string)"
IL_002b: ret
}
""");
}

[ConditionalFact(typeof(CoreClrOnly))]
public void SingleOverloadString()
{
string source = """
using System;

Console.Write("0123"[1..]);
""";
var comp = CompileAndVerify(source, targetFramework: TargetFramework.Net70, expectedOutput: "123");
comp.VerifyDiagnostics();
comp.VerifyIL("<top-level-statements-entry-point>", """
{
// Code size 17 (0x11)
.maxstack 2

IL_0000: ldstr "0123"
IL_0005: ldc.i4.1
IL_0006: callvirt "string string.Substring(int)"
IL_000b: call "void System.Console.Write(string)"
IL_0010: ret
}
""");
}
}
}
2 changes: 2 additions & 0 deletions src/Compilers/Core/Portable/SpecialMember.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,8 @@ internal enum SpecialMember

System_Type__GetTypeFromHandle,

System_String__SubstringInt,
tats-u marked this conversation as resolved.
Show resolved Hide resolved

Count
}
}
9 changes: 9 additions & 0 deletions src/Compilers/Core/Portable/SpecialMembers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1313,6 +1313,14 @@ static SpecialMembers()
1, // Method Signature
(byte)SignatureTypeCode.TypeHandle, (byte)InternalSpecialType.System_Type, // Return Type
(byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_RuntimeTypeHandle,

// System_String__SubstringInt
(byte)MemberFlags.Method, // Flags
(byte)SpecialType.System_String, // DeclaringTypeId
0, // Arity
1, // Method Signature
(byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_String, // Return Type
(byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Int32,
};

string[] allNames = new string[(int)SpecialMember.Count]
Expand Down Expand Up @@ -1474,6 +1482,7 @@ static SpecialMembers()
"Empty", // System_Array__Empty
"SetValue", // System_Array__SetValue
"GetTypeFromHandle", // System_Type__GetTypeFromHandle
"Substring", // System_String__SubstringInt
};

s_descriptors = MemberDescriptor.InitializeFromStream(new System.IO.MemoryStream(initializationBytes, writable: false), allNames);
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/Core/Portable/WellKnownMember.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,9 @@ internal enum WellKnownMember

System_Runtime_CompilerServices_ParamCollectionAttribute__ctor,

System_Span_T__Slice_Int,
System_ReadOnlySpan_T__Slice_Int,

Count,

// Remember to update the AllWellKnownTypeMembers tests when making changes here
Expand Down
24 changes: 24 additions & 0 deletions src/Compilers/Core/Portable/WellKnownMembers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4336,6 +4336,28 @@ static WellKnownMembers()
0, // Arity
0, // Method Signature
(byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Void, // Return Type

// System_Span_T__Slice_Int
(byte)(MemberFlags.Method), // Flags
(byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_Span_T - WellKnownType.ExtSentinel), // DeclaringTypeId
0, // Arity
1, // Method Signature
(byte)SignatureTypeCode.GenericTypeInstance, (byte)SignatureTypeCode.TypeHandle,
(byte)WellKnownType.ExtSentinel, (WellKnownType.System_Span_T - WellKnownType.ExtSentinel),
1,
(byte)SignatureTypeCode.GenericTypeParameter, (byte)0, // Return Type
(byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Int32,
// System_ReadOnlySpan_T__Slice_Int
tats-u marked this conversation as resolved.
Show resolved Hide resolved
(byte)(MemberFlags.Method), // Flags
(byte)WellKnownType.ExtSentinel, (byte)(WellKnownType.System_ReadOnlySpan_T - WellKnownType.ExtSentinel), // DeclaringTypeId
0, // Arity
1, // Method Signature
(byte)SignatureTypeCode.GenericTypeInstance, (byte)SignatureTypeCode.TypeHandle,
(byte)WellKnownType.ExtSentinel, (WellKnownType.System_ReadOnlySpan_T - WellKnownType.ExtSentinel),
1,
(byte)SignatureTypeCode.GenericTypeParameter, (byte)0, // Return Type
(byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Int32,

};

string[] allNames = new string[(int)WellKnownMember.Count]
Expand Down Expand Up @@ -4871,6 +4893,8 @@ static WellKnownMembers()
"Empty", // System_Collections_Immutable_ImmutableArray_T__Empty
"AddRange", // System_Collections_Generic_List_T__AddRange
".ctor", // System_Runtime_CompilerServices_ParamCollectionAttribute__ctor
"Slice", // System_Span_T__Slice_Int
"Slice", // System_ReadOnlySpan_T__Slice_Int
};

s_descriptors = MemberDescriptor.InitializeFromStream(new System.IO.MemoryStream(initializationBytes, writable: false), allNames);
Expand Down
Loading