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] Allow otherwise unresolvable simple names to resolve to an enum member of the target type #952

Closed
ThatRendle opened this issue Feb 28, 2015 · 25 comments

Comments

@ThatRendle
Copy link

There are probably dozens of reasons why this wouldn't work, but I was just looking at an API with a method ListContainers(bool) (it was in Go) and thinking about enums being better than Boolean flags, when it occurred to me that in .NET, expressing enums can be verbose.

In cases where the type can be determined from argument, property or field type, why does one have to type the enum name? Would it not be easier and more readable to just use the enum member name, like type.GetProperties(Public | Static)?

This is bound to be (a) stupid because of obvious reasons and also (b) already possible in F#, so I apologise in advance for wasting everybody's time.

@ashmind
Copy link
Contributor

ashmind commented Feb 28, 2015

This is something I wanted for a while (one way or another), but wouldn't static imports cover most of the use cases? Sure they aren't as neat since you need an import, but they do not need any other new features to be added.

@gafter
Copy link
Member

gafter commented Mar 1, 2015

@markrendle This already works with static import, and there is at least one source file in the Roslyn compiler that uses it extensively.

@gafter gafter closed this as completed Mar 1, 2015
@gafter gafter added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Mar 1, 2015
@paulomorgado
Copy link

@gafter, I kind of like @markrendle's proposal. This would equate to have enums implicitly staticaly imported when the value is known to be of an enum type. This would be scoped to that expression instead of the whole file.

@gafter
Copy link
Member

gafter commented Mar 3, 2015

How would we do this without it being a breaking change?

@paulomorgado
Copy link

I haven't given that much thought, but I suppose it's the same as with static imports. Only that the static import is only in effect at that expression that as a type of the enum.

Imangine this:

namespace Enums
{
    public enum SomeEnum
    {
        SomeValue,
        AnotherValue
    }
}

namespace ConsoleApplication5
{
    using Enums;
    using static Enums.SomeEnum;

    class Program
    {
        static void Main(string[] args)
        {
            MethodThatUsesSomeEnum(SomeValue);
        }

        static void MethodThatUsesSomeEnum(SomeEnum v)
        {

        }
    }
}

What I'm sayning is that this would work just the same if using static Enums.SomeEnum; was not there.

Whatever is not valid with static imports, is not valid here.

@gafter
Copy link
Member

gafter commented Mar 3, 2015

@paulomorgado With static import, the meaning of your code doesn't change unless you change your source code. I'm concerned that your proposal could change the meaning of existing code when the source has not changed. I'd need to see the precise name lookup rules you propose to know.

@ThatRendle
Copy link
Author

Hi, sorry, been unable to post for a few days so couldn't respond.

The issue with using static is that it's pretty common for multiple enums in a particular area to have common member names; for example BindingFlags and FieldAttributeFlags both have Public (I like this example because having half a dozen verbose BindingFlags values OR'd together really clutters up a GetProperties call). I'm not sure what the rules are for having two using static declarations with conflicting member names?

As far as changing the meaning of existing code, there's obviously the possibility of introducing ambiguity where there is a member in scope with the same name as an enum member, either a property or constant of the same type, or a member of a type for which an overload of the method being called is also available. If those ambiguities caused compiler errors where previously there were none then obviously that's bad. If those ambiguities were resolved by defaulting to the original behaviour, and requiring explicit enum qualification, then I don't know how that shakes out.

@paulomorgado
Copy link

@gafter, it's @markrendle proposal, not mine. I'm just supporting it.

This might make code, that otherwise wouldn't compile, compile. But I don't think it would break any code that compiles now.

But I do understand your caution. Importing statics will involve explicitly changing the code while this wouldn't.

@gafter
Copy link
Member

gafter commented Mar 4, 2015

OK, I'll reopen this so it can be considered as a separate approach for handling enums.

@gafter gafter reopened this Mar 4, 2015
@gafter gafter removed the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Mar 4, 2015
@gafter gafter added this to the C# 7 and VB 15 milestone Mar 4, 2015
@gafter gafter changed the title [Proposal] Infer enum names when enum type is implicit [Proposal] Allow otherwise unresolvable simple names to resolve to an enum member of the target type Mar 4, 2015
@TedDriggs
Copy link

This still seems brittle to me. Consider the following case, where SomeClass has a constructor that accepts a Color:

public class Okay
{
    public void DoSomething()
    {
        var foo = new SomeClass(Red);
    }
}

Now I add a new property to my class:

public class Okay
{
    public Okay(Color reddish)
    {
        this.Red = reddish;
    }

    public Color Red { get; private set; }

    public void DoSomething()
    {
        var foo = new SomeClass(Red);
    }
}

At this point, the behavior of DoSomething has silently changed from passing Color.Red to passing Okay.Red. There is no warning or error: Both snippets are valid under the proposed syntax. This particular example is contrived, but I could easily see this becoming an issue with inheritance, partial classes, or properties imported via using static

@chrisaut
Copy link

chrisaut commented Mar 4, 2015

