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

Params Collections: Add a bunch of PROTOTYPE comments and a couple of VB tests. #71598

Merged

Conversation

AlekseyTs
Copy link
Contributor

No description provided.

@AlekseyTs AlekseyTs requested a review from a team as a code owner January 12, 2024 00:46
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Jan 12, 2024
@RikkiGibson RikkiGibson self-assigned this Jan 12, 2024
@AlekseyTs
Copy link
Contributor Author

@333fred, @dotnet/roslyn-compiler For the second review, please.

' The old VB compiler will not be able to consume them decorated with ParamArrayAttribute neither in normal, nor in expanded form.
' Therefore, an addition of 'params' modifier is likely to be break VB consumers, and very likely consumers from other languages.
' We possibly could fix up the new version of VB compiler, but I am not sure it would be worth the effort since we can
' simply use a different attribute, which is likely to work better for other languages too.
Copy link
Member

Choose a reason for hiding this comment

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

General question: would a different constructor work as well? IE, if we added a ParamArray(int version) constructor, could we avoid having to add an entire new attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

General question: would a different constructor work as well?

It would work for VB, I think. But might not work for other compilers/tools.

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jan 12, 2024

Choose a reason for hiding this comment

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

BTW, we can synthesize an attribute type when it is not available, but we cannot inject a constructor into an existing type.

Copy link
Member

Choose a reason for hiding this comment

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

True, but I don't expect us to synthesize this attribute. In .NET 8, we tried to move most of the standard embedded attributes into the BCL, and I expect us to continue doing in so in the future. I don't think we should synthesize such an attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should synthesize such an attribute.

I think this is what we have been doing recently for all attributes implicitly applied by compiler (as opposed for attributes applied by user explicitly). We do add the attributes to BCL as well, but do not fail when the type isn't available.

Copy link
Member

@333fred 333fred Jan 12, 2024

Choose a reason for hiding this comment

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

I think this is what we have been doing recently for all attributes implicitly applied by compiler (as opposed for attributes applied by user explicitly)

No, we've been inconsistent about that. Required does not, nor does ref readonly parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we've been inconsistent about that.

Perhaps. It is quite possible there are outliers. And probably there are good reasons why they are. It is quite possible that the new attribute will fall in to the same category. But, at the moment, that is not obvious to me.

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jan 12, 2024

Choose a reason for hiding this comment

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

I, personally, do not see a downside of synthesizing the attribute type for params.

@AlekseyTs AlekseyTs merged commit 55b6a79 into dotnet:features/ParamsCollections Jan 12, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - ParamsCollections untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants