Correctly handle polyfilled required infrastructure attributes

[DoctorVanGogh]

This fixes #1420

---

Turned out it was more than a single place where RequiredMemberAttribute/SetsRequiredMemberAttribute were used. Centralized the logic and removed any NET7+ conditional constraints on source & tests.

Tests include a .NET6 Target Framework which has no inbuilt support for required.
Added the polyfill nuget package in there to allow use of required and have tests subsequently passing on NET6.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.55%. Comparing base (ec4907f) to head (e68c1b8).
Report is 7 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1421      +/-   ##
===========================================
- Coverage    78.57%   78.55%   -0.03%     
===========================================
  Files          200      200              
  Lines         5713     5716       +3     
  Branches      1168     1168              
===========================================
+ Hits          4489     4490       +1     
- Misses         712      713       +1     
- Partials       512      513       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[DoctorVanGogh]

Small addendum:
I've looked at ReflectionActivator.cs and I cannot make sense of its check for required members.

Even before 4547d16 a check was done on a type for a RequiredMemberAttribute. Types don't carry that attribute. Those attributes are on fields or properties.:

The required modifier can be applied to fields and properties ...¹

I've tweaked this check slightly in DoctorVanGogh/Autofac@1608ec7. Should that be rolled into this PR?

Would it make sense to "improve" the analysis memoization a bit more? I've made it cache per-type in DoctorVanGogh/Autofac@6046e61.
Should this be extended for the added helper methods in ReflectionExtensions.cs?
Should that reuse the ReflectionCacheSet.Shared.Internal.HasRequiredMemberAttribute cache? That would then be a dual store with Type and Field|PropertyInfo keys where Type entries mean "it has any required member" and for others it means "this property/field is required".

🤔

Footnotes

1. From https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/required ↩

[alistairjevans]

If you decompile a binary with required properties, you will find that the types do carry the attribute, indicating that at least one property in the type is required. See spec (https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-11.0/required-members#metadata-representation).

We use that as an optimisation to branch down the more expensive code path for the type only if necessary, so I'd leave that in please.

With that knowledge I suspect the memoisation holds as-is?

[DoctorVanGogh]

Ah, hadn't actually checked the types themselves with a decompiler, only the properties. That check makes sense then.

As far as I'm concerned, this PR is good to go as-is then.

The memoization still could be tweaked to not just store composite results but cache intermediate analysis as well, but if you're doing that, you could also cache per-property/constructor analysis too and just stick it all into the helper methods.
But I think that would warrant its own PR then.

[tillig]

I'd prefer to not list the names of other packages in our docs. I get that it's internal/private, but it may appear that we're "endorsing" the package, where I don't think we can really speak to whether this is the right answer for everyone.

[tillig]

Looks like this also appears below.

[DoctorVanGogh]

Sure, I'll strip this from the xmldocs. Was merely traing to retain the rationale for the non-reference checks.

Would sticking things inside a plain // ... code comment still be endorsing? Or should I just go with "for example imported via nuget or entirely self-supplied".

On that same note:
Two Test projects now have a PackageReference to that Required package (to get the required feature working in .NET6). Are those okay?

[tillig]

I'd probably just leave it generic with "... an internally defined custom polyfill attribute with the type name" and call it a day. We don't have to point to a specific implementation.

It may be worth actually putting our own polyfill in place in the test projects to show how you can do it without referencing a whole polyfill package, unless that's a lot of work. We don't have a lot of dependencies that aren't framework.

[DoctorVanGogh]

Okay, removed the mentions and added the custom polyfill in f5346a7

Had to muzzle stylecop slightly to retain the polyfill's original copyright notice & license. If you have a more "structured" process to include third party notices, feel free to tweak that yourself ;)

[tillig]

It would have been nice to maybe create these from scratch instead of copying them out of an existing library. Part of the point is to show folks how to do this without references and without extra licensing.

[DoctorVanGogh]

Well, considering the polyfill is purely functional and has to take that specific shape with those specific class names & preprocessor guards (okay, one could skip the nullable directives and warning toggles) and me knowing where I saw this, whatever I did would always be a "derivative" of a pre-existing version.
And if that's the case I might as well acknowledge where it came from... I'm not too fond of not-invented-here-syndrome, and considering the pre-existing polyfill is MIT licensed.... I'm even less tempted to pretend having done a clean room implementation ;)

[tillig]

@alistairjevans I'll leave this one for you. My thoughts:

• Don't keep the preprocessor directives if we don't need them.
• One class per file, even for the polyfills.
• Possibly make them public rather than internal since they're in the test project anyway.
• It may be arguably fundamentally different enough that it's unique but I'm not sure I care enough to argue it beyond that.

[DoctorVanGogh]

• Possibly make them public rather than internal since they're in the test project anyway.

I'd strongly discourage that. (At least if the polyfill is supposed to stand as an example for how to do this). Imagine the scenario of someone (re)using that example with public attributes. They build a netstandard2 assembly with it. That assembly then gets consumed in a NET7+ project. Suddenly you have duplicate attribute definitions and a lot of compiler barfing.

[alistairjevans]

I'm not fussed about making the shims public; they're defined by the .NET Compiler as internal, so if we're looking to create proper shims, we should do the same. See my other note about them being the 'formal' spec for the attribute.

I would like to be clear though that this polyfill in this test project is absolutely not an example of how to polyfill the required attributes. I'd really hope no-one ends up using an Autofac test project as an example of polyfilling in a distributed library.

[alistairjevans]

Nice! This looks clean, thanks for making the licensing changes.