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

Allow parameter type name to be specified for WriteCodeFragment task #6285

Merged

Conversation

@reduckted
Copy link
Contributor

@reduckted reduckted commented Mar 20, 2021

Fixes #2281

Context

This change allows the WriteCodeFragment task to define assembly attributes that require parameters that are not of type System.String. For example, CSLCompliantAttribute can be generated with a parameter of true instead of "true".

Changes Made

Additional metadata can be defined on an AssemblyAttribute that specifies how to treat the parameters specified in the metadata. There are three different ways that the parameters can be treated.

Infer the Type

Without specifying any additional metadata, attributes that are defined in the mscorlib assembly (i.e. types that can be loaded via System.Type.GetType(string)) will have their parameter types inferred by finding the constructor where the parameter count matches the number of parameters specified in the metadata. For example, this:

<ItemGroup>
  <AssemblyAttribute Include="CLSCompliantAttribute">
    <_Parameter1>true</_Parameter1>
  </AssemblyAttribute>
</ItemGroup>

Will produce the code:

[assembly: CLSCompliantAttribute(true)]

For backward-compatibility, if the attribute cannot be found, or no matching constructor is found, the parameter is treated as a string.

Declare the Type

An additional metadata item can be used to specify the full name of the parameter type. To do this, add a metadata item that has the same name as the parameter with "_TypeName" appended to the end. For example, this:

<ItemGroup>
  <AssemblyAttribute Include="TestAttribute">
    <_Parameter1>True</_Parameter1>
    <_Parameter1_TypeName>System.Boolean</_Parameter1_TypeName>
  </AssemblyAttribute>
</ItemGroup>

Will produce the code:

[assembly: TestAttribute(true)]

This also works with named parameters:

<ItemGroup>
  <AssemblyAttribute Include="TestAttribute">
    <Foo>42</IdentifyLevel>
    <Foo_TypeName>System.Int32</Foo_TypeName>
  </AssemblyAttribute>
</ItemGroup>
[assembly: TestAttribute(42)]

All types that can be used as attribute parameters are supported, except for arrays.

For backward-compatibility, if a metadata item ends with "_TypeName", but there is no metadata item for the parameter with that name, then it will be treated as another named property. For example, this:

<ItemGroup>
  <AssemblyAttribute Include="TestAttribute">
    <Foo_TypeName>System.Int32</Foo_TypeName>
  </AssemblyAttribute>
</ItemGroup>

Would produce the code:

[assembly: TestAttribute(Foo_TypeName="System.Int32")]

Specify the Exact Code

For cases where declaring the type is insufficient (such as when the parameter is an array), you can specify the exact that that will be generated for the parameter by adding metadata that has the same name as the parameter with "_IsLiteral" appended to the end. For example, this:

<ItemGroup>
  <AssemblyAttribute Include="TestAttribute">
    <_Parameter1>new int[] { 1, 3, 5 } /* odd numbers */</_Parameter1>
    <_Parameter1_IsLiteral>true</_Parameter1_IsLiteral>
  </AssemblyAttribute>
</ItemGroup>

Will produce the code:

[assembly: TestAttribute(new int[] { 1, 3, 5 } /* odd numbers */)]

The limitation with this is that the code you provide is language-specific. For example, the literal value in the metadata above will only work in C#. If you used that same metadata in a VB.NET project, you would receive a compiler error.

This works with both positional and named parameters. As with the ..._TypeName metadata, if an ..._IsLiteral metadata does not have a corresponding parameter name, it will be treated as a named parameter for backward-compatibility.

Mixed Parameter Behavior

Because the additional metadata only applies to a specific parameter, you can choose to treat different parameters in different ways. For example, you can infer/use the default behavior for one parameter, specify the type for the second parameter and use a literal value for the third. For example:

<ItemGroup>
  <AssemblyAttribute Include="TestAttribute">
    <_Parameter1>This is a string</_Parameter1>
    <_Parameter2>42></Parameter2>
    <_Parameter2_TypeName>System.Int32</_Parameter2_TypeName>
    <_Parameter3>new int[] { 1 }</_Parameter3>
    <_Parameter3_IsLiteral>true</_Parameter3_IsLiteral>
  </AssemblyAttribute>
</ItemGroup>

Testing

I've added tests for inferring the parameter type, declaring the parameter type and using literal values. I've also added tests to confirm backward-compatibility is maintained where it's possible to do so.

The new tests I added all use helper methods to reduce the boilerplate that the existing tests had. The existing tests could also be changed to use these new methods, but I've left them as is to reduce the amount of changes in this PR.

