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

[Proposal]: Compromise design for extensions #8519

Open
MadsTorgersen opened this issue Oct 17, 2024 · 85 comments
Open

[Proposal]: Compromise design for extensions #8519

MadsTorgersen opened this issue Oct 17, 2024 · 85 comments
Assignees

Comments

@MadsTorgersen
Copy link
Contributor

MadsTorgersen commented Oct 17, 2024

Compromise design for extensions

This has been moved to https://github.com/dotnet/csharplang/blob/main/meetings/working-groups/extensions/compromise-design-for-extensions.md.

@MadsTorgersen MadsTorgersen self-assigned this Oct 17, 2024
@TahirAhmadov
Copy link

// Core LINQ methods as compatible extension methods

So, we'll have old extension methods, new extension methods, and new extension types?
It's still entirely unclear why binary compatibility with old extension methods at the level of the extension feature itself is a goal, when a combination of a straightforward code fixer and source generator solves the problem very nicely.

As a compromise, this proposal may be less principled and conceptually clear to users.

That is putting it very, very mildly.

@CyrusNajmabadi
Copy link
Member

when a combination of a straightforward code fixer and source generator

No one has demonstrated it is straightforward.

@HaloFour
Copy link
Contributor

I kind of agree, I feel that this compromise design leaves things in a worse state. Now we'd have three syntaxes, with one intended to replace the other but only necessary in the more complicated scenarios. I wonder what percentage of those existing extension methods actually fall into that category.

I'm going to throw some half-thought-out spaghetti using constraint syntax at the wall:

public extension Enumerable for IEnumerable {
    // simple extension method
    public IEnumerable<T> OfType<T>() { ... }

    // static helper method
    public static IEnumerable<T> Empty<T>() { ... }

    // generic extension constraints
    public IEnumerable<T> Where<T>(Func<T, bool> predicate)
        where this : IEnumerable<T> { ... }

    public IOrderedEnumerable<TSource> ThenBy<TSource, TKey>(Func<TSource, TKey> selector)
        where this : IOrderedEnumerable<TSource> { ... }
}

Honestly not sure that it really differs from the compromise proposal, but it feels like it's trying to address it via specialization rather than two flavors of extension methods.

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 18, 2024

[ExtensionMethods("Enumerable")] // static class name
public extension EnumerableExtension<T> for IEnumerable<T> 
{
  public IEnumerable<T> Where(Func<T, bool> predicate) { ... }
  public IOrderedEnumerable<T> OrderBy<TKey>(Func<T, TKey> selector) { ... }
  [RenameTypeArgument("T", "TSource")] // if we want to preserve the old type argument name
  public IOrderedEnumerable<T> OrderBy<TKey>(Func<T, TKey> selector) { ... }
}

// generated
public static partial class Enumerable
{
  // insert extension generics in the beginning of method generics automatically
  public static IEnumerable<T> Where<T>(this IEnumerable<T> This, Func<T, bool> predicate)
    => ((EnumerableExtension<T>)This).Where(predicate); // and omit them when calling the new extension

  // insert before method's generics
  public static IOrderedEnumerable<T> OrderBy<T, TKey>(this IEnumerable<T> This, Func<T, TKey> selector)
    => ((EnumerableExtension<T>)This).OrderBy<TKey>(selector); // in this case, the generic of the extension type is removed, 
  // but additional generic parameters are kept

  // with the rename
  public static IOrderedEnumerable<TSource> OrderBy<TSource, TKey>(this IEnumerable<TSource> This, Func<TSource, TKey> selector)
    => ((EnumerableExtension<TSource>)This).OrderBy<TKey>(selector);
}

@HaloFour
Copy link
Contributor

@TahirAhmadov

ExtensionMethods("Enumerable")] // static class name
public extension EnumerableExtension<T> for IEnumerable<T> 

The problem is the generics, as I expect a lot of people would assume that would emit the class Enumerable<T>, not Enumerable.

I, personally, think that flattening the generics is probably the cleanest way to go, but I can understand this concern. However, supporting partial declarations with different extension targets might be one way to avoid the problem with heterogeneous targets, although that might become a real problem if extension implementation becomes a thing.

@TahirAhmadov
Copy link

The problem is the generics, as I expect a lot of people would assume that would emit the class Enumerable<T>, not Enumerable.

How can they possibly assume that a genetic class will be generated, if generic classes cannot house extension methods? We're talking novice/ school levels of knowledge. I doubt it's the same group who maintains LINQ and other libraries. 🤣

I'm not sure I understand the rest of your comment, so I'll refrain from responding in any way.

@HaloFour
Copy link
Contributor

@TahirAhmadov

How can they possibly assume that a genetic class will be generated, if generic classes cannot house extension methods?

Because the developer is writing what looks like a generic type, not a static class, so the limitations specific to static classes don't necessarily apply.

I'm not sure I understand the rest of your comment, so I'll refrain from responding in any way.

I'm agreeing that moving the generic type argument from the extension to the method, as you suggested, is probably the cleanest/easiest to emit the extension types to remain binary compatible. Additionally, if you can write multiple partial extensions, you could theoretically have a different extension target for each:

public partial extension Enumerable<T> for IEnumerable<T> {
    public IEnumerable<T> Where(Func<T, bool> predicate) { ... }
}

public partial extension Enumerable<T> for IOrderedEnumerable<T> {
    public IOrderedEnumerable<T> ThenBy<TKey>(Func<T, TKey> selector) { ... }
}

// emits
public partial static class Enumerable {
    public static IEnumerable<T> Where<T>(this IEnumerable<T> source, Func<T, bool> predicate) { ... }
    public static IOrderedEnumerable<T> ThenBy<T, TKey>(this IOrderedEnumerable<T> source, Func<T, TKey> selector) { ... }
}

@TahirAhmadov
Copy link

Because the developer is writing what looks like a generic type, not a static class, so the limitations specific to static classes don't necessarily apply.

The limitations apply because the attribute and the SG it triggers are clearly and unequivocally for generating "old extension methods", and those have to be in a non-generic class. Just a little comprehension of the context quickly clarifies the intent. Besides, they can always inspect the generated code, and existing test suites would either fail to compile or fail to execute if there were any issues.

I'm agreeing that moving the generic type argument from the extension to the method, as you suggested, is probably the cleanest/easiest to emit the extension types to remain binary compatible. Additionally, if you can write multiple partial extensions, you could theoretically have a different extension target for each:

If I understand correctly, you mean the language should do it as part of the feature? There are a few major issues with this.

  1. It leaves very little room for the language to be flexible about extension implementation/emit strategy - we now establish a promise of how things are emitted.
  2. It's a lot more rigid and less customizable than a source generator; if some library out there follows a convention different than what most libraries follow, that library is out of luck - unless they can convince the team to make a language change to allow their scenario, and that is not a workable approach.
  3. Designing the extensions with the goal of binary back-compat takes away from the goal of making the best possible extensions feature. It would be one thing if the best extensions feature made binary compat impossible, which would justify having this "dual mandate". However, like I demonstrated above, that's just not the case - even if we went with the extensions feature as originally designed (without static types), binary compat is still trivial.
  4. In addition to the competing priorities issue in item 3 above, it just takes more time to create these proposals - as we all know, making language features is very complicated and time consuming - plain economics tell us that tools like code fixers and SGs are cheaper.

Really though, I don't even think an SG is necessary. If we adopt a very clear direction of "all extension stuff going forward will be done using new extension types", then the only extension methods we need to worry about are the ones currently existing in the ecosystem; there is no need to create new old-style extension methods after extension types ship. Consumption of new extensions can only happen in new code, which means binary compat is a non-issue. And for existing old extension methods, the code fixer can handle the "transition" with the "convert-and-retain-a-redirect" feature.

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 18, 2024

Basically, we already have:

public static class Enumerable
{
  public static IEnumerable<T> Where<T>(this IEnumerable<T> This, Func<T, bool> predicate) { ... }
  // ...
}

After code fixer:

public extension EnumerableExtension<T> for IEnumerable<T> 
{
  public IEnumerable<T> Where(Func<T, bool> predicate) { ... }
  // ...
}
public static class Enumerable
{
  public static IEnumerable<T> Where<T>(this IEnumerable<T> This, Func<T, bool> predicate) 
    => ((EnumerableExtension<T>)This).Where(predicate);
  // ...
}

No extension feature back compat necessary, no SG necessary. Just the original design and a smart code fixer.

@HaloFour
Copy link
Contributor

If I understand correctly, you mean the language should do it as part of the feature?

Yes, I strongly believe that we're far from the point of punting on this and evoking source generators. Extensions are a core language feature, and as long as this new thing is also called "extensions" it seems obvious that the language should strive to bridge the gap between them and provide a built-in mechanism to safely evolve them.

C# is a 22 year old language, and extensions are a 17 year old language feature. We can't just throw out that enormous amount of existing code or relegate it to the bin of deprecation, regardless of how "clean" a potential replacement could be.

@TahirAhmadov
Copy link

Frankly, the stuff that I wrote above is so obvious to me, that I suspect all of it is known to LDT and there are reasons that escape me why they're going down the road they are going.
If so, it's unfortunate that these reasons are not communicated clearly. At this point, I would really like to hear a good compelling reason for all this from them.

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 18, 2024

BTW: https://github.com/microsoft/referencesource/blob/master/System.Core/System/Linq/Enumerable.cs
Image

They're not using if (source is Iterator<TSource> iterator) return iterator.Where(predicate); syntax for some reason, and the curly braces are formatted Java style, not C# style. No bueno 🤣

@alrz
Copy link
Member

alrz commented Oct 18, 2024

@HaloFour