Wouldn't the same problem occur with that code if there was a using static System. Color? Meaning of the code silently changing?

@gafter
Copy link
Member

gafter commented Mar 4, 2015

@markrendle Can you tell me what changes you are proposing to the name lookup rules? In the case of a method call, one does not have a target type until the proper overload has been resolved, but we need to bind the arguments in order to resolve the overload. I don't see how to specify (or implement) the feature requested here.

@paulomorgado
Copy link

@TedDriggs, you changed the code and caused it. What @gafter is concerned about is unchanged compilable code where some the compiler will now assume it's an enum member where it wasn't before.

As @chrisaut stated, if you use static imports, you'll have the same problem.

@paulomorgado
Copy link

@gafter, it's more complicated than this but let me try.

  1. If the expression with the enum members is not resolvable, statically import all enums in scope
  2. ...

It's a tall order...

What if this would only happen if the parameter name was specified?

@gafter
Copy link
Member

gafter commented Mar 4, 2015

@paulomorgado I think that could work... what if the name is a member of more than one enum type? Ambiguity error?

enum Mood { cheerful, blue }
enum Color { cherry, blue }

var k = blue; // ??

@gafter
Copy link
Member

gafter commented Mar 4, 2015

Having the parameter name specified shouldn't make any difference, as it could be the name of a parameter in more than one method.

@ThatRendle
Copy link
Author

@gafter I'm afraid I don't know enough about name lookup rules to answer that, but what @paulomorgado said sounds right to me :)

Obviously var x = UnqualifiedMemberName wouldn't work; this is similar to var f = MethodName not being valid. But BindingFlags x = Public would work.

@paulomorgado
Copy link

@gafter, this would work just the same as static imports but only scoped to the method call.

In order for this to work, all enums in scope would have to be imported. And that can be a lot.

@judemelancon
Copy link

The concern raised by @TedDriggs strikes me as significant enough to merit being addressed more thoroughly. (I will proceed from the assumption he meant ConsoleColor for his example, as it is an enum.)

First off, I must admit I find this change quite aesthetically pleasant, and it really would make some code much less verbose. As such, it pains me to say that this feature is a spooky action at a distance nightmare. Adding any field, property, local, or parameter with the same name as any enumeration constant (even in a different assembly) can silently change the meaning of code where both are in scope. Unless the compiler is going to warn for every field, property, local, or parameter in scope at the same time as an enum value with the same name, it will be quite transparent to the programmer, as well. Even if such a warning were implemented, the sad truth is that many ignore warnings, and it would be likely so noisy as to further encourage that.

The analogous situation with a static import has the directive there to see at debugging time. Furthermore, it's a choice to use that feature.

@TedDriggs brought all of the following scenarios up, but I feel they are serious enough to bear reiterating:

  • This turns adding a field or property to a non-sealed type into a breaking change for library or generated code.
  • It likewise makes adding a field or property to an automatically generated partial type into a breaking change.
  • This turns adding a static field or property to a type into a breaking change for library or generated code. (The type might be statically imported somewhere.)

Imagine a currently existing FooLibrary (or a code generation tool) that added such a property between versions 1.1 and 1.2. This wasn't a breaking change, so it wouldn't make such a list if it were kept. Imagine code using FooLibrary 1.1 that moves to a version of C# including this feature. Now upgrading FooLibrary to 1.2 can break that code. This feature would retroactively make any change from the list above breaking for library code.

@gafter gafter self-assigned this Nov 18, 2015
@gafter
Copy link
Member

gafter commented Nov 18, 2015

We're considering this for certain contexts, such as the branches of a switch. See #6739 for related discussion.

@HaloFour
Copy link

@gafter I hope you didn't help Sun take out a patent on that one. 😀

@HaloFour
Copy link

@gafter

I assume that the meaning of the following would not change. Could be confusing, but probably not a common scenario:

public enum MyEnum { Foo, Bar, Baz };
const MyEnum Foo = MyEnum.Bar;

MyEnum value = ...;
switch (value) {
    case Foo:
        Console.WriteLine("Surprise, I'm Bar!");
        break;
    case Baz:
        Console.WriteLine("I'm Baz!");
        break;
}

@gafter
Copy link
Member

gafter commented Nov 18, 2015

@HaloFour Right, it would not change. That is the reason for the words "otherwise unresolvable" in the title. It only tries this lookup if the rules from the previous version of the language fail to find something.

@bondsbw
Copy link

bondsbw commented Feb 1, 2016

@TedDriggs @judemelancon
I'm not sure this actually represents a breaking change to working code, if you assume that enum member resolution has lower precedence than the others. Anything that did compile would compile the same, and any cases where the enums could be resolved according to this proposal would not have compiled previously.

Perhaps a compiler warning is warranted.

Another assumption is that, like today, enum member resolution would not allow implicit conversions from one type of enum to another. Resolution would need to consider only the target enum type.

@gafter
Copy link
Member

gafter commented Mar 27, 2017

Issue moved to dotnet/csharplang #354 via ZenHub

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

9 participants