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: represent ref fields and scoped parameters and locals #61647

Closed
cston opened this issue Jun 2, 2022 · 11 comments
Closed

API proposal: represent ref fields and scoped parameters and locals #61647

cston opened this issue Jun 2, 2022 · 11 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request New Language Feature - Ref Fields
Milestone

Comments

@cston
Copy link
Member

cston commented Jun 2, 2022

This issue is referenced in source/test.

Background and Motivation

Represent ref modifier for fields and the related scoped modifier for parameters and locals (see language proposal).

ref fields:

readonly ref struct Ref<T>
{
    private readonly ref T _t;
    public Ref(ref T t) { _t = ref t; }
}

readonly ref struct ReadOnlyRef<T>
{
    private readonly ref readonly T _t;
    public ReadOnlyRef(ref T t) { _t = ref t; }
}

scoped parameters and locals:

static void F(scoped ref int x, ref int y)
{
    scoped Ref<int> r = new Ref<int>(ref x);
}

Proposed API

Symbols

namespace Microsoft.CodeAnalysis
{
    public interface IFieldSymbol : ISymbol
    {
+       /// <summary>
+       /// Returns the RefKind of the field.
+       /// </summary>
+       RefKind RefKind { get; }

+       /// <summary>
+       /// Custom modifiers associated with the ref modifier, or an empty array if there are none.
+       /// </summary>
+       ImmutableArray<CustomModifier> RefCustomModifiers { get; }
    }

    public interface ILocalSymbol : ISymbol
    {
+       /// <summary>
+       /// Returns true if the local ref or value is scoped to the current method.
+       /// </summary>
+       bool IsScoped { get; }
    }

    public interface IParameterSymbol : ISymbol
    {
+       /// <summary>
+       /// Returns true if the parameter ref or value is scoped to the current method.
+       /// </summary>
+       bool IsScoped { get; }
    }
}

Syntax

SyntaxKind.ScopedKeyword is added.

ScopedTypeSyntax is added that combines a scoped keyword and following type syntax.

If the type following scoped includes a ref, ref readonly, in, or out, the type is represented as a nested RefTypeSyntax.

namespace Microsoft.CodeAnalysis.CSharp
{
    public enum SyntaxKind
    {
+       /// <summary>Represents <see langword="scoped"/>.</summary>
+       ScopedKeyword = 8447,
    }

    public class SyntaxFactory
    {
+       public static ScopedTypeSyntax ScopedType(SyntaxToken scopedKeyword, TypeSyntax type);
    }
}

namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
+   public sealed class ScopedTypeSyntax : TypeSyntax
+   {
+       public SyntaxToken ScopedKeyword { get; }
+       public TypeSyntax Type { get; }
+
+       public ScopedTypeSyntax Update(SyntaxToken scopedKeyword, TypeSyntax type);
+
+       public ScopedTypeSyntax WithScopedKeyword(SyntaxToken scopedKeyword);
+       public ScopedTypeSyntax WithType(TypeSyntax type);
+   }
}

Metadata encoding

ScopedRefAttribute is used by the compiler to emit scoped modifiers to metadata.

The attribute type definition is synthesized by the compiler if missing from the compilation.

namespace System.Runtime.CompilerServices
{
+   [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
+   internal sealed class ScopedRefAttribute : Attribute
+   {
+   }
}

Usage Examples

Using RefTypeSyntax is a change for ParameterSyntax which currently represents ref keywords in ParameterSyntax.Modifiers. The proposal is to use RefTypeSyntax for ref keywords in parameters within scoped types only, to avoid a breaking change for C#10 syntax.

For example, the parameters from static void M(ref int x, scoped ref int y) will be represented as:

SyntaxFactory.ParameterList(
    SyntaxFactory.Parameter(
        modifiers: SyntaxFactory.TokenList(SyntaxFactory.Token(SyntaxKind.RefKeyword)),
        type: SyntaxFactory.PredefinedType(SyntaxFactory.Token(SyntaxKind.IntKeyword)),
        identifier: "x"),
    SyntaxFactory.Parameter(
        modifiers: default,
        type: SyntaxFactory.ScopedType(
            SyntaxFactory.Token(SyntaxKind.ScopedKeyword),
            SyntaxFactory.RefType(
                SyntaxFactory.Token(SyntaxKind.RefKeyword),
                SyntaxFactory.PredefinedType(SyntaxFactory.Token(SyntaxKind.IntKeyword)))),
        identifier: "y"));

Alternative Designs

Risks

@cston cston added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Jun 2, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 2, 2022
@cston
Copy link
Member Author

cston commented Jun 2, 2022

cc @333fred, @jaredpar

@RikkiGibson
Copy link
Contributor

It feels like in order to merge the feature in a timely manner, we could reduce the set of proposed public APIs for the initial merge, perhaps to just the following:

