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

chore: Add common shims for new language features #182

Closed

Conversation

austindrenski
Copy link
Member

Handful of commonly used shim types to un-gate new(-ish) language features in older TFMs. Limited to features that are only gated by symbol existence checks, but covers some fairly common features.

See: #264, #181


tldr; I kept reaching for these features while drafting #181, so figured I'd open a standalone PR to see if there are any blocking concerns so these don't just sneak in as part of #181.

If accepted here, will copy/pasta to the contrib repo as well.

FAQ

This seems sketchy? Is it safe?

It's not not sketchy, but you can find similar examples of these types of un-feature-gating shims around the .NET ecosystem (e.g. StackExchange/StackExchange.Redis#2621).

The key to playing with these bits is making sure you don't start reimplementing logic vs placing type definitions in scope for Roslyn to consume. That way these types disappear at compile-time and Roslyn's normal bits take their place.

What's the argument in favor of this? Is there added complexity?

Given that this project will likely want to maintain compat for netstandard2.0 and net462 until they EOL (far into the future), many new language features will remain gated just out-of-reach, so when/where possible, low-hanging shims can bring some fun to the otherwise dreary world of net462 and friends.

Those features are often times just sugar, but sometimes that sugar is useful (e.g. record syntax), and other times it's just more familiar and can lower the barrier to new contributors.

@austindrenski austindrenski requested a review from a team as a code owner January 16, 2024 23:22
@austindrenski austindrenski changed the title Add common shims for new language features chore: Add common shims for new language features Jan 16, 2024
@austindrenski austindrenski force-pushed the treats-and-toys branch 2 times, most recently from a0e63dc to da00f1c Compare January 16, 2024 23:26
@austindrenski austindrenski self-assigned this Jan 18, 2024
Handful of commonly used shim types to unlock new(-ish) language
features in older TFMs. Limited to features that are only gated by
symbol existence checks, but covers some fairly common features.

Signed-off-by: Austin Drenski <[email protected]>
@askpt askpt self-assigned this Apr 15, 2024
toddbaert
toddbaert previously approved these changes Apr 15, 2024
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Some of this is a bit beyond me, though the reasoning from @austindrenski makes sense.

@askpt I won't block this, but I don't feel confident enough to approve it.

@kinyoklion @benjiro how do you feel about this? Do you see risks here?

@kinyoklion
Copy link
Member

Some of this is a bit beyond me, though the reasoning from @austindrenski makes sense.

@askpt I won't block this, but I don't feel confident enough to approve it.

@kinyoklion @benjiro how do you feel about this? Do you see risks here?

I think that actually following the example provided backward through its journey would express the type of concern I have.
StackExchange/StackExchange.Redis#2621

Is a fix for a previously incorrectly shimmed item, which was originally reported on the dotnet runtime. So, by shimming that feature it created a multi-party chain of issues and corrections. Which maybe, based on guidance from the issues, we will not encounter.
dotnet/runtime#96197
StackExchange/StackExchange.Redis#2619

I think you get some ergonomic improvements in exchange for unexpected and confusing experiences when things go wrong.

I do think that the pattern in Check.cs is fine. Take the thing you want, put it in your own namespace, and then conditionally use the language feature or a local implementation.

Thank you,
Ryan

@askpt
Copy link
Member

askpt commented Apr 16, 2024

Some of this is a bit beyond me, though the reasoning from @austindrenski makes sense.
@askpt I won't block this, but I don't feel confident enough to approve it.
@kinyoklion @benjiro how do you feel about this? Do you see risks here?

I think that actually following the example provided backward through its journey would express the type of concern I have. StackExchange/StackExchange.Redis#2621

Is a fix for a previously incorrectly shimmed item, which was originally reported on the dotnet runtime. So, by shimming that feature it created a multi-party chain of issues and corrections. Which maybe, based on guidance from the issues, we will not encounter. dotnet/runtime#96197 StackExchange/StackExchange.Redis#2619

I think you get some ergonomic improvements in exchange for unexpected and confusing experiences when things go wrong.

I do think that the pattern in Check.cs is fine. Take the thing you want, put it in your own namespace, and then conditionally use the language feature or a local implementation.

Thank you, Ryan

@toddbaert and @kinyoklion: I will close this PR and add new shims as needed. I was not the original proponent for this PR, so I don't feel comfortable merging it if the team is also uncomfortable. I am just trying to unblock all the breaking changes at the moment.

@askpt askpt closed this Apr 16, 2024
@toddbaert
Copy link
Member

@toddbaert and @kinyoklion: I will close this PR and add new shims as needed. I was not the original proponent for this PR, so I don't feel comfortable merging it if the team is also uncomfortable. I am just trying to unblock all the breaking changes at the moment.

Thanks @askpt, much appreciated. 🙏

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

Successfully merging this pull request may close these issues.

4 participants