However, supporting partial declarations with different extension targets might be one way to avoid the problem with heterogeneous targets, although that might become a real problem if extension implementation becomes a thing.

Curious why that is? I assume interfaces implemented on a partial declaration would be only applied to that target.

IMO the name should be only there for disambiguation, which probably require some new syntax to be used with operators/properties as well as conversion to an interface. However that turned out to be, you can still disambiguate by name since each extension can only implement an interface once on each target.

On top of that, I wouldn't find it surprising if the language emit Enumerable for extension Enumerable<T> and allow Enumerable.Where as a fully qualified member access the way it's used today.

@HaloFour
Copy link
Contributor

HaloFour commented Oct 18, 2024

@alrz

Curious why that is? I assume interfaces implemented on a partial declaration would be only applied to that target.

Well, it at least becomes a conversation. I agree, it doesn't preclude extension implementation, and there are options for what the language could do to support it with a set of heterogeneous partial extensions, but that depends on whether the team is up for that.

On top of that, I wouldn't find it surprising if the language emit Enumerable for extension Enumerable<T>

I could see it slightly surprising/confusing, but I agree that most people probably wouldn't even notice. I almost wonder if there's a better way to describe that the extension target is generic without having to declare the extension itself as generic, as a way to better clarify it to the developer.

@alrz
Copy link
Member

alrz commented Oct 18, 2024

I almost wonder if there's a better way to describe that the extension target is generic without having to declare the extension itself as generic

I think that's somehow essential with this because it would mean that type parameters are strictly not part of the type definition, allowing partial extension E<T> for M<T>; partial extension E<T, U> for M<T, U>;. That's why I think the name could be completely optional (that is, type parameters are independent of the name and vice versa).

@HaloFour
Copy link
Contributor

HaloFour commented Oct 18, 2024

More spaghetti at the wall, but maybe just exclude them? Treat the extension target as part of the signature and capable of introducing generic type parameters? The scoping would work.

public extension Enumerable for IEnumerable<T> where T : IAdditionOperators<T, T, T> {
    public T Sum { get { ... } } // shouldn't be a property, just for illustrative purposes
}

// emitted as:
public static class Enumerable {
    public static T get_Sum<T>(IEnumerable<T> @this) { ... }
}

Or allow extensions to declare additional generic type parameters:

public extension Enumerable<TOther, TResult> for IEnumerable<T> where T : IAdditionOperators<T, TOther, TResult> { 
    public TResult Sum { get { ... } }
}

// emitted as:
public static class Enumerable<TOther, TResult> {
    public static TResult get_Sum<T>(IEnumerable<T> @this) { ... }
}

// or, if flattened:
public static class Enumerable {
    public static TResult get_Sum<T, TOther, TResult>(IEnumerable<T> @this) { ... } // pick an order, any order
}

@CyrusNajmabadi
Copy link
Member

[ExtensionMethods("Enumerable")] // static class name
public extension EnumerableExtension<T> for IEnumerable<T> 
{
  public IEnumerable<T> Where(Func<T, bool> predicate) { ... }
  public IOrderedEnumerable<T> OrderBy<TKey>(Func<T, TKey> selector) { ... }
  [RenameTypeArgument("T", "TSource")] // if we want to preserve the old type argument name
  public IOrderedEnumerable<T> OrderBy<TKey>(Func<T, TKey> selector) { ... }
}

// generated
public static partial class Enumerable
{
  // insert extension generics in the beginning of method generics automatically
  public static IEnumerable<T> Where<T>(this IEnumerable<T> This, Func<T, bool> predicate)
    => ((EnumerableExtension<T>)This).Where(predicate); // and omit them when calling the new extension

  // insert before method's generics
  public static IOrderedEnumerable<T> OrderBy<T, TKey>(this IEnumerable<T> This, Func<T, TKey> selector)
    => ((EnumerableExtension<T>)This).OrderBy<TKey>(selector); // in this case, the generic of the extension type is removed, 
  // but additional generic parameters are kept

  // with the rename
  public static IOrderedEnumerable<TSource> OrderBy<TSource, TKey>(this IEnumerable<TSource> This, Func<TSource, TKey> selector)
    => ((EnumerableExtension<TSource>)This).OrderBy<TKey>(selector);
}

How is that guaranteeing the exact same semantics? You've done a translation. But the point is that it isn't necessarily the case that all forms are translatable between the two.

@CyrusNajmabadi
Copy link
Member

and existing test suites would either fail to compile or fail to execute if there were any issues.

People are not necessarily testing every calling permutation with generics. Things may continue to compile (but with different semantics). And the cases that don't compile may be missing tests.

@TahirAhmadov
Copy link

How is that guaranteeing the exact same semantics?

What do you mean by the "same semantics"? If you can provide an example of differing semantics, it would be helpful for us to understand.

But the point is that it isn't necessarily the case that all forms are translatable between the two.

What is an example of something which cannot be translated? And how is it impossible to translate with an SG, but possible to do so with a language emit strategy?

@CyrusNajmabadi
Copy link
Member

Meaning that existing callers of the library will continue to can the same abi as before, and the impl of that ABI will have the exact same semantics it has before.

If you can provide an example of differing semantics

It's the reverse. You have to prove the semantics are identical.

That's the point. To claim this is a solution, your claim must come with a string proof that it actually does perfectly preserve semantics.

but possible to do so with a language emit strategy?

All the language emit strategies that give perfect compat do not do things like break type parameter lists into separate lists. We can "prove" compat because we can show exactly how every extension form today maps to the new syntax with the same semantics.

Your approach says that extensions will work by having the user code so normal lookup, which calls into a stub, which then does a different lookup, which then calls into a method written differently with no guarantees on the last two steps that it follows the logic of the original code.

@CyrusNajmabadi
Copy link
Member

Frankly, the stuff that I wrote above is so obvious to me, that I suspect all of it is known to LDT and there are reasons that escape me why they're going down the road they are going.

You keep ignoring the continued posts being made about problems, and you keep insisting your perspective is trivially correct (even though you can't guarantee compat, and it depends on people examining their metadata AND IL to ensure semantics are the same AND having a full test suite around all these issues to catch any problems).

This alone is a deal breaker. Libraries are my going to adopt this if they have to crack open their newly built code and verify nothing broke. They're not going to adopt if we tell them "semantics may have changed, hopefully you tested thoroughly enough to discover where that happened". They simply will not update the code because of that very real risk.

Extension methods are nearly 20 years old. They're one of our most successful features, with usage everywhere. Telling people: "here's a new, not necessarily compatible, way of doing them" just means people not moving since compat is so important.

Think of it like NRT where we took great pains to make it possible for people to move without any danger of breaking things at runtime.

@TahirAhmadov
Copy link

Meaning that existing callers of the library will continue to can the same abi as before,

Yes, that is satisfied - my solution is to literally leave existing extension methods stubs in place as they are.

and the impl of that ABI will have the exact same semantics it has before.

How can the language guarantee that? What if the library starts using new extension feature, and elects to change the behavior of one of the methods? Is that now the language's fault?

It's the reverse. You have to prove the semantics are identical.

I'm not sure what I need to do to prove it beyond what I already wrote. Would you like me to write out the contents of the Where, Select, etc. methods - the entire Enumerable class - in this thread and how they will look before and after? Or should I write the code fixer which accomplishes the task before the feature is even available?

All the language emit strategies that give perfect compat do not do things like break type parameter lists into separate lists. We can "prove" compat because we can show exactly how every extension form today maps to the new syntax with the same semantics.

I don't really see a difference here. If the user writes extensions in the new form, and the end result is a DLL which is binary compatible with old extension methods, what does it matter how it was accomplished, with an SG or with an emit strategy?
Besides, like I wrote already, I don't even think we need an SG here - the library authors (including BCL) can just leave existing extension methods as is with the bodies being redirects to the extension types' methods.

Your approach says that extensions will work by having the user code so normal lookup, which calls into a stub, which then does a different lookup, which then calls into a method written differently with no guarantees on the last two steps that it follows the logic of the original code.

Again, how can the language guarantee that? The developers of that library are free to change anything and everything between the versions of their library.
My idea is that the code fixer literally moves the code of the old extension methods as is - just replacing the This parameter from this Type This, with this, along with other necessary syntactic changes for the extension feature. Why do you insist that I want to change the logic of the code?

@CyrusNajmabadi
Copy link
Member

How can the language guarantee that?

By literally having the exact same language rules for both and stating a 1:1 translation between every feature of one to the other.

@CyrusNajmabadi
Copy link
Member

to prove it beyond what I already wrote

You would have to demonstrate that the new system has the exact same language and emit semantics.

So that code translated over with have the same meaning.

@CyrusNajmabadi
Copy link
Member

can just leave existing extension methods as is with the bodies being redirects to the extension types' methods.

Again, there is no guarantee you're giving that the new methods have the same semantics as the old ones.

The bcl will not change them if it risks then breaking someone who took a dependency on behavior that subtly changed with the new extension semantics.

@CyrusNajmabadi
Copy link
Member

the code of the old extension methods as is - just replacing the This parameter from this Type This, with this

That may change semantics. For example, refness of variables may change. In an extension value type, 'this' is by-ref, which means mutating methods now mutate the original receiver, which the original extension method would not have done (since it got a copy).

This is what I mean about perfectly preserving semantics. It is non-trivial, and you have to prove this for every part of the language in use in the extension method.

@CyrusNajmabadi
Copy link
Member

Or should I write the code fixer which accomplishes the task before the feature is even available?

That doesn't prove you preserved semantics. I can write a generator that doesn't, I could write a generator that seems to, but fails on subtle details. How would you be able to tell and actually prove it was correct?

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 18, 2024

Here is my proposal: (only one method example, for brevity)

public static partial class Enumerable
{
        public static IEnumerable<TSource> Where<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate) {
            if (source == null) throw Error.ArgumentNull("source");
            if (predicate == null) throw Error.ArgumentNull("predicate");
            if (source is Iterator<TSource>) return ((Iterator<TSource>)source).Where(predicate);
            if (source is TSource[]) return new WhereArrayIterator<TSource>((TSource[])source, predicate);
            if (source is List<TSource>) return new WhereListIterator<TSource>((List<TSource>)source, predicate);
            return new WhereEnumerableIterator<TSource>(source, predicate);
        }
        class WhereEnumerableIterator<TSource> : Iterator<TSource>
        {
        }
}

After code fixer:

public extension IEnumerableExtension<TSource> for IEnumerable<TSource> 
{
  public IEnumerable<TSource> Where(Func<TSource, bool> predicate) {
            if (this == null) throw Error.ArgumentNull("this");
            if (predicate == null) throw Error.ArgumentNull("predicate");
            if (this is Iterator<TSource>) return ((Iterator<TSource>)this).Where(predicate);
            if (this is TSource[]) return new WhereArrayIterator<TSource>((TSource[])this, predicate);
            if (this is List<TSource>) return new WhereListIterator<TSource>((List<TSource>)this, predicate);
            return new WhereEnumerableIterator<TSource>(this, predicate);
        }

        // move all internal stuff here; the generics can be simplified
        class WhereEnumerableIterator : Iterator
        { 
        // ......
        }
}
public static partial class Enumerable
{
  public static IEnumerable<TSource> Where<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate) 
    => ((IEnumerableExtension<TSource>)source).Where(predicate);
}

What can possibly go wrong here?
PS. Apologies for missing the source -> this refactoring when I first posted this, I'm on another call at the same time.

@TahirAhmadov
Copy link

That may change semantics. For example, refness of variables may change. In an extension value type, 'this' is by-ref, which means mutating methods now mutate the original receiver, which the original extension method would not have done (since it got a copy).

And that is exactly why we need to do something like a code fixer, so that the result can be inspected before they ship the new version.

What you are saying is, we the language team will expend months on top of months of effort to seek out and write the perfect emit strategy for all extension methods out there written over ~20 years. And library authors have to trust you blindly that you got everything just right.

And that is the opposite of the correct approach.

Instead, with a code fixer, these minor edge cases will quickly come to light, the code fixer can be patched, and users get what they need quicker and more reliably.

@CyrusNajmabadi
Copy link
Member

To clarify, I meant that they do 95% of the conversion using the fixer (which can handle the common scenarios),

Again, how does the fixer handle the common cases. You keep saying it can. I keep saying that as a subject matter expert in analyzers and generators i literally do not know how to accomplish this. And that i don't even know how i would ensure the translations have the exact semantics in even the simple cases.

We do not have hte tools for this, and the user has very little at their disposal to guarantee correctness either. Examination of the result is not sufficient. And the only hope they have is that literally code they have fails to compile, or causes a test failure. That's not a palatable answer for virtually all our partners.

@CyrusNajmabadi
Copy link
Member

True, but generic specialization is worthwhile on its own.

You are shifting goalposts. We've gone from: this is so simple and easy, and you can easily write a fixer to support people moving, to: this is only possible if we pull in other features which we don't even know if they will ever be possible.

That's the point. The new extensions feature is a subset of capabilities of hte old one. You're claiming that that's not a problem, and that people being able to partially move over is sufficient. But that's an opinion on your part which is not universal. And it certainly doesn't reflect a 'best' outcome. It represents a local maxima for you for the things you care about. But there are other local maxima which excel in other aspects of design, even if they're not as 'elegant' as you would like. The point of hte discussion and design ideas is to explore all these maxima and the decide which of them to take.

Of course, if someone finds a maxima that is better on all dimensions, that woul.d be amazing. But so far that has evaded everyone on the team and everyone in the community :)

@TahirAhmadov
Copy link

How so? It's a legal classic extension method.

Yes but it relies on a "quirk" to accomplish something which there is a proposal to address across the board nicely.

Think about it like this: when we get generic specialization, and we also have "new extension methods", and somebody will be discussing this topic, and they will be like, "oh yes, we added those because we didn't have generic specialization in C# 13, but it was added in C# 15, which kind of means the 'new extension methods' are no longer needed".

@TahirAhmadov
Copy link

You are shifting goalposts. We've gone from: this is so simple and easy, and you can easily write a fixer to support people moving, to: this is only possible if we pull in other features which we don't even know if they will ever be possible.

No problem, I'm willing to concede that there are edge cases like the U : T scenario. I'm just here to find the "truth" (rather, best possible solution).
If we absolutely must deliver extensions which on day one support ALL of the old extension methods, like 100% (and not 99% like I meant), then we should postpone extensions and tackle generic specialization first. (Again, I expect people to hate me for saying that. 🤣 )

@CyrusNajmabadi
Copy link
Member

Yes but it relies on a "quirk"

It relies on no quirk. Classic extension methods are methods. They can and do use every capability of normal methods.

Again, this is why i keep asking you for a proof that your preferred approach is 'best' and that all existing extensions can move over. That involves demonstrating, for example, that every part of type infererence, conversions, best common type, etc. all work the same way in teh new system.

Compiler devs tried to do this and even with full access to the spec and impl:

  1. they could not.
  2. they kept discovering it very much was not the same.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Oct 18, 2024

If we absolutely must deliver extensions which on day one support ALL of the old extension methods, like 100% (and not 99% like I meant), then we should postpone extensions and tackle generic specialization first

But why? We have trivial ways to support all existing extension methods. Why start with a take-back. Instead of making 'new extensions' just fully subsume classic extension methods?

Who benefits from that? TO me, it's much better to tell someone: hey... you know extension emthods, the thing you've used for 20 years? Now you can do everything you're already doing with them and you can have things like static-extensions, and extension properties. And it's all with a new consistent form that keeps things grouped just like you would expect. You can also completely safely move forward with any risk of compat at any level. All the capabilities you have always had are still there, and still feel natural and first class.

You are asking me to give that up. And i'm lacking even one case where it feels warranted.

Again, if you're asking people to give up compat, there better be a large number of insanely compelling use cases. I'm still waiting for those.

Again, I expect people to hate me for saying that. 🤣

I don't hate you for this. I just don't understand it. You seem to be making life more difficult for the ecosystem for... what?

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 18, 2024

OK I think there are multiple issues at play here. Let me start with one example:

    public void MakeNotNull for [NotNull] ref string? => this ??= "";

Why is this needed? Currently, this ref is only for value types in extension methods. What scenario is the above aiming to preserve in the new extensions?

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Oct 18, 2024

It would not be needed. We only need ref support for value types. That example was simply showing that to preserve compat, changing refness would be supported. Not that it had to support this we didn't support before.

As mentioned:

This proposal is for declaration syntax and its meaning.

It is showing how the syntax could look for specifying ref-ness. Not that ref-ness just be legal for reference types.

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 19, 2024

Another aspect is the nullability stuff. I understand old extension methods support receiving both Class? and Struct?, but I think we should reconsider this for new extensions.
Imagine this:

interface IFoo
{
  void Baz();
}
extension StringNullableExt for string? : IFoo
{
  public int LettersCount => this != null ? this.Where(char.IsLetter).Count() : 0;
  public int DigitsCount => this != null ? this.Where(char.IsDigit).Count() : 0;
  public void Baz()
  {
    // originally this worked
    for(int i = 0; i < this.LettersCount; ++i) Console.Write("letter, ");
    // then System.String added LettersCount property, and now above throws NRE
  }
}

void Bar(IFoo foo) { }
void Bar<T>(T foo) where T : IFoo { }

string? s = null;
Bar(s); // what happens in this case?
// is foo == null, or is it boxed StringNullableExtStructWrapper with _value == null?

And just in general:

string? s = null;
int length = s.Length; // NRT warning, NRE at runtime
int digitsCount = s.DigitsCount; // this has no NRT warning and doesn't throw NRE at runtime

When I read the above code, it makes no sense to me - why can I access one property, and not the other? Oh, wait, it's an extension on a nullable string....

When I originally said "straightforward" and "all old extension methods", I admit I was thinking more from my experience and I never do things like above.
I think we should reconsider the "all old extension methods" goal. I think it's a detriment to the extensions feature to allow the scenario above, and those nullable scenarios shouldn't be carried over from old extension methods. Those scenarios should be handled with static helper methods, not extensions.

I'll write about the refness stuff later, need to think about it some more.

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 19, 2024

Another question I have is, (and I'm not necessarily arguing one way or another here) are we going with static class approach for extensions as an implementation detail, or as part of the spec? And if it's the former, are there plans to re-write it to struct wrapper types later?

I think it's an important question to answer now, lest we draw ourselves into some kind of corner.

PS. Also, we need to know this so that we can make decisions about the ref stuff. From the OP, it sounds like we're going with static classes as spec forever.
That's possible, in fact in the beginning I thought we should just do that and call it a day.
But it contradicts what I read previously (meeting notes, I believe, or maybe another issue) that static class is a stop gap to ship extensions without waiting for N years for the runtime improvement.
And it does raise some new questions other than ref.
This confusion is making it difficult to properly assess the OP proposal.

@CyrusNajmabadi
Copy link
Member

I think it's a detriment to the extensions feature to allow the scenario above,

I don't see why. It was something we considered and explicitly rejected when doing extension methods originally. It was not accidental, but was very intentional in the design. .

@CyrusNajmabadi
Copy link
Member

why can I access one property, and not the other?

The same holds for methods. Why not properties?

What if I want something like DefaultCount which returns 0 on something null?

@CyrusNajmabadi
Copy link
Member

And if it's the former, are there plans to re-write it to struct wrapper types later?

Unknown. But act as if it might. And compat will continue being required.

@CyrusNajmabadi
Copy link
Member

I think it's a detriment to the extensions feature to allow the scenario above

I'm still missing what makes the type-based approach better given all the limitations and breaks that keep adding up.

Instead of starting at -100, these issues are taking it to -1000. So it better be amazing to warrant it. But I'm missing what is actually getting better (let alone AMAZINGLY better to warrant all the takebacks and problems this causes.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Oct 19, 2024

btw, a core issue I have is you describing things as quirks, or edge cases. They're not. Extension methods are normal methods, with just a tiny set of restrictions. All the unrestricted stuff is a normal part of it, and something we've definitely seen people do since these have been around 20 years.

Telling people for 20 years they could do that things and now taking it away just because you don't like them is not ok. Takebacks can be ok. But generally only when they are paired with some fantastic things to make that ok.

Right now, the type approach is lacking that. It seems strictly less capable, and it puts people in the unpalatable position of having to bifurcate their codebase for safety.

The member approach lets people move forward with complete safety, supports all the scenarios from before, and adds support for the cases people are asking for.

Why go with a proposal with all the drawbacks?

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 19, 2024

The same holds for methods. Why not properties?

It shouldn't be allowed for methods, either, and existing extension methods like that should be refactored (perhaps with a built-in analyzer and code fixer) to static helper methods.

What if I want something like DefaultCount which returns 0 on something null?

Why not do ?.Count ?? 0 like everywhere else? Now we have two ways to achieve the same thing. Confusion and no real benefit.

Unknown. But act as if it might. And compat will continue being required.

It has to be known, though. For example, the ref questions are intertwined with emit strategy; if we ever transition to a struct wrapper type, the ref stuff goes out the window; in fact, we won't be able to do it at all depending on what we do with ref now.

I'm still missing what makes the type-based approach better given all the limitations and breaks that keep adding up.

To clarify, regarding the type-based approach, I'm not arguing either way about the emit strategy. Regarding emit strategy, I'm just asking for clarity of intention - not because I need it for my personal gain, but in order to be able to design the feature correctly.
Regarding type based for syntax purposes, the objective I'm pursuing is simplicity and elegance, which I think are important considerations. Especially given that the reasons for adding new extension members are questionable (at least some of them).

@HaloFour
Copy link
Contributor

the objective I'm pursuing is simplicity and elegance

Which are entirely subjective.

@TahirAhmadov
Copy link

the objective I'm pursuing is simplicity and elegance

Which are entirely subjective.

To an extent in other situations, but in this case, we have significant effort going into new syntactic constructs, major emit decisions, consequences for future language development - it is very objective in this case.

@CyrusNajmabadi
Copy link
Member

Now we have two ways to achieve the same thing.

The irony :-)

the objective I'm pursuing is simplicity and elegance

I don't find your approach simple or elegant. As a consumer it would be very complex and inelegant. I'd have to keep extensions around for safety. And the only way to try to migrate would be to hang up to examine IL and hope my test suite missed nothing. Including the language now redirecting my callers to different extensions.

@HaloFour
Copy link
Contributor

it is very objective in this case.

No, that's still definitely subjective. As is weighing the value of supporting a safe migration strategy for the existing feature set compared to potential future directions which aren't even considered yet. Language design is compromise. It doesn't matter how simple and elegant a solution seems to be if people won't adopt it.

@CyrusNajmabadi
Copy link
Member

To an extent in other situations, but in this case, we have significant effort going into new syntactic constructs, major emit decisions, consequences for future language development - it is very objective in this case.

This applies to both directions.

@CyrusNajmabadi
Copy link
Member

I think it's telling that I keep asking for the compelling scenarios that are so worthwhile that it's ok to give up compat, and that still hasn't been answered.

Now the argument has shifted to it being a good thing to just break what extensions can do because someone doesn't like the capabilities of existing extensions. That's a non starter for me. Extension methods are huge. They are broadly produced and consumed. We would only consider breaks for things we were completely sure were mistakes that we'd been living with for decades. And even THEN we'd be cautious. You're suggesting breaking semantics that have been a normal, fine, intentional, part of the design since the beginning. And, again, without compelling value to make up for that.

@CyrusNajmabadi
Copy link
Member

we have significant effort going into new syntactic constructs,

So does the type approach.

major emit decisions,

Same for both. Why is the type approach objectively better?

consequences for future language development

Same for both. Why is the type approach objectively better?

I personally feel like it is worse for future language development as it's too dissimilar for how library authors actually want to model things. So we would be going down a path with a higher chance of future problems.

@iam3yal
Copy link
Contributor

iam3yal commented Oct 19, 2024

