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

Make Options source gen support Validation attributes having constructor with params Parameter #91915

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Sep 12, 2023

This change is to support the validation attributes having constructor with params keyword parameter like DeniedValuesAttribute and AllowedValuesAttribute.

This fix need to be ported to rc2.

@ghost
Copy link

ghost commented Sep 12, 2023

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

Issue Details

This change is to support the validation attributes having constructor with params keyword parameter like DeniedValuesAttribute and AllowedValuesAttribute.

This fix need to be ported to rc2.

Author: tarekgh
Assignees: -
Labels:

area-Extensions-Options

Milestone: -

@tarekgh tarekgh added the source-generator Indicates an issue with a source generator feature label Sep 12, 2023
@tarekgh tarekgh added this to the 8.0.0 milestone Sep 12, 2023
@tarekgh
Copy link
Member Author

tarekgh commented Sep 12, 2023

CC @geeknoid @ericstj

{
validationAttr.ConstructorArguments.Add(GetArgumentExpression(constructorArgument.Type!, constructorArgument.Value));
TypedConstant argument = arguments[i];
if (argument.Kind == TypedConstantKind.Array)
Copy link
Member

@eiriktsarpalis eiriktsarpalis Sep 12, 2023

Choose a reason for hiding this comment

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

Probably too much of a niche scenario to consider here, but could this logic be tripped up by something like the following?

new MyValidationAttribute(new int[] { 1, 2, 3 });

public class MyValidationAttribute(params int[][] values) : ValidationAttribute
{
}

Copy link
Member Author

@tarekgh tarekgh Sep 12, 2023

Choose a reason for hiding this comment

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

We can consider that in .NET 9.0 as I want to limit the changes, we are porting to .NET 8.0.

Copy link
Member

@ericstj ericstj Sep 12, 2023

Choose a reason for hiding this comment

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

That's actually not a valid attribute parameter type if you want to apply the attribute (could be interesting for testing, but I gather this generator only observes applied attributes).
https://sharplab.io/#v2:D4AQTAjAsAUCDMACciCyBPAagQwDYEsATbAF3wHsA7AQRJICd8AjAVxIFNEAuRWh5tpwDesRGORIMOAsTJU+jVhwAUAB2z1sAWwDOifJRIBtALqnEANzwt2OgJSIhiAL6xXMWEal4ipCjTpFQWVKdgB3fUNzJwgAGkQwePhnOxNYBGQwNHQAFXRVdlgRGHcgA===

error CS0181: Attribute constructor parameter 'values' has type 'int[][]', which is not a valid attribute parameter type

Attribute's parameters need to be represented in metadata and can only take a very limited set of types.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I did recall that it's possible to nest arrays in attribute declarations, but it looks like it's only possible for object[] parameters:

https://sharplab.io/#v2:D4AQTAjAsAUCDMACciCyBPAagQwDYEsATbAF3wHsA7AQRJICd8AjAVxIFNEAuRWh5tpwDesRGORIMOAsTJU+jVhwAUAB2z1sAWwDOickwBW7AMYkA2gF1EANzwt2OgJSIhiAL6xPMWOal4iUgpKZUp2AHd9I1MLazcwyINjMytXRAgPDydLWARkMDR0ABV0VXZYERhvIA===

I think that case would work fine with the logic as-is, but maybe a test is in order?

Copy link
Member

Choose a reason for hiding this comment

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

I see this was brought up here as well: #91934 (comment)

@tarekgh
Copy link
Member Author

tarekgh commented Sep 12, 2023

@dotnet-policy-service rerun

@tarekgh tarekgh merged commit 7ed33d8 into dotnet:main Sep 12, 2023
103 of 105 checks passed
@tarekgh
Copy link
Member Author

tarekgh commented Sep 12, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6161762714

@ghost ghost locked as resolved and limited conversation to collaborators Oct 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Options source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants