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

reduckted · 2021-03-20T01:08:16Z

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.

Forgind

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

Forgind · 2021-03-29T14:52:21Z

src/Tasks/WriteCodeFragment.cs

+                AttributeParameter parameter = parameters[i];
+                CodeExpression value;
+
+                switch (parameter.Type.Kind)


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") ?
...
}

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

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

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. 😄

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

…d allocations.

… inferring parameter types.

src/Tasks/WriteCodeFragment.cs

Forgind · 2021-04-08T16:09:15Z

Thank you @reduckted!

billybraga · 2021-04-08T18:40:41Z

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 · 2021-04-08T20:06:35Z

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.

0xced · 2021-04-12T21:35:48Z

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 · 2021-04-15T22:30:04Z

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 · 2021-04-16T02:50:38Z

@marcpopMSFT for the release alignment question.

Forgind · 2021-05-13T22:40:02Z

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

/cc: @billybraga

marcpopMSFT · 2021-05-14T21:18:34Z

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.

Crono1981 · 2021-09-18T15:15:55Z

Hey @reduckted , thanks for a really nice contribution!

Quick question: is this supposed to work for F# projects as well?

reduckted · 2021-09-19T00:35:08Z

@Crono1981 Good question! I just had a look into it, and no, it doesn't work for F#. It looks like F# has its own WriteCodeFragment task that's based on an older version of this task.

Crono1981 · 2021-09-19T22:38:31Z

@reduckted I see. I'll document my plugin accordingly. Thanks for confirming.

It would be possible to set the `CLSCompliantAttribute` to true in the csproj directly with the .NET 6 SDK and get rid of AssemblyInfo.cs but that's 5 lines of XML, see dotnet/msbuild#6285 So let's keep it simple with a one line assembly attribute instead.

RussKie · 2023-06-29T00:03:29Z

Is this feature documented anywhere at learn.microsoft.com? It was rather non-trivial to find this implementation, but I couldn't find any public docs for it.

rainersigwald · 2023-06-29T14:29:38Z

@RussKie good catch: MicrosoftDocs/visualstudio-docs#9379.

Allow parameter type name to be specified for WriteCodeFragment task.

c684258

reduckted mentioned this pull request Mar 20, 2021

Add support for non-string assembly attributes #2281

Closed

Forgind approved these changes Mar 29, 2021

View reviewed changes

dev-in-disguise mentioned this pull request Apr 1, 2021

Comply to CLS Flash0ver/F0.Minesweeper#63

Open

reduckted added 3 commits April 4, 2021 14:05

Readability improvements.

ece1f59

Delay-created the collections when extracting parameter types to avoi…

38e5d33

…d allocations.

Added a better explanation about selecting the first constructor when…

d41cb3a

… inferring parameter types.

KirillOsenkov reviewed Apr 4, 2021

View reviewed changes

src/Tasks/WriteCodeFragment.cs Outdated Show resolved Hide resolved

KirillOsenkov approved these changes Apr 4, 2021

View reviewed changes

Added argument names to improve readability.

4a1913e

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 5, 2021

Forgind merged commit c4cda20 into dotnet:main Apr 8, 2021

reduckted deleted the write-code-fragment-non-string-parameters branch April 8, 2021 21:40

rainersigwald mentioned this pull request Jun 29, 2023

WriteCodeFragment: Document _TypeName metadata MicrosoftDocs/visualstudio-docs#9379

Closed

KalleOlaviNiemitalo mentioned this pull request Feb 9, 2024

[Bug]: WriteCodeFragment - assembly attribute doesn't work for F# #9724

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

reduckted commented Mar 20, 2021

Forgind left a comment

Forgind Mar 29, 2021

reduckted Apr 3, 2021

Forgind Apr 3, 2021

reduckted Apr 4, 2021

Forgind Apr 5, 2021 •

edited

Forgind commented Apr 8, 2021

billybraga commented Apr 8, 2021

Forgind commented Apr 8, 2021 •

edited

0xced commented Apr 12, 2021

Forgind commented Apr 15, 2021

dsplaisted commented Apr 16, 2021

Forgind commented May 13, 2021

marcpopMSFT commented May 14, 2021

Crono1981 commented Sep 18, 2021

reduckted commented Sep 19, 2021

Crono1981 commented Sep 19, 2021

RussKie commented Jun 29, 2023

rainersigwald commented Jun 29, 2023

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

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

Conversation

reduckted commented Mar 20, 2021

Context

Changes Made

Infer the Type

Declare the Type

Specify the Exact Code

Mixed Parameter Behavior

Testing

Notes

Forgind left a comment

Choose a reason for hiding this comment

Forgind Mar 29, 2021

Choose a reason for hiding this comment

reduckted Apr 3, 2021

Choose a reason for hiding this comment

Forgind Apr 3, 2021

Choose a reason for hiding this comment

reduckted Apr 4, 2021

Choose a reason for hiding this comment

Forgind Apr 5, 2021 • edited

Choose a reason for hiding this comment

Forgind commented Apr 8, 2021

billybraga commented Apr 8, 2021

Forgind commented Apr 8, 2021 • edited

0xced commented Apr 12, 2021

Forgind commented Apr 15, 2021

dsplaisted commented Apr 16, 2021

Forgind commented May 13, 2021

marcpopMSFT commented May 14, 2021

Crono1981 commented Sep 18, 2021

reduckted commented Sep 19, 2021

Crono1981 commented Sep 19, 2021

RussKie commented Jun 29, 2023

rainersigwald commented Jun 29, 2023

Forgind Apr 5, 2021 •

edited

Forgind commented Apr 8, 2021 •

edited