Notes

I'm not sure if "IsLiteral" is the best terminology, but the general consensus in #2281 was that it was a good term to use, so I stuck with it. I'm happy to change it to something else if needed.

Copy link
Member

@Forgind Forgind left a comment

This code is very well-written, and you have an excellent description—thank you for that. I had no understanding of the WriteCodeFragment task going in, but I'm reasonably confident that, assuming valid inputs, this doesn't break existing behavior, and I think it adds support for additional types as you describe.

src/Tasks/WriteCodeFragment.cs Outdated Show resolved Hide resolved
AttributeParameter parameter = parameters[i];
CodeExpression value;

switch (parameter.Type.Kind)

This comment has been minimized.

@Forgind

Forgind Mar 29, 2021
Member

nit:
This could be cleaner with the pattern:
CodeExpression value = parameter.Type.Kind switch
{
ParameterTypeKind.Literal => new CodeSnippetExpression(parameter.Value),
ParameterTypeKind.Typed => parameter.Type.TypeName.Equals("System.Type") ?
...
}

This comment has been minimized.

@reduckted

reduckted Apr 3, 2021
Author Contributor

I'm not sure this is possible because the case for ParameterTypeKind.Typed can return false instead of setting value.

This comment has been minimized.

@Forgind

Forgind Apr 3, 2021
Member

For the return false case, you had previously called TryConvertParameterValue(parameter.Type.TypeName, parameter.Value, out value), which would set value.

This comment has been minimized.

@reduckted

reduckted Apr 4, 2021
Author Contributor

The default case causes problems here because it delays finding the positional parameters until they are needed, and that can't (as far as I'm aware) be put in a switch expression. This could be worked around using a Lazy<Type[]> for the positional parameter types, but that would result in an allocation that could be avoided in some situations.

I'm happy to go ahead and make that change, but just thought I would check if it's worth the extra allocation. 😄

This comment has been minimized.

@Forgind

Forgind Apr 5, 2021
Member

I was thinking something like:

int t = 0;
char[] b = null;
int k = t switch
{
        2 => 3,
         _ => otherFunc((b = b is null ? new char[] { 'A', 'B' } : b)[0])
};

That's a little messy, but I think it would be cleaner. Probably have to write out everything to see for sure, and if you are happy with it this way, I am, too.

src/Tasks/WriteCodeFragment.cs Outdated Show resolved Hide resolved
src/Tasks/WriteCodeFragment.cs Outdated Show resolved Hide resolved
src/Tasks/WriteCodeFragment.cs Show resolved Hide resolved
src/Tasks/WriteCodeFragment.cs Outdated Show resolved Hide resolved
@Forgind Forgind merged commit c4cda20 into dotnet:main Apr 8, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210404.1 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
@Forgind
Copy link
Member

@Forgind Forgind commented Apr 8, 2021

Thank you @reduckted!

@billybraga
Copy link

@billybraga billybraga commented Apr 8, 2021

How can I know when this will be available? Ideally I'd subscribe to something somewhere on github to be alerted when I can start using this.

@Forgind
Copy link
Member

@Forgind Forgind commented Apr 8, 2021

This should be available in 16.10 preview 3. I will try to remember to ping you in this thread when that's available. That should be roughly a month from now.

@reduckted reduckted deleted the reduckted:write-code-fragment-non-string-parameters branch Apr 8, 2021
@0xced
Copy link

@0xced 0xced commented Apr 12, 2021

available in 16.10 preview 3

Are you referring to the MSBuild or Visual Studio version? Will it be part of the next .NET 6.0 SDK preview release?

@Forgind
Copy link
Member

@Forgind Forgind commented Apr 15, 2021

MSBuild versions and Visual Studio versions line up nicely, so both.

Will it be part of the next .NET 6.0 SDK preview release?

That's a good question. I think it will be in preview 4, but I'm not 100% sure on that. @dsplaisted, do you know?

@dsplaisted
Copy link
Member

@dsplaisted dsplaisted commented Apr 16, 2021

@marcpopMSFT for the release alignment question.

@Forgind
Copy link
Member

@Forgind Forgind commented May 13, 2021

16.10 preview 3 has been released! The .NET 6 preview 4 SDK hasn't been released yet.

/cc: @billybraga

@marcpopMSFT
Copy link
Contributor

@marcpopMSFT marcpopMSFT commented May 14, 2021

When the .NET 6 preview 4 SDK releases, it should include this fix as we'll be shipping with an MSBuild from the first week of May it looks like.

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

Successfully merging this pull request may close these issues.

7 participants