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

Update SymbolDisplay to include scoped for parameters and locals #63208

Closed
cston opened this issue Aug 4, 2022 · 3 comments · Fixed by #64285
Closed

Update SymbolDisplay to include scoped for parameters and locals #63208

cston opened this issue Aug 4, 2022 · 3 comments · Fixed by #64285
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 - Ref Fields Feature Request

Comments

@cston
Copy link
Member

cston commented Aug 4, 2022

Background and Motivation

Include scoped modifier for parameter and local types from SymbolDisplay.

Proposed API

For SymbolDisplayLocalOptions, the IncludeRef option is extended to include scoped, and a new IncludeRefAndScoped option with the same value is added.

For SymbolDisplayParameterOptions, the IncludeParamsRefOut option is extended to include scoped, and a new IncludeParamsRefOutScoped option with the same value is added.

namespace Microsoft.CodeAnalysis
{
    [Flags]
    public enum SymbolDisplayLocalOptions
    {
        // ...

        /// <summary>
-       /// Includes the <c>ref</c> keyword for ref-locals.
+       /// Includes the <c>ref</c> keyword for ref-locals and the <c>scoped</c> keyword for scoped locals.
        /// Replaced by <see cref="IncludeRefAndScoped"/>.
        /// </summary>
        IncludeRef = 1 << 2,

+       /// <summary>
+       /// Includes the <c>ref</c> keyword for ref-locals and the <c>scoped</c> keyword for scoped locals.
+       /// </summary>
+       IncludeRefAndScoped = 1 << 2,

    }

    [Flags]
    public enum SymbolDisplayParameterOptions
    {
        // ...

        /// <summary>
-       /// Includes the <c>params</c>, <c>ref</c>, <c>in</c>, <c>out</c>, <c>ByRef</c>, <c>ByVal</c> keywords before parameters.
+       /// Includes the <c>params</c>, <c>scoped</c>, <c>ref</c>, <c>in</c>, <c>out</c>, <c>ByRef</c>, <c>ByVal</c> keywords before parameters.
        /// Replaced by <see cref="IncludeParamsRefOutScoped"/>.
        /// </summary>
        IncludeParamsRefOut = 1 << 1,

+       /// <summary>
+       /// Includes the <c>params</c>, <c>scoped</c>, <c>ref</c>, <c>in</c>, <c>out</c>, <c>ByRef</c>, <c>ByVal</c> keywords before parameters.
+       /// </summary>
+       IncludeParamsRefOutScoped = 1 << 1,
    }
}

Usage Examples

// some lines of code here

Alternative Designs

Risks


Separated out from #61647. See #61647 (comment) that should be addressed.

@cston cston added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Aug 4, 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 Aug 4, 2022
@cston cston added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Aug 4, 2022
@jaredpar jaredpar added Feature - Ref Fields and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 9, 2022
@jaredpar jaredpar added this to the 17.4 milestone Aug 9, 2022
@jaredpar jaredpar assigned jjonescz and unassigned cston Sep 19, 2022
@cston
Copy link
Member Author

cston commented Sep 20, 2022

The notes for SymbolDisplay options from API review:

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?

@cston cston assigned cston and unassigned jjonescz Sep 23, 2022
@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 Sep 29, 2022
@cston
Copy link
Member Author

cston commented Sep 29, 2022

Updated the proposed API based on previous API review discussion.

@cston cston modified the milestones: 17.4, 17.5 Sep 29, 2022
@333fred
Copy link
Member

333fred commented Sep 29, 2022

API Review

  • Like reusing
  • Should we just call it IncludeModifiers?
    • Yes
  • What about implicit cases? out is implicitly scoped in some cases, for example.
    • Shouldn't include in symbol display if it's not legal.
    • Rather than trying to really determine this, maybe we just say we don't put scoped on out parameters.
  • Can we punt it to 17.5, as we're running up to the deadline for 17.4?
    • Yes, this is fine.

Conclusion: Accepted, renaming the options to IncludeModifiers.

@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 Sep 29, 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 - Ref Fields Feature Request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants