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

C# should support indexed property #2144

Closed
ygc369 opened this issue Apr 21, 2015 · 22 comments
Closed

C# should support indexed property #2144

ygc369 opened this issue Apr 21, 2015 · 22 comments

Comments

@ygc369
Copy link

ygc369 commented Apr 21, 2015

for example:
class A
{
int[] a, b;
public int pa[int index]
{
get { return a[index]; };
set { a[index]=value; };
};
public int pb[int index]
{
get { return b[index]; };
set { b[index]=value; };
};
};

@ygc369
Copy link
Author

ygc369 commented Oct 16, 2015

nobody is interested in this?

@jrmoreno1
Copy link
Contributor

Well, I like the idea, but it's been out there for a while and rejected because of "ambiguous syntax" (not sure what is ambiguous about it myself). I like the idea, because semantically an indexed property is not an item in a collection. In fact I found this while checking to make sure that it hadn't been added since the last time I thought it would make things easier. In this case I was hoping to build a facade around Session, where most of the items are not just per Session but per window/tab. Index properties would make that trivial: Session.ExtensionMethodReturnsFacade().Property[page].

@alrz
Copy link
Member

alrz commented Mar 5, 2016

If the the type has an indexer you would get that syntax,

class Foo { public int this[int index] { ... } }
class Bar { public Foo Foo { get; } }

var bar = new Bar();
int foo = bar.Foo[5];

And if you define Foo property as an "indexed property" it'll become ambiguous. I don't see why this is useful.

@jrmoreno1
Copy link
Contributor

@alrz and you see value in writing and instantiating a class in order to return an indexed value? The class Foo is never going to be used for anything other than the property Foo in Bar. As for the ambiguity -- I still don't see it, at least as far as the compiler is concerned. For people looking at the line in isolation, it wouldn't be clear whether it was an indexed property or a property for an indexer or Foo is just an array or dictionary, but I don't see how that matters. Any way you look at it, it is returning a value from something else. Looking at the definition will tell you what it is.

@HaloFour
Copy link

HaloFour commented Mar 6, 2016

From http://roslyn.codeplex.com/discussions/542484

Please explain why creating nested classes with separate indexers does not satisfy your requirement.

I don't believe this is a feature you're using very often, so extra typing isn't a very good reason why we should add a whole new feature.

If you are using it a lot, I'd like an actual example, because I believe multiple indexers is rarely appropriate.

And that nested type can be a struct, no allocations necessary.

@alrz
Copy link
Member

alrz commented Mar 6, 2016

As for the ambiguity -- I still don't see it

See,

class Foo { public Bar this[int index] { ... } }
class Bar { public Foo Foo[int index] { ... } }

var bar = new Bar();
var foo = bar.Foo[5]; // which indexer I'm calling?

For people looking at the line in isolation, it wouldn't be clear whether it was an indexed property or a property for an indexer or Foo is just an array or dictionary, but I don't see how that matters.

If it doesn't matter why don't you use a method instead? You are basically suggesting a method that has [] instead of (),

class Bar { public Foo Foo[int index] { ... } }
class Bar { public Foo Foo(int index) { ... } }

Why is this important then?

@alrz
Copy link
Member

alrz commented Mar 6, 2016

As for assignment, ref returns make it possible, but for more complicated scenarios you should seriously consider a method.

@jrmoreno1
Copy link
Contributor

@HaloFour I gave an example of creating a facade around Sesssion where a page id is used to avoid collissions between tabs. Most of the properties should be indexed. C#, unlike VB, requires that an indexed property be the default property of a class. C# is conflating two entirely separate concepts, and in doing so being unclear.

@alrz in VB, where that works, it calls the indexer in Bar (which returns a Foo), not the default property of a Foo (which returns a Bar). I don't see how it could be otherwise. As for why it matters, you hit upon it in your second message: assignment. A property handles both retrieving and setting a value. Also a method that returns a value is a calculation, while a property is state.

As for ref return, I know the CLR supports it, but c# doesn't, and even if it did, that's not quite the same thing as a property.

@HaloFour
Copy link

HaloFour commented Mar 6, 2016

So your facade exposes a bunch of "named indexable" properties each taking the same index and each returning a different value?

var x = obj.X[id];
var y = obj.Y[id];

Why would that be better than having one normal indexer which returns an object containing all of the properties for that page?

var x = obj[id].X;
var y = obj[id].Y;

Then you can pass all of the properties around without each consumer having to know/care about how they're indexed.

Your use case sounds like the very design that the language should be discouraging.

@jrmoreno1
Copy link
Contributor

First let me say to that I understand that everything starts at -100, and this isn't a pony let alone a unicorn. I'm not trying to say "do it" but rather, "it shouldn't be rejected". Anyway...

Sorta, not all of the properties will be indexed. Maybe half, pssoibly less.

And doing it as you suggests works, but it creates an artificial distinction between properties on the wrapper and the indexed properties (class page with some properties). Session.w().D seems quite different from Session.w()[id].Y, as opposed to Session.w().D and Session.w().Y[id].

And I don't see why Session.w()[id].Y should be considered better than Session.w().Y[id](other than it works). The later seems more natural to me. The first seems to be saying "get the page of wrapper with the value id and then get the Y property" and the second "get the property Y of wrapper and then get the id value". But page doesn't really exist, it is an additional abstraction that is necessary only in order to make the access actually work.

And since page doesn't model any real object and the properties aren't related in any way, the abstraction doesn't help you reason about the program.

Again, I'm not saying this is a must-have or the best way (let alone the only) way to solve any particular problem. Just that it makes sense and would work, and that the "it's ambiguous" argument is, uhm, weak at best. It provides feature parity with VB. Don't reject the idea of someone doing it.

@Corey-M
Copy link

Corey-M commented Apr 7, 2016

Feature parity aside, I believe that this is still a valid extension to C#. The fact that it is possible in VB.NET shows that it is possible and CLR-supported, so that's not an impediment. The issues raised here seem to revolve around the way that it is used in a particular example... which seems to be a frivolous objection, since there are certainly other use cases which don't necessarily suffer from the same perceived problems.

For instance, I have been working on an extendable wrapper for executing scripts and commands in an embedded PowerShell Runspace. Part of my code looks something like this:

public class Indexer<TKey, TValue> 
{
    private readonly Func<TKey, TValue> _getter;
    private readonly Action<TKey, TValue> _setter;
    public Indexer(Func<TKey, TValue> getter, Action<TKey, TValue> setter)
    {
        _getter = getter;
        _setter = setter;
    }
    public TValue this[TKey key] 
    {
        get { return _getter(key); }
        set { _setter(key, value); }
    }
}

public class PSRunner : IPSRunner
{
    private readonly Runspace _runspace;
    private readonly Indexer<string, object> _variables;
    public Indexer<string, object> Variables { get { return _variables; } }

    public PSRunner(Runspace runspace)
    {
        _runspace = runspace;
        if (_runspace.RunspaceStateInfo.State != RunspaceState.Opened)
            _runspace.Open();
        _variables = new Indexer<string, object>(
            name => _runspace.SessionStateProxy.GetVariable(name),
            (name, value) => _runspace.SessionStateProxy.SetVariable(name, value)
        );
    }
}

This allows me to completely abstract the session state of the PowerShell Runspace and treat the PowerShell variables collection almost as if it were a dictionary without allowing full dictionary semantics that would not work in this situation. All I need this to do (at the moment anyhow) is get and set variables by name, which is easily handled by the above code.

I have other collections that I would also like to be able to access this way, and will end up coding as above, but would much prefer to do this:

public class PSRunner : IPSRunner
{
    private readonly Runspace _runspace;
    public object Variables[string name]
    {
        get { return _runspace.GetVariable(name); }
        set { _runspace.SetVariable(name, value); }
    }

    public PSRunner(Runspace runspace)
    {
        _runspace = runspace;
        if (_runspace.RunspaceStateInfo.State != RunspaceState.Opened)
            _runspace.Open();
    }
}

Shorter code, much more self-explanatory, fewer issues with object initialization (no null assignment to the _variables field in a rogue constructor), etc.

Another issue with the first code block above is that lambda expressions do funny things to garbage collection of the objects they capture. I've had to implement the Indexer as an IDisposable to work around this and ensure that my object get disposed of correctly. I could use weak referencing to resolve the issue, but that way leads to madness and endless custom indexer classes.

And finally, there is not way to prevent or track external references to Indexer instances outside of the class. This could lead to a situation where the Variables index property could be referenced externally and either prevent the correct disposal or garbage collection of my PSRunner instance. A proper indexer property implementation should not allow this to happen.

@alrz
Copy link
Member

alrz commented Apr 7, 2016

@Corey-M I believe #6136 can address your use case, something like

extension Runspace {
  public object this[string key] {
    get { return GetVariable(key); }
    set { SetVariable(key, value); }
  }
}

@Corey-M
Copy link

Corey-M commented Apr 8, 2016

@alrz Nice thought, but in order to use an extension on an object I would need to expose that object, which I do not wish to do in this case. All interactions with the Runspace instance need to be mediated by my wrapper class(es). In order to attain proper isolation using extension properties I would have to create, instantiate and expose yet another wrapper object... at which point I am back where I started with an Indexer class with all of the same issues.

I suppoort #6136 in principle - I think it'd be a nice feature to have - but don't see a simple implementation path. Indexed properties (named indexers) already have been implemented in VB so there are no obvious impediments to C# implementation. This is a comparatively simple feature to add.