I'm not sure whether I'm deviating here, I haven't been following through all the discussions and notes so excuse my ignorance but was the following syntax (using PC) suggested before? probably was but putting it here just in case it wasn't. :)

public extension StringsExtensions(this string self)
{
}

as opposed to the following:

public extension StringsExtensions for string
{
}

@HaloFour
Copy link
Contributor

@iam3yal

I posted something like that to the LDM notes discussion:

See: #8521 (comment)

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 20, 2024

I think it's telling that I keep asking for the compelling scenarios that are so worthwhile that it's ok to give up compat, and that still hasn't been answered.

I'm building to that. For me to state that the proposal is not ideal, I need to explain either how to do things differently, or why things don't need to be done.

Again, leaving emit aside for a moment, the "extending a nullable" scenario should not be supported, IMO.

Which means one of the reasons for the "compromise" of "new extension members" goes away.

We would only consider breaks for things we were completely sure were mistakes that we'd been living with for decades. And even THEN we'd be cautious. You're suggesting breaking semantics that have been a normal, fine, intentional, part of the design since the beginning.

Huh? I'm not saying we should break anything. Old extension methods stay around (and will likely stay around for a very long time). I'm not proposing to deprecate them with warnings or anything like that. (At most, we can give an analyzer, or maybe just guidance - a "best practice" line item that doing stuff like that is an anti-pattern.)
We're discussing the new feature, extensions. My point is that, this new feature does not need to support the nullable receiver scenario, because it would be a net negative.
Like everybody says, we start at -100 points, for nullable receivers. Here, the 1) confusion of the resulting code (accessing members on a null), 2) questionable semantics WRT interface implementation, both take nullable receivers to -300. I understand the argument of, let's allow extensions to replace old extension methods which support nullable receivers, fine, now we are at -200. In the end, we're still way below 0.

PS. Going back to emit strategy, I still haven't heard a definite answer to the question, particularly regarding ref. If we allow all of the ref modifiers on the receiver (to be able to completely replace old extension methods), how are those going to be emitted with the struct wrapper approach if we ever decide to switch to that?
If we are never going to switch to struct wrappers, fine - we can continue the discussion with that assumption. We will need to figure out how everything will play together in the future, but we can't discuss neither the ref questions nor the future interface implementation questions without knowing the answer to this question.

@HaloFour
Copy link
Contributor

My point is that, this new feature does not need to support the nullable receiver scenario, because it would be a net negative.

That's your opinion. It seems that the language team does not share this opinion with you. I certainly don't either. You're making the assumption that this is considered a mistake of the design of extension methods, one that needs to be carried over to whatever this new feature is only because of legacy reasons, and that is not a correct assessment.

Like everybody says, we start at -100 points, for nullable receivers.

It's up to the team to assess how each aspect of a feature impacts the cost. It's not a blanket -100. Supporting nullable receivers is cheaper than not as the emit strategies are known and it's less work to enforce. It's something the runtime has always supported out of the box and the C# does extra work to prevent it. Struct wrappers would have to do the same additional work.

@TahirAhmadov
Copy link

TahirAhmadov commented Oct 20, 2024

You're making the assumption that this is considered a mistake of the design of extension methods, one that needs to be carried over to whatever this new feature is only because of legacy reasons, and that is not a correct assessment.

Care to explain why? Why is it so necessary to be able to write ((string?)null).LettersCount instead of ((string?)null)?.LettersCount ?? 0? Why introduce this inconsistency with regular instance members?

Supporting nullable receivers is cheaper than not as the emit strategies are known and it's less work to enforce.

It's not cheaper, though, because in order to support them, all of the syntax work needs to be done as proposed in the OP. The original syntax proposal for extensions was a very close match to regular type syntax, which was its strength. Now we're saying, we'll keep that, PLUS add all of these compromise solutions because we absolutely MUST replace ALL old extension methods. It's this last part I'm not seeing a good reason to do.

@HaloFour
Copy link
Contributor

HaloFour commented Oct 20, 2024

Care to explain why?

Because I find it convenient? It allows me to absorb the onus of the null check on helper methods in one place rather than force every consumer of that method to do so?

It's not cheaper, though, because in order to support them, all of the syntax work needs to be done as proposed in the OP.

The syntax is not relevant as to whether or not an extension will perform a null check on the receiver.

Now we're saying, we'll keep that, PLUS add all of these compromise solutions because we absolutely MUST replace ALL old extension methods. It's this last part I'm not seeing a good reason to do.

Also not necessarily true. Even if the language team decided to completely skip binary compatibility with existing extensions, that doesn't say anything about whether or not null receivers would be supported. I'd suggest that they would continue to be, not because of some concern over legacy migration, but because it's a genuinely useful feature. And frankly because it's cheaper and easier than forcing the compiler to have to emit the null check in every extension, always incurring that cost even in cases where null isn't a possible value, as the runtime will provide no help here.

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

No branches or pull requests

8 participants