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

Overloading on in/byval should cause an error #22132

Closed
VSadov opened this issue Sep 14, 2017 · 6 comments
Closed

Overloading on in/byval should cause an error #22132

VSadov opened this issue Sep 14, 2017 · 6 comments

Comments

@VSadov
Copy link
Member

VSadov commented Sep 14, 2017

The following seems to not result in an error, but should:

namespace ConsoleApp33
{
    class Program
    {
        static void Main(string[] args)
        {
            var o = new P1();
        }

        public void Foo(in int z) { }

        public void Foo(int z) { }
    }

    class P1: Program
    {

    }
}

We should also add tests to make sure we select the correct overload, also in case of in situation of ambiguity (because of inheritance or extension methods)

@tannergooding
Copy link
Member

@VSadov, why would that cause an error (I'm guessing a language design decision)?

I would expect a user is allowed to differentiate by Foo(in 5) (or Foo(ref 5) or Foo(ref readonly 5)) vs Foo(5).

This makes adding ref readonly overloads, where you only had byval overloads previously, a breaking change.

This is a bit confusing, because users are allowed to add a ref overload and have it work (so I can have Foo(ref int z) and Foo(int z), but not Foo(in int z) and Foo(int z)).

@VSadov VSadov changed the title Overloading on in/byval shoudl cause an error Overloading on in/byval should cause an error Sep 15, 2017
@VSadov
Copy link
Member Author

VSadov commented Sep 15, 2017

Right now there is no way to guide overload resolution at the call site.
You cannot do Foo(ref readonly 42). I can see this as a possibility in the future, but I still would not guess what syntax would be for operators - ref readonly 42 + ref readonly 42 ?

Anyways, as it is currently would not be possible to call either of the overloads, we will disallow such situations. Once/if it is possible to disambiguate, we can relax the restriction.

@OmarTawfik
Copy link
Contributor

@VSadov do we have a decision on this? should we move this to 15.later?

@VSadov
Copy link
Member Author

VSadov commented Sep 22, 2017

It is not officially confirmed, but otherwise nearly decided to allow this kind of overloading, even when it is not callable (if common, we can introduce disambiguating syntax at call site in the future).

@jcouv jcouv moved this from Backlog to C# 7.2 in Compiler: Needs LDM attention Sep 22, 2017
@VSadov
Copy link
Member Author

VSadov commented Sep 27, 2017

It is confirmed with LDM to be ok.

@VSadov VSadov closed this as completed Sep 27, 2017
@tannergooding
Copy link
Member

@VSadov, do you have a link to the design notes?

@jcouv jcouv removed this from C# 7.2 Backlog in Compiler: Needs LDM attention Sep 30, 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

3 participants