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# Feature Request] Inline enumeration definition #3497

Closed
andrewducker opened this issue Jun 15, 2015 · 17 comments
Closed

[C# Feature Request] Inline enumeration definition #3497

andrewducker opened this issue Jun 15, 2015 · 17 comments

Comments

@andrewducker
Copy link

It's quite common to end up writing methods that take booleans to say whether certain functionality should be switched on or off. It would be nicer to use enumerations, not least because that way you couldn't end up getting them the wrong way around.

Something like:
void DoSomething(Thing thing, bool withLogging, bool withRollBack){}
which would then be called with:
DoSomething(myThing, true,false);

Looking at that, it's hard to tell whether it's logging without rollback, or rollingback but not logging.

However, the overhead of creating additional enumerations is enough that it puts people off of creating them most of the time. If there was a simple inline declaration format then this would be greatly simplified, and much more likely to be used:
void DoSomething(Thing thing, enum logging{Yes,No}, enum rollBack{Never, OnFailure}){}
which would then be called with:
DoSomething(myThing, logging.Yes, rollback.Never);

Which I think we can all agree is a lot clearer than:
DoSomething(myThing, true,false);

Under the covers there would be a normal enumeration created, scoped to the same accessibility as the method, with a unique name which would be hidden by intellisense in any language which supported this feature. The compiler would only do the conversion from the 'sugarred' name to the real name within the method in which it was defined.

(Inspired partially by the Powershell syntax for parameters making it really easy to define what options you accept.)

@Joe4evr
Copy link

Joe4evr commented Jun 15, 2015

You could also just name your arguments at the callsite, which would be a very good practice anyway:
DoSomething(myThing, withLogging: true, withRollBack: false);
One could probably even write a Roslyn Analyzer to detect when you're passing an unnamed boolean value into a method with a fix to add the parameter name.

@MikeyBurkman
Copy link

I don't think named parameters are the perfect solution to this. When the type is simply a boolean, then yes, it works, but oftentimes a boolean is simply not sufficient. (For instance, the roolback.Onfailure bit in the initial example. And also when more than 2 possible values.)

However, I feel it's just bloat to require a separate enum in the namespace just for clarifying a single parameter in a single function. Surely the compiler could just transform the above enums into "anonymous" enums, so everything still works the same under the covers. The code will read easier, and there's no need to come up with distinct (and thus probably tedious) enum names that are only used once.

The tricky part I see is how to avoid naming conflicts with code outside the method call. In the above code, we couldn't have something in the namespace of the caller named "Rollback" or there would be a nasty ambiguity.

@hraban
Copy link

hraban commented Jun 15, 2015

Two problems with named boolean parameters:

Boolean parameters only work for 2-value enums. The proposed solution in this bug scales to multiple values.

Named parameters are not enforced. As you already mention, this would require an extra analyzer / tool to handle it. I'm not sure if that tool exists? Even if it does, how many people have it?

@ColinDabritz
Copy link

This was from a discussion on an HN thread earlier: https://news.ycombinator.com/item?id=9718012

The advantage I see to using an enum in any way is that it forces the caller to use it explicitly. With named parameters good style would suggest you should and many good coders will, but say you have a function in a popular library. A lot of the calls will be confusing and ambiguous. Additionally, enums are much more expressive, you could allow T-Shirt sizes in S/M/L/XL, and many more use cases, which is why we have enums. To me this makes enums superior for this use, although you can obviously use them already, just not inline.

This suggestion is explicitly about including an enum inline in the declaration of a function.

Regardless of the specific method, having a way to be explicit about what values you expect in a function is nice. This is similar to some of the code contract options, where you can do things like constrain integers to ranges. The question to me is if the additional syntax complexity and learning for developers is worth the clarity provided by having the enum directly associated with the function declaration. On the plus side, it would be optional for the person writing the function, and general 'behind the scenes' to callers unless they went looking at the source code, so that mitigates the 'new syntax confusion' aspect some.

@jcdickinson
Copy link

I'm also part of the "booleans are an anti-pattern" crowd. The DoSomething example is a great reason why. Said DoSomething probably started out as DoSomething(Thing, bool) and because an enum wasn't used a second boolean had to be added for withRollback: something that call-site naming doesn't fix.

However, I'm not a fan of the suggested syntax.

Pros

  • It encourages avoiding bools as the aliasing occurs in the prototype.

Cons

  • Parameters and enums have conflicting casing standards in most coding standards (camelCase and PascalCase respectively).
    • Non-trivial refactoring if you decide to "promote" the enum into the namespace
    • Also break ABI if you promote the enum.
    • Breaks coding standards and hence is ugly.
  • Doesn't parse easily to the human eye.
  • Huge amounts of horizontal screen estate consumed with no obvious way to format it vertically.

Proposal

void DoSomething(Thing myThing, SomethingOptions options)
  with: [Flags] enum SomethingOptions { None, Logging, Rollback }

or (more DRY):

void DoSomething(Thing myThing, enum options)
  where options: [Flags] SomethingOptions { None, Logging, Rollback }

with vertical formatting:

void DoSomething(Thing myThing, enum options)
  where options: [Flags] SomethingOptions { 
    None, 
    Logging, 
    Rollback
  }

It's still ugly, I don't like it but it does seem better. One possible addition (not sure I like it, gets dangerously close to yoda conditionals) is to also make only enums more bool-like thereby providing even more incentive to avoid bools:

if (options /* != (SomethingOptions)0 */)

Alternative

Solve the true/false ambiguity alone:

void DoSomething(Thing myThing, {NoLogging, Logging} logging, {NoRollback, Rollback} rollback)
DoSomething(myThing, Logging, NoRollback);

NoLogging, Logging, NoRollback, Rollback can be used at call-sites as aliases for true and false. No enum gets compiled for this. Effectively:

void DoSomething(Thing myThing, 
  [CompilerServices.BooleanAlias("NoLogging", "Logging")] bool logging, 
  [CompilerServices.BooleanAlias("NoRollback", "Rollback")] bool rollback)
DoSomething(myThing, true, false);

@chrisaut
Copy link

I think this is a great suggestion.

Only one thing I would add, is that the created enum should follow a clear known type, so that later on, if say I need more values, I can manually host it outside and callsites can remain the same.

So this:

namespace NS {
 public class C {
   void DoSomething(Thing thing, enum logging{Yes,No}, enum rollBack{Never, OnFailure}){}
  }
}

would generate

namespace NS {
 public enum Logging { Yes, No }
 public enum RollBack { Never, OnFailure }
 public class C {
   void DoSomething(Thing thing, NS.Logging logging, NS.Rollback rollBack) {}
  }
}

instead of some weird compiler generated name or something.

@HaloFour
Copy link

The generated code seems significantly clearer to me than the syntax candy used to generate it. You don't save all that much in the way of keystrokes either, just the accessibility modifier and a name. The latter of which I wouldn't trust to auto-generation anyway, at least not for public members.

@dsaf
Copy link

dsaf commented Jun 15, 2015

  1. what is the suggested syntax for default values?

  2. I am not sure if "overhead" is a good enough reason. In that case I can say that we should also support something like this (to save the overhead of creating Dependency Injection driven interfaces):

public interface ISample class Sample
{
    public string Name { get; private set; }
}

//Will generate the following interface automagically listing all public members:
/*
public interface ISample
{
    string Name { get; }
}
*/
  1. Is it just me or it feels like either a false dichotomy or an Always value missing:
    public enum RollBack { Never, OnFailure }

@mburbea
Copy link

mburbea commented Jun 15, 2015

@dsaf, as they are still enum, you can always pass 0 (this is awful please stop doing this).

And I think thats the point in that usually the boolean flag doesn't tell you exactly what both sides of the coin mean. It might mean, Never and Always or it could be Never and OnFailure. This is the problem with bools in general.

However, I just think this is a lot of work for very little gain. I think the Options would be muddled in the declaration, and while this somewhat solves my usual complaints of enums, as you in theory could never pass a random number, (you can't cast to something you can't name), I just think its most likely going to be a breaking change waiting to happen.

If you can name them e.g. RollbackOptions{Never,OnFailure}, what happens when you have two methods with conflicting options?

How would you even call it? DoSomething(params, Never) would look odd to me as I would expect Never to be a class or a static value defined somewhere, not directly at the callsite. Not an enum. If I have to say RollBackOptions.Never, then can I still do (RollbackOptions)7?

@MrJul
Copy link
Contributor

MrJul commented Jun 16, 2015

The generated code seems significantly clearer to me than the syntax candy used to generate it. You don't save all that much in the way of keystrokes either, just the accessibility modifier and a name. The latter of which I wouldn't trust to auto-generation anyway, at least not for public members.

I totally agree, the proposal doesn't seem worth it to me. IMHO, the original syntax is clearer and not much typing is saved.

Moreover, this proposal would raise many questions:

  • If the enum is generated in the containing namespace, what does happen if a type already exists by that name? Is it a compilation error, meaning you can't have two similarly named enum parameters in a whole namespace?
  • If the enum is generated in the containing class, the callers will become much more verbose. Plus, you still have a conflict problem if two methods of a class define an enum parameter with the same name. Oh, and that won't work in interfaces.
  • Are similarly defined enums with this syntax merged into a single one?
    • If yes, how are two enums considered similar? Same name only? Same name and values? Does the order of the values matter? Assuming you solve this, then you now have to maintain a definition for a single enum at several places. What does happen if a new enum value is added at one place but not the others?
    • If no, there's now a bunch of duplicate types in the codebase.
  • How is the visibility of the generated enum determined? C# always uses the most restricted visibility when nothing is specified, but that won't work well here, and will probably need special cases.

Additionally, renaming a parameter becomes a binary breaking change. (Currently it's only a source breaking change, if named parameters are used)

@MikeyBurkman
Copy link

I'm not sure why an enum generated by the compiler would ever have the exact same name as the one you gave it. The compiler already creates getters and setters with internal(ish) names, and I've seen very few complaints about that. Just because you name it "Rollback" for ease of client use, doesn't mean the compiler can't mangle it behind the scenes. (As an aside, these enums should probably be treated as special in reflection code, just as properties are, which means a CLR change.)

The goal here is an enumerated type for a single parameter in a method, not just an alias for a boolean. This doesn't necessarily have to be a strict enum. This could, for instance, be a string, but one that the compiler limits to a few defined values. However, this would require runtime checks if not a string literal, so it's probably not the best choice. Hopefully it can be a denotable type, so you can pass it around as a parameter, but that's the really hard part -- It kind of needs to be scoped to the function it belongs to.

People saying that we already have solutions to this reminds me a lot of the opposition to lambdas in Java 8. Sure, making anonymous classes in Java isn't that much more difficult, but it's tedious boilerplate that hides your intent, and adds enough of a barrier that people will likely still opt for the easier anti-pattern. (Not to mention that using enums defined in your namespace requires polluting that namespace with enums that are only applicable to a single function. If two functions each require a "Logging" enum, and they have slightly different values, then good luck coming up with decent names for both.)

@HaloFour
Copy link

The compiler already creates getters and setters with internal(ish) names, and I've seen very few complaints about that.

Because the name of the property accessor methods is irrelevant. The property metadata itself is a simple name which points to one or more accessor methods which can have any name as long as they have the proper signature. This is not true of enums.

Just because you name it "Rollback" for ease of client use, doesn't mean the compiler can't mangle it behind the scenes.

The compiler can't just produce any old mangled name. The name of the enum is part of the contract and accessible outside of the class.

Hopefully it can be a denotable type, so you can pass it around as a parameter, but that's the really hard part -- It kind of needs to be scoped to the function it belongs to.

But enums can't belong to methods, at best they can be nested in the declaring class.

People saying that we already have solutions to this reminds me a lot of the opposition to lambdas in Java 8.

There is a fine line between succinct and terse. An accessibility modifier (which is already optional) and a name don't represent excessive boilerplate, especially considering that the enum must be given a unique and predictable name.

@dsaf
Copy link

dsaf commented Jun 17, 2015

Maybe there is a way of implementing it via contracts #119 and custom literals #263?

public void DoSomething(Aenum rollBack)
    requires rollBack == "Never"_ae || rollBack == "OnFailure"_ae
{
    //...
}

//Stands for AnonymousEnum
public struct Aenum
{
//Custom literal implementation: ""_ae.
}

PS: yes, it does look beautiful.

@HaloFour
Copy link

@dsaf Doesn't look like that fits the spirit of this proposal which is to shove everything inline and reduce typing. Where does Aenum get it's values and is the requires clause even validatable at compile time?

@default0
Copy link

public void DoSomething({ Yes, No } someOptions, { Maybe, Not } otherOptions)
{
    // stuff
}

DoSomething(someOptions.Yes, otherOptions.Maybe);

If the syntax is like that, I think I could make friends with it. But it leaves the question where and how the enums would be defined. They technically should be part of the method signature, which is not possible in IL, because frankly every other variant seems horrible: They'd have to be public, in-scope when the method is accessible, and referable via non-prefixed names (they become too clunky for the caller to be justifiable otherwise), which would mean they must be declared at the scope of the namespace, which would make them visible in IntelliSense to everyone who is able to use that class, despite only being relevant for that method. Further, they would have to be either public or internal, so this will interact poorly with protected/protected internal/private methods (and banning the feature for them would seem weird).
Since a proper implementation (adding the notion of method signature based enums) is way too over the top for saving some typing in some situations, this really isn't worth it at all.

Best Regards

@alrz
Copy link
Member

alrz commented Oct 5, 2015

What about C-style declarations like void F(enum {Yes, No} options) { } looks like a "anonymous enum and a trailing declarator". This helps to keep camel case convention for method args and also more readable. and for invocation, #952 can be helpful, so simply F(Yes); However, I suspect if those enums can be inferred from usage (#8272).

@gafter
Copy link
Member

gafter commented Mar 27, 2017

We are now taking language feature discussion in other repositories:

Features that are under active design or development, or which are "championed" by someone on the language design team, have already been moved either as issues or as checked-in design documents. For example, the proposal in this repo "Proposal: Partial interface implementation a.k.a. Traits" (issue 16139 and a few other issues that request the same thing) are now tracked by the language team at issue 52 in https://github.com/dotnet/csharplang/issues, and there is a draft spec at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md and further discussion at issue 288 in https://github.com/dotnet/csharplang/issues. Prototyping of the compiler portion of language features is still tracked here; see, for example, https://github.com/dotnet/roslyn/tree/features/DefaultInterfaceImplementation and issue 17952.

In order to facilitate that transition, we have started closing language design discussions from the roslyn repo with a note briefly explaining why. When we are aware of an existing discussion for the feature already in the new repo, we are adding a link to that. But we're not adding new issues to the new repos for existing discussions in this repo that the language design team does not currently envision taking on. Our intent is to eventually close the language design issues in the Roslyn repo and encourage discussion in one of the new repos instead.

Our intent is not to shut down discussion on language design - you can still continue discussion on the closed issues if you want - but rather we would like to encourage people to move discussion to where we are more likely to be paying attention (the new repo), or to abandon discussions that are no longer of interest to you.

If you happen to notice that one of the closed issues has a relevant issue in the new repo, and we have not added a link to the new issue, we would appreciate you providing a link from the old to the new discussion. That way people who are still interested in the discussion can start paying attention to the new issue.

Also, we'd welcome any ideas you might have on how we could better manage the transition. Comments and discussion about closing and/or moving issues should be directed to #18002. Comments and discussion about this issue can take place here or on an issue in the relevant repo.


I am not moving this particular issue because I don't have confidence that the LDM would likely consider doing this.

@gafter gafter closed this as completed Mar 27, 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