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

API Proposal: C# 10 interpolated strings support, part 3 #51962

Closed
333fred opened this issue Apr 27, 2021 · 14 comments · Fixed by #51653
Closed

API Proposal: C# 10 interpolated strings support, part 3 #51962

333fred opened this issue Apr 27, 2021 · 14 comments · Fixed by #51653
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@333fred
Copy link
Member

333fred commented Apr 27, 2021

Background and Motivation

Part 1 and part 2 added APIs to support the improved interpolated string literal feature in C# 10. After a bit more design in the space, we want to make a few minor changes to the names and add a new attribute to help improve the authoring and usage experience for users.

The general driving goal here was to standardize the names builder types use. It is expected that our guidance to API authors will be "Your interpolated string builder types should start with the words InterpolatedString and end with Builder". This convention will help make it clearer for users when something is a valid builder type.

Updated API: InterpolatedStringDefaultBuilder

We rename InterpolatedStringBuilder to InterpolatedStringDefaultBuilder. This is being done to avoid confusion with the new attribute.

-    public ref struct InterpolatedStringBuilder
+    public ref struct InterpolatedStringDefaultBuilder

Proposed API: InterpolatedStringBuilderAttribute

This attribute marks a type as being a valid builder type. This is a change from the existing proposal, where the validity of the conversion was being determined from doing a bunch of overload resolution. This should make errors more sane and helpful to end users.

Open question: Should we allow this on interfaces as well? With abstract statics in interfaces, it would theoretically be possible to use type parameter constrained to an interface as a builder type, but that would only be valid if the interface can be attributed with this.

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, AllowMultiple = false, Inherited = false)]
    public sealed class InterpolatedStringBuilderAttribute : Attribute
    {
        public InterpolatedStringBuilderAttribute()
        {
        }
    }
}

We then update the structs defined in Parts 1 and 2 as follows, to add the attribute and also standardize the naming of the other struct types with the rules mentioned above:

namespace System.Runtime.CompilerServices
{
+   [InterpolatedStringBuilder]
    public ref struct InterpolatedStringDefaultBuilder ...

+   [InterpolatedStringBuilder]
-   public ref struct InterpolatedSpanBuilder ...
+   public ref struct InterpolatedStringSpanBuilder ...
}

namespace System.Text
{
    public sealed class StringBuilder
    {
+       [InterpolatedStringBuilder]
-       public struct InterpolatedAppendFormatBuilder ...
+       public struct InterpolatedStringAppendFormatBuilder ...
    }
}
@333fred 333fred added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 27, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 27, 2021
@333fred 333fred added area-System.Runtime blocking Marks issues that we want to fast track in order to unblock other important work labels Apr 27, 2021
@ghost
Copy link

ghost commented Apr 27, 2021

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

Part 1 and part 2 added APIs to support the improved interpolated string literal feature in C# 10. After a bit more design in the space, we want to make a few minor changes to the names and add a new attribute to help improve the authoring and usage experience for users.

The general driving goal here was to standardize the names builder types use. It is expected that our guidance to API authors will be "Your interpolated string builder types should start with the words InterpolatedString and end with Builder". This convention will help make it clearer for users when something is a valid builder type.

Updated API: InterpolatedStringDefaultBuilder

We rename InterpolatedStringBuilder to InterpolatedStringDefaultBuilder. This is being done to avoid confusion with the new attribute.

-    public ref struct InterpolatedStringBuilder
+    public ref struct InterpolatedStringDefaultBuilder

Proposed API: InterpolatedStringBuilderAttribute

This attribute marks a type as being a valid builder type. This is a change from the existing proposal, where the validity of the conversion was being determined from doing a bunch of overload resolution. This should make errors more sane and helpful to end users.

Open question: Should we allow this on interfaces as well? With abstract statics in interfaces, it would theoretically be possible to use type parameter constrained to an interface as a builder type, but that would only be valid if the interface can be attributed with this.

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, AllowMultiple = false, Inherited = false)]
    public sealed class InterpolatedStringBuilderAttribute : Attribute
    {
        public InterpolatedStringBuilderAttribute()
        {
        }

        public bool UseLazyComputation { get; }
    }
}

UseLazyComputation controls the execution time of the interpolated string components. If false, the code $"{MyFunction1()}{MyFunction2()}" is executed as:

var tmp1 = MyFunction1();
var tmp2 = MyFunction2();
var builder = BuilderType.Create(0, 2);
builder.AppendFormatted(tmp1);
builder.AppendFormatted(tmp2);
builder

If UseLazyComputation is true, it would instead be executed as:

var builder = BuilderType.Create(0, 2);
builder.AppendFormatted(MyFunction1());
builder.AppendFormatted(MyFunction2());
builder

Note the move from temporary variables, executed before the builder is created, to execution inline with the AppendFormatted calls. This gives the builder author better control over exactly when things are executed and gives the user better insight into what the semantics of the code the compiler will generate is.