  1. Syntax changes
  2. IFieldSymbol.RefKind

Then in the following couple of weeks or so, we could converge on a design for how to represent scoped. It might be useful to consider how the feature could evolve in future versions of the language when we consider the shape of the public API.

@jaredpar
Copy link
Member

jaredpar commented Jun 2, 2022

The ref fields feature is in a bit of a time crunch here and it needs to be merged in soon-ish so we can coordinate changes with dotnet/runtime. Sounds like a good suggestion to balance getting the feature in and merged so we can work with runtime and backload some of the other work which is more fit / finish.

@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 7, 2022
@jcouv jcouv added this to the C# 11.0 milestone Jun 7, 2022
@jaredpar jaredpar modified the milestones: C# 11.0, 17.4 Jun 24, 2022
@cston
Copy link
Member Author

cston commented Jul 7, 2022

Updated proposal based on recent changes to the spec:

  • Removed the proposed Scoped field from RefTypeSyntax
  • IParameterSymbol and ILocalSymbol now have a single IsScoped property in each case rather than separate IsRefScoped and IsValueScoped properties
  • LifetimeAnnotationAttribute takes a single int value rather than two bool values, and a single IsScoped property

@333fred
Copy link
Member

333fred commented Jul 7, 2022

@cston is this ready for review? If so, it needs the ready-for-review label.

@cston cston added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Jul 7, 2022
@333fred
Copy link
Member

333fred commented Jul 7, 2022

API Review

  • Need to update the docs for SymbolDisplayMemberOptions.IncludeRef
  • For locals and parameters, could we hide IncludeRef and add a new IncludeRefAndScoped with the same value?
  • Should we have a ScopedKind now, instead of IsScoped? That way we could adapt to ref scoped in the future
    • An enum ScopedKind, with None, ScopedValue, and ScopedRef

@333fred 333fred added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 7, 2022
@cston
Copy link
Member Author

cston commented Jul 21, 2022

Updated the synthesized attribute in the description (see also #62838).

@cston
Copy link
Member Author

cston commented Aug 4, 2022

Updated syntax API based on recent discussions, and moved SymbolDisplay proposal to a separate issue: #63208.

@cston cston added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Aug 4, 2022
@333fred
Copy link
Member

333fred commented Aug 4, 2022

API Review

The shape is generally approved, swapping the IsScoped for the ScopedKind previously discussed. We will also follow up with Cyrus when he's back from vacation on whether we should use the ScopedTypeSyntax for parameters, or whether we should be consistent with existing parameter parsing (putting the keyword as a modifier on the parameter).

namespace Microsoft.CodeAnalysis
{
    public interface IFieldSymbol : ISymbol
    {
       /// <summary>
       /// Returns the RefKind of the field.
       /// </summary>
       RefKind RefKind { get; }

       /// <summary>
       /// Custom modifiers associated with the ref modifier, or an empty array if there are none.
       /// </summary>
       ImmutableArray<CustomModifier> RefCustomModifiers { get; }
    }

    public interface ILocalSymbol : ISymbol
    {
       /// <summary>
       /// Returns the scope kind of the local.
       /// </summary>
       ScopedKind ScopedKind { get; }
    }

    public interface IParameterSymbol : ISymbol
    {
       /// <summary>
       /// Returns the scope kind of the parameter.
       /// </summary>
       ScopedKind ScopedKind { get; }
    }
    public enum ScopedKind
    {
        None,
        ScopedValue,
        ScopedRef,
    }
}
namespace Microsoft.CodeAnalysis.CSharp
{
    public enum SyntaxKind
    {
       /// <summary>Represents <see langword="scoped"/>.</summary>
       ScopedKeyword = 8447,
    }

    public class SyntaxFactory
    {
       public static ScopedTypeSyntax ScopedType(SyntaxToken scopedKeyword, TypeSyntax type);
    }
}

namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
   public sealed class ScopedTypeSyntax : TypeSyntax
   {
       public SyntaxToken ScopedKeyword { get; }
       public TypeSyntax Type { get; }

       public ScopedTypeSyntax Update(SyntaxToken scopedKeyword, TypeSyntax type);

       public ScopedTypeSyntax WithScopedKeyword(SyntaxToken scopedKeyword);
       public ScopedTypeSyntax WithType(TypeSyntax type);
   }
}

@333fred 333fred 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 Aug 4, 2022
@cston
Copy link
Member Author

cston commented Aug 23, 2022

We will also follow up ... on whether we should use the ScopedTypeSyntax for parameters ... .

The conclusion from the follow up discussion over email:

For parameters, the scoped modifier will be represented as a token in the ParameterSyntax.Modifiers list. This matches how ref/in/out are represented for parameters.

For local declarations, the scoped modifier will be represented, with a new ScopedTypeSyntax type, as part of VariableDeclarationSyntax.Type. This is similar to RefTypeSyntax for ref and ref readonly in local declarations.

@cston
Copy link
Member Author

cston commented Oct 1, 2022

API has been updated.

@cston cston closed this as completed Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request New Language Feature - Ref Fields
Projects
None yet
Development

No branches or pull requests

5 participants