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

Create params-collections.md proposal #7661

Merged
merged 12 commits into from
Nov 14, 2023
Merged

Create params-collections.md proposal #7661

merged 12 commits into from
Nov 14, 2023

Conversation

AlekseyTs
Copy link
Contributor

No description provided.

@AlekseyTs AlekseyTs requested a review from a team as a code owner November 6, 2023 16:17
@333fred
Copy link
Member

333fred commented Nov 6, 2023

This proposal needs a corresponding Champion issue, and it seems broader than either #1757 or #179. Perhaps we should create a new one, and close the other two as superseded by the new one? #WontFix

Also, one might say, that with [collection expressions](https://github.com/dotnet/csharplang/blob/main/proposals/csharp-12.0/collection-expressions.md)
now in the language, there is no need to extend `params` support at all. Foe any collection type. To consume an API with collection type, a developer
simply needs to add two characters, `[` before the expanded list of arguments, and `]` after it. Given that, extending `params` support might be an overkill,
especially that other languages are unlikely to support consumption of non-array `params` parameters any time soon.
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Nov 6, 2023

Choose a reason for hiding this comment

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

right. this was the POV of a few people the last time we discussed this. namely, that it's totally fine to just have the user write [...] around teh arguments. THe critical case as params Span because in that case we want the API developer to be able to add the overload and have it take precedence over the array version, just by recompiling (so no change to the callsite).

--

Note: i'm personally fine going either way. params collection seems the most consistent approach to take. but params span also seems sufficient. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth expanding the "we at least need to do params Span so the BCL can offer better overloads" part explicitly here.

Copy link
Member

Choose a reason for hiding this comment

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

The better overloads part is definitely a big motivator for this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The better overloads part is definitely a big motivator for this feature.

I will make sure to clearly capture this in the "Motivation" section.

;
```

A *parameter_collection* consists of an optional set of *attributes*, a `params` modifier, an optional `scoped` modifier,
Copy link
Member

@333fred 333fred Nov 6, 2023

Choose a reason for hiding this comment

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

Feels like we should have an open question on whether to permit scoped for params, or if we should always infer it if the type is a ref struct, similar to how we do for this and out parameters. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

When params modifies a ref struct parameter I think scoped should be implicitly enabled. The number of cases where you want that is virtually 100% when looking through the BCL cases. If it turns out that there is need / demand for it to be not scoped in a few cases then we could augment the design to support [UnscopedRef] here but I wouldn't want to do that without a strong supporting scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feels like we should have an open question on whether to permit scoped for params, or if we should always infer it if the type is a ref struct, similar to how we do for this and out parameters.

I captured what I am proposing. You should be able to raise concerns, if any, during LDM discussion.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is good to capture any known open questions before LDM. This is one such question.


A *parameter_collection* consists of an optional set of *attributes*, a `params` modifier, an optional `scoped` modifier,
a *type*, and an *identifier*. A parameter collection declares a single parameter of the given type with the given name.
The *type* of a parameter collection shall be one of the following valid target types for a collection expression
Copy link
Member

@333fred 333fred Nov 6, 2023

Choose a reason for hiding this comment

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

Could we not simplify this list by just specifying that there must exist a collection expression conversion from a collection expression to the target type? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we not simplify this list by just specifying that there must exist a collection expression conversion from a collection expression to the target type?

First, there is no collection expression. Second, I intentionally exclude the nullable value type case.

Also, one might say, that with [collection expressions](https://github.com/dotnet/csharplang/blob/main/proposals/csharp-12.0/collection-expressions.md)
now in the language, there is no need to extend `params` support at all. Foe any collection type. To consume an API with collection type, a developer
simply needs to add two characters, `[` before the expanded list of arguments, and `]` after it. Given that, extending `params` support might be an overkill,
especially that other languages are unlikely to support consumption of non-array `params` parameters any time soon.
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth expanding the "we at least need to do params Span so the BCL can offer better overloads" part explicitly here.

proposals/params-collections.md Show resolved Hide resolved
proposals/params-collections.md Outdated Show resolved Hide resolved
@AlekseyTs AlekseyTs merged commit 52763e3 into main Nov 14, 2023
1 check passed
@AlekseyTs AlekseyTs deleted the AlekseyTs-patch-5 branch November 14, 2023 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants