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

Should it be possible to use getter-only auto-properties to explicitly implement an interface property? #1219

Open
gafter opened this issue Dec 27, 2017 · 6 comments

Comments

Projects
None yet
6 participants
@gafter
Copy link
Member

commented Dec 27, 2017

@ghost commented on Fri Jun 05 2015

Someone at Stack Overflow suggested I raise an issue here.

If I have an interface implementation like this:

public interface IFoo 
{
    string TestFoo { get; } 
}

And I wanted to implement it explicitly using the new C# 6 getter-only auto-properties:

public class Impl : IFoo
{
    // This was not possible in C# 5, but now works.
    string IFoo.TestFoo { get; }

    public Impl(string value)
    {
        // ERROR: Property or indexer 'IFoo.TestFoo' cannot be assigned to -- it is read only.
        ((IFoo)this).TestFoo = value;
    }
}

I was expecting this to work, since I am assigning a value in the constrcutor. It seems the compiler is not happy with the cast, though. The error can be seen as a comment in the code, but here it is again:

Property or indexer 'IFoo.TestFoo' cannot be assigned to -- it is read only.

Note that if the returned value does not change and can be assigned at initialization, the following code works fine (as suggested by Daniel White as an answer in the Stack Overflow question):

string IFoo.TestFoo { get; } = "Value to return";

Link to Stack Overflow question for reference.


@kush2207 commented on Fri Jun 05 2015

Is this really an issue? I'd expect it to error out. Given the fact property is getter-only property in the interface.

If you allow it to flow through, then this would be violation on the interface contract. I can understand contextually you are suggesting that it should work (since you are in the constructor).


@ghost commented on Fri Jun 05 2015

@kush2207 Since it IS a getter-only property, albeit an automatic one, I don't see this as a violation of contract. Using an auto-property with a private setter would be a violation of the contract.

The getter-only property can only be set in the constructor and it directly affects the readonly backing field (there is no hidden set method).


@kush2207 commented on Fri Jun 05 2015

"Since it IS a getter-only property, albeit an automatic one" - This is implementation detail. With the interface, you don't know if it will be implemented as automatic property or backed with some private member


@ghost commented on Fri Jun 05 2015

I don't know what you mean.

The interface expects a read-only property. It shouldn't matter if it's a getter-only auto property or i implemented the getter myself. That is an implementation detail.

The contract is that there is a getter for TestFoo, what else there is should not matter.


@ghost commented on Fri Jun 05 2015

Didn't mean to click "Close and Comment" there.


@kush2207 commented on Fri Jun 05 2015

I see your point, Looked at the code once more, sipped coffee and now it all makes sense :)


@whoisj commented on Sat Jun 06 2015

Didn't mean to click "Close and Comment" there.

I do that all the time, GitHub really could use a better UI.

I think this is a non-issue. The interface doesn't expose a setter. Therefore the interface should not support setting (i.e. don't cast to the interface). The fact that the explicit implementation allows setting is just a compat issue.


@AnthonyDGreen commented on Mon Jun 08 2015

I believe the example illustrates the problem with this. While it could be made possible to define a read-only auto prop that explicitly implements an interface unless it were initialized ... with an initializer it would be impossible to set it later as explicit interface members can never be referenced by name. Were we do enable this it would make sense to do so if we revisit primary constructors for C# 7.


@jnm2 commented on Fri Aug 26 2016

It is an annoyance though. When you set a getter-only auto property in the constructor, the compiler already breaks the contract in a well defined and helpful way and sets the backing field. There's no reason it couldn't recognize the this cast and do the same for explicit properties.

What can you actually do with an explicit getter-only auto property? You can't access constructor parameters or this in the initializer.


@whoisj commented on Mon Aug 29 2016

the compiler already breaks the contract in a well defined and helpful way and sets the backing field.

