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

Add IParameterSymbol.IsParamCollection property #71816

Closed
Tracked by #71137
AlekseyTs opened this issue Jan 26, 2024 · 3 comments
Closed
Tracked by #71137

Add IParameterSymbol.IsParamCollection property #71816

AlekseyTs opened this issue Jan 26, 2024 · 3 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 - ParamsCollections Feature Request

Comments

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 26, 2024

Background and Motivation

Related to "Params Collections" feature work.

Proposed API

namespace Microsoft.CodeAnalysis
{
    /// <summary>
    /// Represents a parameter of a method or property.
    /// </summary>
    /// <remarks>
    /// This interface is reserved for implementation by its associated APIs. We reserve the right to
    /// change it in the future.
    /// </remarks>
    public interface IParameterSymbol : ISymbol
    {
        /// <summary>
        /// Returns true if the parameter was declared as a parameter array. 
        /// </summary>
        bool IsParams { get; }

+        /// <summary>
+        /// Returns true if the parameter was declared as a parameter collection. 
+        /// </summary>
+        bool IsParamCollection { get; }
    }
}

Alternative Designs

Option 1

We could extend the meaning of IsParams property to return true for non-array param collections instead,
but then VB, for example, will print C# methods like

Public Shared Sub M1(ParamArray a As System.Collections.Generic.IEnumerable(Of Integer))

which is likely to confuse the user. VB is not going to support params collections.

Other consumers can be affected in a similar way.

Option 2

As the top proposal, but with additional new property:

namespace Microsoft.CodeAnalysis
{
    /// <summary>
    /// Represents a parameter of a method or property.
    /// </summary>
    /// <remarks>
    /// This interface is reserved for implementation by its associated APIs. We reserve the right to
    /// change it in the future.
    /// </remarks>
    public interface IParameterSymbol : ISymbol
    {
        /// <summary>
        /// Returns true if the parameter was declared as a parameter array. 
        /// </summary>
        bool IsParams { get; }

+        /// <summary>
+        /// Same as IsParams
+        /// </summary>
+        bool IsParamArray { get; }

+        /// <summary>
+        /// Returns true if the parameter was declared as a parameter collection. 
+        /// </summary>
+        bool IsParamCollection { get; }
    }
}

Option 3

Have IsParams return true if if the parameter was declared either as a parameter array,
or a parameter collection. Add new property for parameter aray.

namespace Microsoft.CodeAnalysis
{
    /// <summary>
    /// Represents a parameter of a method or property.
    /// </summary>
    /// <remarks>
    /// This interface is reserved for implementation by its associated APIs. We reserve the right to
    /// change it in the future.
    /// </remarks>
    public interface IParameterSymbol : ISymbol
    {
        /// <summary>
-       /// Returns true if the parameter was declared as a parameter array. 
+       /// Returns true if the parameter was declared as a parameter array or as a parameter collection. 
        /// </summary>
        bool IsParams { get; }

+        /// <summary>
+        /// Returns true if the parameter was declared as a parameter array 
+        /// </summary>
+        bool IsParamArray { get; }

+        /// <summary>
+        /// Returns true if the parameter was declared as a parameter collection. 
+        /// </summary>
+        bool IsParamCollection { get; }
    }
}

Risks

@AlekseyTs AlekseyTs added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Jan 26, 2024
@AlekseyTs AlekseyTs self-assigned this Jan 26, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 26, 2024
@AlekseyTs AlekseyTs added Feature - ParamsCollections api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 26, 2024
@CyrusNajmabadi
Copy link
Member

Thoughts/questions for review.

  1. is i2 intentional as Param not Params? I'd personally prefer the correspondance with IsParams and IsParamsCollection. As well as the fact that in C# you'd be using the params keyword.
  2. if IsParams is true, will IsParamCollection be true as well? Or is the latter only for teh non array cases?

I would consider having us add an IsParamsArray property to go with IsParamsCollection. And consider deprecating IsParams and telling users to use one of the others :)

@AlekseyTs
Copy link
Contributor Author

is i2 intentional as Param not Params?

That is to match name of the attribute that is supposed to mark the parameter in metadata, ParamCollectionAttribute. The attribute for array also doesn't include s, ParamArrayAttribute.
We will certainly discuss the naming.

if IsParams is true, will IsParamCollection be true as well? Or is the latter only for teh non array cases?

From metadata:

  • as it is used to be for IsParams property and will remain this way, IsParams simply reflects the presence of ParamArrayAttribute, the type of the parameter doesn't matter.
  • IsParamCollection simply reflects the presence of ParamCollectionAttribute regardless of type.

For correct source code, only one of the attributes will be emitted and the properties will reflect the fact (again, no change for IsParams in that regard).

Therefore, for conventional metadata and for source, both properties are never true.

I would consider having us add an IsParamsArray property to go with IsParamsCollection. And consider deprecating IsParams and telling users to use one of the others

This is definitely an option. However, implementers will still have to implement the deprecated property, but in a prescribed way (I think we cannot use default interface implementations). Also, existing consumers of IsParams are likely to be broken in this case, as VB symbol display would be.

@333fred
Copy link
Member

333fred commented Feb 1, 2024

API Review

  • Only C# will recognize these extra types as params parameters
  • Naming with an s?
    • We've gone with C# naming for these in the past
  • Could consider an enum option if we think there could be more params things in the future?
    • Considered for completeness, not particularly caring
  • IDE usage seems like it would be fine with option 3, and may be the easiest to absorb.
    • There's at least one case in the IDE codebase that does a hard cast
    • params can be put on bad things in source, would analyzers have to handle this already?
      • The cast will now break on correct code, which it would not have before
  • Going through references in grep.app, nearly all would be better served by option 3. Only one example of obviously bad code in the presence of option 3.
    • Given this, we prefer option 3

Conclusion: We will go with option 3, with IsParamsArray and IsParamsCollection as the property names.

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 - ParamsCollections Feature Request
Projects
None yet
Development

No branches or pull requests

3 participants