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.

Sign up for GitHub

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 RegexGenerator to System.ComponentModel.TypeConverter. #62325

Merged
merged 8 commits into from
Feb 24, 2022
Merged

Add RegexGenerator to System.ComponentModel.TypeConverter. #62325

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2c41703
Add RegGen to System.ComponentModel.TypeConverter.
Clockwork-Muse Dec 3, 2021
75b9dda
Change to targets generator enablement
Clockwork-Muse Dec 4, 2021
177180d
Correct RegGen for System.ComponentModel.TypeConverter
Clockwork-Muse Dec 4, 2021
5b96443
Remove extra space
Clockwork-Muse Dec 7, 2021
95cadd0
Remove .NetF inclusion definition
Clockwork-Muse Jan 4, 2022
154b7fd
Remove implicit regex generator entrypoint
Clockwork-Muse Feb 8, 2022
8863e8f
Remove regex generator enabled variable target.
Clockwork-Muse Feb 8, 2022
e4af436
Update eng/generators.targets
joperezr Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion eng/generators.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,34 @@
and @(ProjectReference->AnyHaveMetadataValue('Identity', '$(CoreLibProject)'))))" Include="$(LibrariesProjectRoot)Common\src\System\Runtime\InteropServices\ArrayMarshaller.cs" />
</ItemGroup>

<ItemGroup>
<EnabledGenerators Include="RegexGenerator" Condition="'$(EnableRegexGenerator)' == 'true'" />
Copy link
Member

Choose a reason for hiding this comment

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

@jkoritzinsky what is the reasoning behind having an explicit path and an implicit path for adding the DLLImport generator? Is that reason also applicable to the RegexGenerator? Basically my question is: for the regexgenerator should we just simplify and only enable it whenever EnableRegexGenerator property is set to true?

Copy link
Member

Choose a reason for hiding this comment

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

We needed the ItemGroup-based path to allow us to filter adding the generator ProjectReference based on referenced projects. With MSBuild static graph (which we use for our restore functionality), we can't add a ProjectReference in a target, so we're limited to evaluation time. Since properties are evaluated before items, we needed to do the generator reference add based on an item. The property was added for simplicity.

Once we aren't adding sources to builds (and all of the APIs the generator uses are approved) then we can significantly simplify this file.

Copy link
Member

Choose a reason for hiding this comment

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

@jkoritzinsky it sounds like you wanted to make it automatic always and you see the property as less desirable, where @joperezr was saying the property is simpler. I do think a property is more WYSIWYG, and it seems acceptable for construction of the shared framework where we are more careful about layering/dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we wanted to make DllImportGenerator enabled automatically and the property was a fallback. We wanted to enable our generator automatically since we're consuming it throughout the runtime, including CoreLib. @stephentoub expressed a desire for us to enable it for all projects (including tests) and not even having a property for opt-in/out, but until we get all of our APIs approved, that's a little difficult.

<!-- Enable the RegexGenerator source generator when the project is a C# source project that references System.Text.RegularExpressions -->
<EnabledGenerators Include="RegexGenerator"
Clockwork-Muse marked this conversation as resolved.
Show resolved Hide resolved
Condition="'$(EnableRegexGenerator)' == ''
and '$(IsFrameworkSupportFacade)' != 'true'
and '$(IsSourceProject)' == 'true'
and '$(MSBuildProjectExtension)' == '.csproj'
and ('@(Reference)' != '' and @(Reference->AnyHaveMetadataValue('Identity', 'System.Text.RegularExpressions')))" />
<EnabledGenerators Include="RegexGenerator"
Condition="'$(EnableRegexGenerator)' == ''
and '$(IsFrameworkSupportFacade)' != 'true'
and '$(IsSourceProject)' == 'true'
and '$(MSBuildProjectExtension)' == '.csproj'
and ('$(TargetFrameworkIdentifier)' == '.NETStandard' or '$(TargetFrameworkIdentifier)' == '.NETFramework')" />
Copy link
Member

Choose a reason for hiding this comment

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

@eerhardt and I had some offline discussions about making source generators available to frameworks where they actually can't really be consumed outside of dotnet/runtime (which is the case for the Regex one) and I believe we landed on the idea that we shouldn't do this, and instead either: a) limit the source generator to only be used on platforms where it can be consumed externally, or b) expose it to other frameworks if we would see value for adding them.

If you agree, can we limit this change to only add the source generator in the netcoreapp case? (basically remove changes from line 75-80 here)

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'm fine with this, but what effect is this going to have in other areas, like tests? If a project has net48 in TargetFrameworks, would that mean it couldn't be used there? What about tests that have PlatformDetection.IsNetFramework?

Copy link
Member

Choose a reason for hiding this comment

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

If a project has net48 in TargetFrameworks, would that mean it couldn't be used there?

Correct. That behavior aligns with our customers' experience. The same goes for netstandard2.0 and net6.0. Only net7.0+ targets should be able to consume the RegexGenerator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, so any project that is expected to reference .NetF at all can't use the generator unless we get fancy with #ifdefs, then, which is something I'll have to consider for some of the other test projects.

Copy link
Member

Choose a reason for hiding this comment

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

As an aside, while technically at the moment the generated code would likely work downlevel, changes are coming that will end up using new API surface area in .NET 7, so the generator will only be usable with .NET 7+.

</ItemGroup>

<!-- Use this complex ItemGroup-based filtering to add the ProjectReference to make sure dotnet/runtime stays compatible with NuGet Static Graph Restore. -->
<ItemGroup Condition="'@(EnabledGenerators)' != ''
and @(EnabledGenerators->AnyHaveMetadataValue('Identity', 'RegexGenerator'))">
<ProjectReference
Include="$(LibrariesProjectRoot)System.Text.RegularExpressions/gen/System.Text.RegularExpressions.Generator.csproj"
OutputItemType="Analyzer"
ReferenceOutputAssembly="false" />
</ItemGroup>

<Target Name="ConfigureGenerators"
DependsOnTargets="ConfigureDllImportGenerator"
DependsOnTargets="ConfigureDllImportGenerator;ConfigureRegexGenerator"
joperezr marked this conversation as resolved.
Show resolved Hide resolved
BeforeTargets="CoreCompile" />

<!-- Microsoft.Interop.DllImportGenerator -->
Expand Down Expand Up @@ -94,5 +120,15 @@
</PropertyGroup>
</Target>

<!-- System.Text.RegularExpressions -->
<Target Name="ConfigureRegexGenerator"
Clockwork-Muse marked this conversation as resolved.
Show resolved Hide resolved
Condition="'@(EnabledGenerators)' != '' and @(EnabledGenerators->AnyHaveMetadataValue('Identity', 'RegexGenerator'))"
DependsOnTargets="ResolveProjectReferences"
BeforeTargets="GenerateMSBuildEditorConfigFileShouldRun">
<PropertyGroup>
<DefineConstants>$(DefineConstants);REGEXGENERATOR_ENABLED</DefineConstants>
Copy link
Member

Choose a reason for hiding this comment

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

I know you were just following what DllImport one is doing here, but it doesn't seem like you are using this constant now and I'd only see it useful whenever a project is multitargeting and wants to conditionally compile the implementation of the regex, which I think would probably more sense to do via conditionally compiled files depending on the EnableRegexGenerator property instead.

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'm fine with removing "it" - what's "it" though, just the constant or that entire target? That is, is that target there just to add that constant?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really see a scenario for this define or the target, I'd vote to remove them.

</PropertyGroup>
</Target>

<Import Project="$(LibrariesProjectRoot)System.Runtime.InteropServices/gen/DllImportGenerator/Microsoft.Interop.DllImportGenerator.props" />
</Project>
1 change: 1 addition & 0 deletions ...raries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<Nullable>enable</Nullable>
<EnableRegexGenerator>true</EnableRegexGenerator>
</PropertyGroup>
<ItemGroup>
<ILLinkSubstitutionsXmls Include="$(ILLinkDirectory)ILLink.Substitutions.xml" />
Expand Down
13 changes: 7 additions & 6 deletions ...ries/System.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesignerVerb.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,26 @@ namespace System.ComponentModel.Design
/// <summary>
/// Represents a verb that can be executed by a component's designer.
/// </summary>
public class DesignerVerb : MenuCommand
public partial class DesignerVerb : MenuCommand
{
/// <summary>
/// Initializes a new instance of the <see cref='System.ComponentModel.Design.DesignerVerb'/> class.
/// </summary>
public DesignerVerb(string text, EventHandler handler) : base(handler, StandardCommands.VerbFirst)
{
Properties["Text"] = text == null ? null : Regex.Replace(text, @"\(\&.\)", "");
}
public DesignerVerb(string text, EventHandler handler) : this(text, handler, StandardCommands.VerbFirst) { }

/// <summary>
/// Initializes a new instance of the <see cref='System.ComponentModel.Design.DesignerVerb'/>
/// class.
/// </summary>
public DesignerVerb(string text, EventHandler handler, CommandID startCommandID) : base(handler, startCommandID)
{
Properties["Text"] = text == null ? null : Regex.Replace(text, @"\(\&.\)", "");
Properties["Text"] = text == null ? null : GetParameterReplacementRegex().Replace(text, "");
}

[RegexGenerator(@"\(\&.\)")]
private static partial Regex GetParameterReplacementRegex();


/// <summary>
Clockwork-Muse marked this conversation as resolved.
Show resolved Hide resolved
/// Gets or sets the description of the menu item for the verb.
/// </summary>
Expand Down