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 support for collection expressions in attributes #69968

Merged
merged 8 commits into from
Sep 26, 2023

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Sep 15, 2023

Fixes #69133

@jcouv jcouv self-assigned this Sep 15, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Sep 15, 2023
@jcouv jcouv added this to the 17.8 milestone Sep 15, 2023
var attr = (XAttribute)System.Attribute.GetCustomAttribute(typeof(C), typeof(XAttribute));
Console.Write(attr._values.Length);

[X([])]
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Sep 16, 2023

Choose a reason for hiding this comment

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

test with a string in the literal, and 'null' in the literal? #Resolved


CreateCompilation(source).VerifyEmitDiagnostics(
// (1,14): error CS9176: There is no target type for the collection expression.
// [X([1, 2, .. [3]])]
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Sep 16, 2023

Choose a reason for hiding this comment

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

definitely an odd error :) #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. It's an existing problem though and I'm not planning to address in this PR.

public class XAttribute : System.Attribute
{
public int[] _values;
public XAttribute(int[] values) { _values = values; }
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Sep 16, 2023

Choose a reason for hiding this comment

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

test with param int[]? #Resolved

@jcouv jcouv marked this pull request as ready for review September 25, 2023 17:21
@jcouv jcouv requested a review from a team as a code owner September 25, 2023 17:21
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Sep 25, 2023
@jcouv jcouv requested a review from a team as a code owner September 25, 2023 21:59
// [X([1])]
Diagnostic(ErrorCode.ERR_BadAttributeParamType, "X").WithArguments("a", "A").WithLocation(12, 2)
);
}
Copy link
Member

@cston cston Sep 25, 2023

Choose a reason for hiding this comment

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

}

Please test a spread: [X([.. new List<int>()])] #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thanks

// [X([[1], [2]])]
Diagnostic(ErrorCode.ERR_BadAttributeParamType, "X").WithArguments("values", "int[][]").WithLocation(1, 2)
);
}
Copy link
Member

@cston cston Sep 25, 2023

Choose a reason for hiding this comment

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

}

Consider testing:
[X([(int[])[1]])] with XAttribute(object[] values) { }
[X((int[])[1, 2, 3])] with XAttribute(object value) { } #Resolved

@jaredpar
Copy link
Member

@jcouv make sure to retarget this to release/dev17.8

}
""";

CreateCompilation(source, targetFramework: TargetFramework.Net70).VerifyEmitDiagnostics(
Copy link
Member

@cston cston Sep 26, 2023

Choose a reason for hiding this comment

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

targetFramework: TargetFramework.Net70

Is there a reason to use TargetFramework.Net70 for this test and several below? #Resolved

string source = """
var attr = (XAttribute)System.Attribute.GetCustomAttribute(typeof(C), typeof(XAttribute));
var inner = (int[])attr._values[0];
System.Console.Write($"OuterLength={attr._values.Length} InnerValue={inner[0]} InnerLength={inner.Length}");
Copy link
Member

@cston cston Sep 26, 2023

Choose a reason for hiding this comment

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

Consider using attr._values.Report(); in this test and below. #Resolved

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Sep 26, 2023

I haven't checked, but are there tests for using a collection-expr as an argument to an attribute 'named arg' and an attribute 'named field/prop'? #Resolved

@jcouv jcouv changed the base branch from main to release/dev17.8 September 26, 2023 17:39
@jcouv
Copy link
Member Author

jcouv commented Sep 26, 2023

I haven't checked, but are there tests for using a collection-expr as an argument to an attribute 'named arg' and an attribute 'named field/prop'?

Thanks, that's a great suggestion. Not sure how I missed that. Will add some tests #Resolved

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass

@jcouv jcouv requested a review from 333fred September 26, 2023 21:31
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Thanks for the additional tests. I do think we likely have a cycle here, but it's not introduced by this PR, probably pre-existing.

@jcouv jcouv enabled auto-merge (squash) September 26, 2023 21:50
@jcouv jcouv merged commit b925063 into dotnet:release/dev17.8 Sep 26, 2023
@CyrusNajmabadi
Copy link
Member

Woohoo! Thanks for getting this in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when using collection-expression for an array-typed attribute argument.
5 participants