We then update the structs defined in Parts 1 and 2 as follows:

namespace System.Runtime.CompilerServices
{
+   [InterpolatedStringBuilder]
    public ref struct InterpolatedStringDefaultBuilder ...

+   [InterpolatedStringBuilder(UseLazyComputation = true)]
    public ref struct InterpolatedSpanBuilder ...
}

namespace System.Text
{
    public sealed class StringBuilder
    {
+       [InterpolatedStringBuilder]
        public struct InterpolatedAppendFormatBuilder ...
    }
}
Author: 333fred
Assignees: -
Labels:

api-suggestion, area-System.Runtime, blocking, untriaged

Milestone: -

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Apr 28, 2021

public bool UseLazyComputation { get; }
+   [InterpolatedStringBuilder(UseLazyComputation = true)]
    public ref struct InterpolatedSpanBuilder ...

Does UseLazyComputation need a set accessor as well?

@333fred
Copy link
Member Author

333fred commented Apr 28, 2021

Does UseLazyComputation need a set accessor as well?

That would be helpful, wouldn't it 😅

@FilipToth
Copy link
Contributor

I don't really see much use in this API. The user can do all that themselves and execute the methods whenever they want. Do you have any use-case scenarios where this would be simpler or more efficient that you could share with us?

@333fred
Copy link
Member Author

333fred commented Apr 28, 2021

I don't really see much use in this API. The user can do all that themselves and execute the methods whenever they want. Do you have any use-case scenarios where this would be simpler or more efficient that you could share with us?

I would suggest reading through the linked part one and two proposals, and the language feature they are here to support. Most of these apis will not be used directly by consumers.

@333fred
Copy link
Member Author

333fred commented Apr 28, 2021

LDM met today, and rejected the idea of UseLazyComputation. The property and explanation has been removed from the proposal.

@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 28, 2021
@michalczerwinski
Copy link

One thing which would be really useful to include in this proposal is some way of splitting string format into constant literals.
I stumbled upon this problem trying to implement feature for EF Core to allow string.Format/interpolation to be translated into SQL.

Currently the string.Format parsing is encapsulated in ValueStringBuilder.AppendFormatHelper. If we can make it public or at least some part of it it would be beneficial.

@bartonjs
Copy link
Member

bartonjs commented May 7, 2021

Video

After a long discussion we think the best convention we'll come up with is the suffix "InterpolatedStringHandler"

Therefore, the renames should be

  • InterpolatedStringHandlerArgumentAttribute
  • InterpolatedStringHandlerAttribute
  • DefaultInterpolatedStringHandler
  • MemoryExtensions+TryWriteInterpolatedStringHandler
  • StringBuilder+AppendInterpolatedStringHandler

The attribute is otherwise good as proposed

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct, AllowMultiple = false, Inherited = false)]
    public sealed class InterpolatedStringHandlerAttribute : Attribute
    {
        public InterpolatedStringHandlerAttribute()
        {
        }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 7, 2021
@bartonjs
Copy link
Member

bartonjs commented May 7, 2021

For what it's worth, the StringBuilder+AppendFormatInterpolatedStringHandler type may want a further rename, I don't believe we updated the name after expanding it to be used on StringBuilder.Append and AppendLine. So it's probably StringBuilder+AppendInterpolatedStringHandler.

@stephentoub
Copy link
Member

SpanInterpolatedStringHandler

We previously decided this was MemoryExtensions+InterpolatedTryWriteBuilder. So should it now be MemoryExtensions+TryWriteInterpolatedStringHandler, or was the switch back to "Span" intentional?

StringBuilder+AppendFormatInterpolatedStringHandler

We'd already previously decided this StringBuilder+InterpolatedAppendBuilder, so, yes, it should now be StringBuilder+AppendInterpolatedStringHandler.

@333fred
Copy link
Member Author

333fred commented May 8, 2021

SpanInterpolatedStringHandler

We previously decided this was MemoryExtensions+InterpolatedTryWriteBuilder. So should it now be MemoryExtensions+TryWriteInterpolatedStringHandler, or was the switch back to "Span" intentional?

If we did that's my fault, as I missed the rename. So yes, it should by TryWrite instead of Span.

@bartonjs
Copy link
Member

bartonjs commented May 9, 2021

Thanks for catching the clerical errors, @stephentoub. I went ahead and updated the "AppendFormat" and "Span" entries to have their correctly agreed upon elements used with the current pattern.

@stephentoub stephentoub self-assigned this May 9, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 9, 2021
@stephentoub stephentoub removed the blocking Marks issues that we want to fast track in order to unblock other important work label May 29, 2021
@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Jul 12, 2021
@tannergooding
Copy link
Member

@stephentoub, could you mark this as 6.0.0 or 7.0.0 as appropriate?

@stephentoub stephentoub added this to the 6.0.0 milestone Jul 12, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
8 participants