This is incorrect. The contract only specifies that a getter property exists. Getters are functions, just hidden from the developer to make code look more 'pretty'. The backing value is part of the structure of the class and must be available for the data the getter retrieves to exist.


@jnm2 commented on Mon Aug 29 2016

This is incorrect. The contract only specifies that a getter property exists. Getters are functions, just hidden from the developer to make code look more 'pretty'. The backing value is part of the structure of the class and must be available for the data the getter retrieves to exist.

I agree entirely. We must be talking past each other. Maybe I could phrase that better. In any case, the point remains: the compiler already allows constructors to use property setter syntax on a property with no setter which is helpful because it allows you to set the otherwise inaccessible backing field. There's no reason the compiler couldn't recognize the this cast and do the same for explicit properties.


@whoisj commented on Mon Aug 29 2016

There's no reason the compiler couldn't recognize the this cast and do the same for explicit properties.

Agreed, having a syntax to set readonly getter backing field would be nice to have, especially since actual fields cannot satisfy interface contracts.

@HaloFour

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2017

I agree that there is a bit of a gap here but I don't think that the compiler should permit casting this in this situation when normally that is verboten. Maybe with the proposed base(T) syntax the property could be initialized like this:

public Impl(string value) {
    base(IFoo).Value = value;
}

Although that has the issue that nothing about IFoo implies that Value can be written to, even in this case.

Perhaps it's just one of those relatively rare cases where it's not unreasonable for the compiler to just require manual implementation:

public class Impl : IFoo {
    private readonly string value;
    string IFoo.Value => value;

    public Impl(string value) {
        this.value = value;
    }
}

@mattwar mattwar added the Discussion label Dec 29, 2017

@binki

This comment has been minimized.

Copy link

commented Feb 9, 2018

I don’t like the (T)this casting syntax but for a different reason: C# doesn’t have a built in C++-style static_cast<T> (yet). So if you do something like ((IThing)this) and the compiler cannot statically determine that this is IThing, it turns into a dynamic runtime cast (which can succeed if a subclass implements IThing). I myself strictly avoid using (T)thing casting syntax because I want to have a dynamic cast turn into a compile time error. I wouldn’t want to have to use the possibly-dynamic-cast syntax in one special case just to set an auto-implemented explicit property.

I wonder if another option would be to use the @ syntax, like this.IFoo@.Value = value; would be OK. @ is already used in other places to allow things which would normally not be identifiers to be treated as identifiers. In JavaScript, the equivalent for setting a property whose name is not a legal identifier production would be the string access this['IFoo.Value'] = value; and this reminds me of the issue with being unable to access explicit interface implementation members.

@yaakov-h

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2018

@binki what's wrong with base(IThing) as suggested above and proposed as a part of Default Interface Methods (#52)?

@gistofj

This comment has been minimized.

Copy link

commented Feb 10, 2018

@binki the entire point of interfaces is that they're dynamic. This is intentional. Late binding allows for many useful scenarios, like test-ability for example.

@binki

This comment has been minimized.

Copy link

commented Feb 11, 2018

@yaakov-h I don't think that's a necessarily wrong way to do it. However, the syntax seems unprecedented. I myself would prefer C# to gain static_cast<IThing>(base) which could be given special treatment within the constructor for initialization of get-only properties while also being useful in other situations.

@whoisj While you could say a common application of interfaces is dynamic feature discovery (e.g., thing is IFoo foo), it is not necessary to use dynamic features to benefit from interfaces. If type compatibility can be statically checked, many errors and accidental regressions from refactoring can be detected at compile time—before even running tests and with potentially less debugging. IMO, new features not directly related to dynamic stuff should not make it hard for developers to remain type safe when using them.

@gistofj

This comment has been minimized.

Copy link

commented Feb 11, 2018

@binki I get what you're saying but how do you do that across library boundaries? The entire point of interface types is to allow passing by contract and nothing else. Sounds to me like you really want C# to handle this how a Rust does, that ship has sailed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.