Add RegexGenerator to System.ComponentModel.TypeConverter. #62325

Clockwork-Muse · 2021-12-03T07:12:55Z

Added generator to source library, not to tests (no tests found)

Added to src, not to test (no test found)

ghost · 2021-12-03T07:13:02Z

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

Issue Details

Added generator to source library, not to tests (no tests found)

Author:	Clockwork-Muse
Assignees:	-
Labels:	`area-System.ComponentModel`, `community-contribution`
Milestone:	-

danmoseley · 2021-12-03T07:24:09Z

Seems reasonable to me -- @stephentoub style OK for you?

Cool that this will be our first use in actual product.

...braries/System.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesignerVerb.cs

stephentoub · 2021-12-03T11:25:04Z

...libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj

@@ -261,5 +261,6 @@
    <Reference Include="System.Threading" />
    <Reference Include="System.Threading.Thread" />
    <Reference Include="System.Xml.XDocument" />
+    <ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions/gen/System.Text.RegularExpressions.Generator.csproj"  OutputItemType="Analyzer" ReferenceOutputAssembly="false" />


Why is this needed? If memory serves, we don't modify every project to be able to use e.g. DllImportGenerator. I believe it's configured in a central location to be usable everywhere.

See

runtime/eng/generators.targets

Lines 9 to 30 in cc2cb04

<EnabledGenerators Include="DllImportGenerator" Condition="'$(EnableDllImportGenerator)' == 'true'" />



<EnabledGenerators Include="DllImportGenerator"

Condition="'$(EnableDllImportGenerator)' == ''

and '$(IsFrameworkSupportFacade)' != 'true'

and '$(IsSourceProject)' == 'true'

and '$(MSBuildProjectExtension)' == '.csproj'

and (

('@(Reference)' != ''

and @(Reference->AnyHaveMetadataValue('Identity', 'System.Runtime.InteropServices')))

or ('@(ProjectReference)' != ''

and @(ProjectReference->AnyHaveMetadataValue('Identity', '$(CoreLibProject)'))))" />

<EnabledGenerators Include="DllImportGenerator"

Condition="'$(EnableDllImportGenerator)' == ''

and '$(IsFrameworkSupportFacade)' != 'true'

and '$(IsSourceProject)' == 'true'

and '$(MSBuildProjectExtension)' == '.csproj'

and ('$(TargetFrameworkIdentifier)' == '.NETStandard' or '$(TargetFrameworkIdentifier)' == '.NETFramework')" />

</ItemGroup>

for how that works.

Yeah, instead of having individual ProjectReferences on each project that uses it, we should instead update generators.targets to have all projects be able to consume the source generator.

Ah, thanks.
I originally did it as a project reference because I ran across the dll one in a PR, before I saw the recommendations. But yes, this would be much better.

I've changed to adding the targets to that file, and it works, but I'm not very experienced with some of the more arcane things in build files. I know this is going to be reviewed anyways, but please be thorough.

joperezr · 2021-12-03T23:49:24Z

I understand it might be hard to do for every one of these PRs, but it would be cool to have a sense of the specific case perf gains out of onboarding the source generator.

joperezr · 2021-12-06T19:23:38Z

eng/generators.targets

+                        and '$(IsFrameworkSupportFacade)' != 'true'
+                        and '$(IsSourceProject)' == 'true'
+                        and '$(MSBuildProjectExtension)' == '.csproj'
+                        and ('$(TargetFrameworkIdentifier)' == '.NETStandard' or '$(TargetFrameworkIdentifier)' == '.NETFramework')" />


@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)

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?

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.

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.

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

joperezr · 2021-12-06T19:27:42Z

eng/generators.targets

@@ -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'" />


@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?

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.

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

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.

joperezr · 2021-12-06T19:30:04Z

eng/generators.targets

+          DependsOnTargets="ResolveProjectReferences"
+          BeforeTargets="GenerateMSBuildEditorConfigFileShouldRun">
+    <PropertyGroup>
+      <DefineConstants>$(DefineConstants);REGEXGENERATOR_ENABLED</DefineConstants>


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.

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?

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

Clockwork-Muse · 2021-12-07T01:23:37Z

I can simplify the targets file (and it has to be done anyways), but I took another look at this, and I'm not sure whether regex is even required here.

The pattern is "$\&.$" - note only the single . pattern - but the tests are only testing with "(&.)", which makes it look like . should actually be escaped (possibly instead of &, because that character doesn't need escaping). If so, that means the use of regex could be removed entirely, and a simple string.Replace() could be used instead. I can't find any uses of "(&.)", or even anything "(&<anything>)", so I'm unsure if even the Replace() is required, but I'm not at all familiar with this part of the code base, and don't know what I should be looking for here.

I can find at least one identical usage in reference source, but this file is not in this repository, so at least some use seems to be in outside packages.

Thoughts?

danmoseley · 2021-12-07T03:04:15Z

I was thinking maybe this is for strings containing an embedded keyboard shortcut, eg C&opy Text means that o is the keyboard shortcut for a menu item, and the text is Copy Text. But then the regex would be & ... (hardly worth a regex)

@DustinCampbell perhaps you know about DesignerVerbs and what this might be for?

danmoseley · 2021-12-07T03:04:38Z

I'd be fine with merging as-is as the behavior is not changing..

...braries/System.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesignerVerb.cs

Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>

DustinCampbell · 2021-12-07T18:47:50Z

I was thinking maybe this is for strings containing an embedded keyboard shortcut, eg C&opy Text means that o is the keyboard shortcut for a menu item, and the text is Copy Text. But then the regex would be & ... (hardly worth a regex)

@DustinCampbell perhaps you know about DesignerVerbs and what this might be for?

You've understood it correctly @danmoseley. The & marks the next character as a keyboard accelerator character when a DesignerVerb is surfaced in UI (such as in VS context menus).

danmoseley · 2021-12-07T19:51:51Z

@DustinCampbell to be clear then, the code could/should simply be .Replace('&', '') with no regex involved (let alone the current broken one)

Clockwork-Muse · 2021-12-07T20:13:00Z

let alone the current broken one

For clarity, broken because:

It also expects enclosing parenthesis
It removes the accelerator character during the replace.

danmoseley · 2021-12-07T21:11:06Z

let alone the current broken one

For clarity, broken because:

It also expects enclosing parenthesis

It removes the accelerator character during the replace.

I was thinking of both. Although it's so broken for that purpose one wonders how it could be. It's not the first time we look at some dusty corner of old code and it's not clear why something is some way.

If @DustinCampbell (from Winforms/designer world) thinks it's OK to just replace & with '' then I suggest we do that. We have months to discover if that breaks something.

Clockwork-Muse · 2021-12-08T17:21:40Z

thinks it's OK to just replace & with '' then I suggest we do that.

I can't believe it took me this long to spot the problem with this: it fails if the ampersand is used "for itself" - If the text is (or should be) "Build & Run Tests", for example. Can we guarantee that we don't have any such uses, or won't? Or is the verb guaranteed to be a single word?

maryamariyan · 2022-01-11T23:56:27Z

Would be good to get sign off from memebers of @dotnet/area-infrastructure-libraries for this PR.

eerhardt · 2022-02-07T16:05:55Z

@joperezr @safern @carlossanlop - any thoughts here? I think this is just waiting for sign off from the libraries infrastructure team.

joperezr · 2022-02-07T18:19:08Z

I will take a look at this today. Apologies for missing the previous notification.

eng/generators.targets

joperezr

Left two small comments, but other than that this seems ready to go from my side. Thanks a lot @Clockwork-Muse for your patience and work on this!

Clockwork-Muse · 2022-02-08T07:39:56Z

Pushed changes as requested by @joperezr .

Currently, this PR performs no behavioral changes, simply switching to the regex generator. However, there are still questions about what the correct behavior should be, and whether regex is even required in this situation. Should I open an issue to have someone look at that?

stephentoub · 2022-02-08T12:41:28Z

Should I open an issue to have someone look at that?

Sure.

eng/generators.targets

Clockwork-Muse · 2022-02-13T20:12:12Z

Error just seems to be a build timeout?

danmoseley · 2022-02-14T00:48:26Z

Indeed. @dotnet/dnceng can you make anything of this? I can't find any more info

2022-02-08T20:26:47.5338090Z   Sending Job to OSX.1200.Amd64.Open...
2022-02-08T20:27:02.6974010Z   Sent Helix Job; see work items at https://helix.dot.net/api/jobs/fc4588bb-fb88-41d9-8528-65d9db3d9c28/workitems?api-version=2019-06-17
2022-02-08T20:27:03.7159050Z   Waiting for completion of job fc4588bb-fb88-41d9-8528-65d9db3d9c28 on OSX.1200.Amd64.Open
2022-02-08T22:51:52.0130490Z ##[error]The operation was canceled.
2022-02-08T22:51:52.0201180Z ##[section]Finishing: Send to Helix

lpatalas · 2022-02-14T11:06:36Z

@danmoseley We'll take all look.

michellemcdaniel · 2022-02-14T15:49:05Z

This looks like it happened at one of the times where the os.1200.amd64.open queue was super backed up and had to be purged.

MattGal · 2022-02-14T17:31:58Z

This looks like it happened at one of the times where the os.1200.amd64.open queue was super backed up and had to be purged.

It's definitely this, turning off core dumps in runtime unblocked it because System.Runtime.InteropServices test was just spending hours uploading dumps on every machine.

lpatalas · 2022-02-14T17:36:17Z

That makes sense. I created an issue to track the investigation here https://github.com/dotnet/core-eng/issues/15567 but I guess we can close it in this situation.

danmoseley · 2022-02-14T17:55:19Z

Thank you @lpatalas @adiaaida

Clockwork-Muse · 2022-02-20T21:31:08Z

On main, the output item type has changed for the dll generator. I'm assuming I should change things to match?

eerhardt

The changes in ComponentModel LGTM.

@dotnet/area-infrastructure-libraries - I don't see an approval there. Is everyone OK with the infra changes in this PR?

joperezr

Infra related changes look good to me.

Add RegGen to System.ComponentModel.TypeConverter.

2c41703

Added to src, not to test (no test found)

dotnet-issue-labeler bot added the area-System.ComponentModel label Dec 3, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 3, 2021

Clockwork-Muse mentioned this pull request Dec 3, 2021

Start using the Regex source generator in this repo #62105

Closed

stephentoub reviewed Dec 3, 2021

View reviewed changes

...braries/System.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesignerVerb.cs Outdated Show resolved Hide resolved

stephentoub reviewed Dec 3, 2021

View reviewed changes

...braries/System.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesignerVerb.cs Outdated Show resolved Hide resolved

stephentoub reviewed Dec 3, 2021

View reviewed changes

Clockwork-Muse added 2 commits December 4, 2021 03:31

Change to targets generator enablement

75b9dda

Correct RegGen for System.ComponentModel.TypeConverter

177180d

joperezr reviewed Dec 6, 2021

View reviewed changes

kasperk81 reviewed Dec 7, 2021

View reviewed changes

...braries/System.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesignerVerb.cs Outdated Show resolved Hide resolved

Remove extra space

5b96443

Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>

Remove .NetF inclusion definition

95cadd0

Clockwork-Muse mentioned this pull request Jan 24, 2022

Inject RegexGeneratorAttribute from the generator #64229

Closed

joperezr reviewed Feb 7, 2022

View reviewed changes

eng/generators.targets Outdated Show resolved Hide resolved

joperezr reviewed Feb 7, 2022

View reviewed changes

eng/generators.targets Outdated Show resolved Hide resolved

joperezr reviewed Feb 7, 2022

View reviewed changes

Clockwork-Muse added 2 commits February 8, 2022 07:33

Remove implicit regex generator entrypoint

154b7fd

Remove regex generator enabled variable target.

8863e8f

joperezr reviewed Feb 8, 2022

View reviewed changes

eng/generators.targets Outdated Show resolved Hide resolved

Update eng/generators.targets

e4af436

Clockwork-Muse mentioned this pull request Feb 8, 2022

System.ComponentModel.TypeConverter.DesignerVerb text replacement potentially erroneous #65015

Open

eerhardt approved these changes Feb 23, 2022

View reviewed changes

joperezr approved these changes Feb 24, 2022

View reviewed changes

stephentoub approved these changes Feb 24, 2022

View reviewed changes

stephentoub merged commit a06befc into dotnet:main Feb 24, 2022

Clockwork-Muse deleted the regex_gen_62105_first branch February 24, 2022 16:41

ghost locked as resolved and limited conversation to collaborators Mar 26, 2022

	<EnabledGenerators Include="DllImportGenerator" Condition="'$(EnableDllImportGenerator)' == 'true'" />
	<!-- If the current project is not System.Private.CoreLib, we enable the DllImportGenerator source generator
	when the project is a C# source project that either:
	- references System.Private.CoreLib, or
	- references System.Runtime.InteropServices -->
	<EnabledGenerators Include="DllImportGenerator"
	Condition="'$(EnableDllImportGenerator)' == ''
	and '$(IsFrameworkSupportFacade)' != 'true'
	and '$(IsSourceProject)' == 'true'
	and '$(MSBuildProjectExtension)' == '.csproj'
	and (
	('@(Reference)' != ''
	and @(Reference->AnyHaveMetadataValue('Identity', 'System.Runtime.InteropServices')))
	or ('@(ProjectReference)' != ''
	and @(ProjectReference->AnyHaveMetadataValue('Identity', '$(CoreLibProject)'))))" />
	<EnabledGenerators Include="DllImportGenerator"
	Condition="'$(EnableDllImportGenerator)' == ''
	and '$(IsFrameworkSupportFacade)' != 'true'
	and '$(IsSourceProject)' == 'true'
	and '$(MSBuildProjectExtension)' == '.csproj'
	and ('$(TargetFrameworkIdentifier)' == '.NETStandard' or '$(TargetFrameworkIdentifier)' == '.NETFramework')" />
	</ItemGroup>

Add RegexGenerator to System.ComponentModel.TypeConverter. #62325

Add RegexGenerator to System.ComponentModel.TypeConverter. #62325

Conversation

Clockwork-Muse commented Dec 3, 2021

ghost commented Dec 3, 2021

danmoseley commented Dec 3, 2021

stephentoub Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joperezr commented Dec 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Clockwork-Muse commented Dec 7, 2021

danmoseley commented Dec 7, 2021

danmoseley commented Dec 7, 2021

DustinCampbell commented Dec 7, 2021

danmoseley commented Dec 7, 2021

Clockwork-Muse commented Dec 7, 2021

danmoseley commented Dec 7, 2021

Clockwork-Muse commented Dec 8, 2021

maryamariyan commented Jan 11, 2022

eerhardt commented Feb 7, 2022

joperezr commented Feb 7, 2022

joperezr left a comment • edited Loading

Choose a reason for hiding this comment

Clockwork-Muse commented Feb 8, 2022

stephentoub commented Feb 8, 2022

Clockwork-Muse commented Feb 13, 2022

danmoseley commented Feb 14, 2022

lpatalas commented Feb 14, 2022

michellemcdaniel commented Feb 14, 2022

MattGal commented Feb 14, 2022

lpatalas commented Feb 14, 2022

danmoseley commented Feb 14, 2022

Clockwork-Muse commented Feb 20, 2022

eerhardt left a comment

Choose a reason for hiding this comment

joperezr left a comment

Choose a reason for hiding this comment

stephentoub Dec 3, 2021 •

edited

Loading

joperezr left a comment •

edited

Loading