@alrz
Copy link
Member

alrz commented Apr 8, 2016

To hide implementation you'd better use interfaces (#8127).

@Corey-M
Copy link

Corey-M commented Apr 8, 2016

@alrz I'm sorry but your extensions do not fit my needs, and interfaces do not prevent any of the issues I have raised. A reference to an interface can still be cached external to the class instance, the interface still needs to be assigned a concrete object whose lifecycle requires management, code is rendered less readable and so on. All of these points (and more) would be resolved by indexed properties.

@HaloFour
Copy link

HaloFour commented Apr 8, 2016

I could create a delegate to your indexed property and hold onto it forever, thereby holding a root reference to your Runspace class and preventing it from ever being collected. There's nothing you can ever do to prevent anything like that.

Yes, it would be easy to add. It always has been. Yet the team intentionally didn't add it, and they continue to question their utility and discourage their pattern today. They never need a reason to not do something.

@Corey-M
Copy link

Corey-M commented Apr 9, 2016

@HaloFour In the current incarnation yes, you could hold all sorts of references, including a lot of unintentional ones. That's the problem.

But how would that work for an indexed property? What would it look like to do so? Referencing the property itself without an index should fail at compile time and with an index it is not going to be returning a delegate (unless that's what is stored in the backing collection). How exactly would you get that delegate reference? How would you get a delegate to a standard property get/set method for that matter? Reflection?

If bypassing via Reflection is reason not to have a feature then you might as well rip out private and protected modifiers since reflection makes them worthless.

Yes, it would be easy to add. It always has been. Yet the team intentionally didn't add it, and they continue to question their utility and discourage their pattern today. They never need a reason to not do something.

So what you're saying is I should never bother asking or showing support for features that might have come up before because The Team is all-wise, their decisions should never be questioned and no reasons need be given?

Wow. Just wow.

I thought that maybe if we could explain the utility of the feature, explain how we intend to use it and what benefits we would get from doing so, showed support for the feature and so on that maybe - just maybe - they might decide that the feature was actually worth taking another look at, and maybe one day I might get an improvement that will help me with this mess.

Is that not the point of forums like this?

@HaloFour
Copy link

HaloFour commented Apr 9, 2016

But how would that work for an indexed property?

You can create a delegate to any method, including property accessors. Simplest C# syntax would be through a closure:

Func<string, object> d = index => runner.Variables[index];

So what you're saying is I should never bother asking or showing support for features that might have come up before because The Team is all-wise, their decisions should never be questioned and no reasons need be given?

Repeating the same questions will generally just result in the same answers. I think it's fine to question those answers, just as I think it's fine to question the questioning of those answers. The Team ultimately owns their language, though.

@Corey-M
Copy link

Corey-M commented Apr 9, 2016

@HaloFour Your lambda doesn't extract a delegate for the property get method, it closes around the parent object (runner in the example). This is no different to holding any form of reference to the parent object, and you're right that there's nothing I can do to stop it.

What I can do though is track those references down in the the code, or in the IL if necessary. Adding my IIndexer interface and the Indexer class to the search increases the problem space and reduces the likelihood that I'll find the errant reference. Indexed properties would not do so.

The Team ultimately owns their language, though.

Have I implied otherwise? If so I apologize. But should I consider their dislike of the idea the final word and not try to show how it could be beneficial?

@alrz
Copy link
Member

alrz commented Apr 9, 2016

This is already backlogged so considering all the things that can be done for C# it is probably up for C# 11.

@HaloFour
Copy link

HaloFour commented Apr 9, 2016

@Corey-M You're right, C# offers no direct syntax to obtain a delegate for a property. However, there are several proposals to add that syntax, not to say that any of them will be implemented. But property accessor methods are just normal methods so some languages could offer that syntax, or you could use reflection to obtain a delegate directly:

PSRunner runner = new PSRunner();
PropertyInfo variablesProperty = typeof(PSRunner).getProperty("Variables");
MethodInfo getterMethod = variablesProperty.GetGetMethod();
Func<string, object> d = (Func<string, object>)Delegate.CreateDelegate(typeof(Func<string, object>), runner, getterMethod);

Have I implied otherwise? If so I apologize. But should I consider their dislike of the idea the final word and not try to show how it could be beneficial?

No, but you'd need a team member to champion the feature in order to get it implemented, even if you (or someone else) were to actually do the work. If they don't think that it's worth the cost in terms of effort and permanent support then it won't happen. In terms of showing how it could be beneficial, I'm not seeing anything new here.

@gafter
Copy link
Member

gafter commented Apr 21, 2017

Issue moved to dotnet/csharplang #471 via ZenHub

@gafter gafter closed this as completed Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants