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: Ternary Operator Scoping #15538

Closed
HaloFour opened this issue Nov 26, 2016 · 59 comments
Closed

Proposal: Ternary Operator Scoping #15538

HaloFour opened this issue Nov 26, 2016 · 59 comments
Assignees
Labels
Area-Language Design Language-C# New Language Feature - Out Variable Declaration Out Variable Declaration New Language Feature - Pattern Matching Pattern Matching Resolution-By Design The behavior reported in the issue matches the current design
Milestone

Comments

@HaloFour
Copy link

I propose that if an identifier is introduced within the body of an expression used with the ternary conditional operator (?:) that the scope of said identifier should be narrow. Specifically said identifiers would only be accessible within the first_expression and second_expression expressions.

I give the following non-exhaustive list of reasons as to why I think that this is reasonable:

  1. Said identifiers could be considered a "working space" of the current expression and would have no purpose after the final result is calculated.
  2. In the case of pattern variables the identifier would almost certainly not be definitely assigned and therefore effectively unusable in the wider scope.
  3. The conditional operator can be viewed as a simpler and terser form of match which would likely carry over the same narrow scoping as switch.

I'll update this proposal with additional reasons as enumerated in the comments. I just wanted to get the ball rolling.

@HaloFour
Copy link
Author

@alrz
Copy link
Contributor

alrz commented Nov 26, 2016

I think the issue is not specific to the ternary operator, otherwise you could "ditch" if scoping,

if (e is T t && t.IsFoo ? true : false) 

I think it belongs to variable declarations and return (plus other statements e.g. invocation, object creation, etc — basically all statements except for if),

var x = e is T t && t.IsFoo; 
return e is T t && t.IsFoo;
F(e is T t && t.IsFoo);
new C(e is T t && t.IsFoo);

and I'm not even using a ternary operator here.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Nov 26, 2016

One concern i have is how this interacts with declaration expressions. As expressions can be nested, and because you're now proposing a scope be at an expression level, it feels like people could end up with confusing interactions. For example:

x is Y z ? Foo(out var m) : Bar(out var n)

--

Another concern is with this statement:

In the case of pattern variables the identifier would almost certainly not be definitely assigned and therefore effectively unusable in the wider scope.

Just because something is not definitely assigned does not make it unusable. People would be free to give the value an assignment if they so wanted later on. Similar to how we can see code that does the following:

if (!(x is Y z))
{
    // z not assigned here.
    ...
    z = something
    ...
}

Just because a pattern does not match does not make me feel that is necessary that the variable will not be used.

@alrz
Copy link
Contributor

alrz commented Nov 26, 2016

@CyrusNajmabadi As you can see in my previous comment, this is not specific to the ternary operator (expression context). I propose narrow scoping for pattern variables in all statements except for if which per your example, is an exception.

@HaloFour
Copy link
Author

@CyrusNajmabadi

One concern i have is how this interacts with declaration expressions. As expressions can be nested, and because you're now proposing a scope be at an expression level, it feels like people could end up with confusing interactions.

This is possible. I saw the condition expression as the best place to start as I see that as being the least likely place to intend to introduce a new variable into the wider scope. I'm more than happy to explore different options.

Just because something is not definitely assigned does not make it unusable.

Indeed not. At least with if statements the block used as the embedded statement can manually assign the variable. That's not the case with the conditional operator. The only way the pattern variable would be assigned is if it is subsequently used as an out argument somewhere within the second_expression. Otherwise while the variable could eventually be assigned that value would more than likely be unrelated to anything pertaining to that original expression. Sure, it's perfectly legal to reuse variables for different purposes within the same scope, but that is definitely a code smell. (I am fully aware that this is how C# and IL assembly manage block-level scoping under the hood.)

@alrz

I'd probably agree, but it seems that trying to have different scopes for out declarations and pattern variables is a non-starter, and that the advocates for out declarations wish it to satisfy the following use case:

int.TryParse(s, out int i);
// use i here, even if parse failed

@alrz
Copy link
Contributor

alrz commented Nov 26, 2016

@HaloFour out parameters have nothing to do with "failure", TryX is just a usage pattern as long as the compiler concerned. On the other hand, the is operator is all about failure and the compiler can reason about it, hence the "definitely not assigned" error. So it would totally make sense to maintain a narrow scope around its validity. The use case @CyrusNajmabadi mentioned is specific to if statements and no more. Your example certainly does not make sense for is operator and we can safely take advantage of narrow scoping without limiting any potential use case. In fact, outside if statement, any usage of the leaked variable can be considered a code smell and should not be encouraged because it certainly hampers readability and increases complexity of the code (plus, as you said, the variable would be definitely unrelated to any subsequent code), not to mention that all those use cases involve mutation and I think we all agree that it's not a good thing (again, if is an exception, and the use cases are not that unreasonable). Furthermore, I don't see any reason for consistent scoping for is and out var as the two don't have anything in common in terms of usage and semantics.

@HaloFour
Copy link
Author

@alrz

Furthermore, I don't see any reason for consistent scoping for is and out var as the two don't have anything in common in terms of usage and semantics.

I'd agree. So would @CyrusNajmabadi it would seem. But it sounds like the LDM decided the two would behave identically.

I've also made the "definitely not assigned" argument multiple times to no avail.

@alrz
Copy link
Contributor

alrz commented Nov 26, 2016

cyh8rpnwqaa-nvm

@CyrusNajmabadi
Copy link
Member

I propose narrow scoping for pattern variables in all statements except for if which per your example, is an exception.

I was just using 'if' as an example of how something not-definite-assigned could still be usable.

The same is true of patterns. Someone may do a pattern test that leaves the value not-definitely assigned. However, they may still end up definitely assigning it, making is usable. Hence why i take issue with:

In the case of pattern variables the identifier would almost certainly not be definitely assigned and therefore effectively unusable in the wider scope.

The first part of the sentence is correct (the variable would not be definitely assigned), but the second part is the part that i don't agree with. It is not "effectively unusable". It simply cannot be read until it has been definitely assigned. That's something that people are completely capable of doing in their code.

@aluanhaddad
Copy link

The first part of the sentence is correct (the variable would not be definitely assigned), but the second part is the part that i don't agree with. It is not "effectively unusable". It simply cannot be read until it has been definitely assigned. That's something that people are completely capable of doing in their code.

I definitely agree with this. However, I find @HaloFour's assertion that

Sure, it's perfectly legal to reuse variables for different purposes within the same scope, but that is definitely a code smell.

to be highly compelling. For example, var declarations are mutable bindings but declaring a fresh variable instead of reassigning to an existing one is widely considered good practice(citation needed). I think this is related to reassigning standard, by value, parameters.

@gafter
Copy link
Member

gafter commented Nov 27, 2016

@MadsTorgersen I think this is a suitable topic for some upcoming LDM.

@alrz
Copy link
Contributor

alrz commented Nov 30, 2016

I did a quick scan on roslyn and corefx repos, turns out, the subsequent code of all negated is operators that did a cast afterwards, is unreachable. In other words:

if (!(e is T))
{
   // return, throw, break, continue
}

var x = (T)e; // or F((T)e);

And it actually makes sense. because otherwise it's a potential bug!

So, they all can be rewritten to the following to avoid double type check (#14239):

var x = e as T ?? return; // or throw, break, continue

@CyrusNajmabadi's example if (!(x is Y z)) { z = something; } is no exception:

var z = x is Y ?? something;

Perhaps, if should also maintain narrow scoping for is var among other statements.

@paulomorgado
Copy link

I don't support this proposal in isolation. Unless the same happens with if, while and such, Like variables introduced in the for statement.

If not, then it will just be confusing!

@alrz
Copy link
Contributor

alrz commented Nov 30, 2016

@paulomorgado

I think so. A scope at an expression level is confusing. As I said. this issue is not specific to ternary operators. For example, there is absolutely no point for t in return e is T t && t.Foo; to leak.

@gafter
Copy link
Member

gafter commented Dec 1, 2016

The LDM discussed this yesterday, and we considered the example

var state = TryFoo(out int i) ? States.Found : States.NotFound;
// use i;

The proposal would disallow this, but we can find nothing so wrong with this style of code that we would want to make it illegal. Confirm the status quo.

@gafter gafter closed this as completed Dec 1, 2016
@gafter gafter added the Resolution-By Design The behavior reported in the issue matches the current design label Dec 1, 2016
@alrz
Copy link
Contributor

alrz commented Dec 1, 2016

@gafter I think the proposal is¹ about pattern variables, not out var.

var state = e is T t ? /* use i */ : otherwise;

¹ was.

@gafter
Copy link
Member

gafter commented Dec 1, 2016

@alrz Pattern variables can be definitely assigned after the conditional expression, and useful later.

var state = complexExpression is var x && x.Something ? a : b;
// use x here

@DavidArno
Copy link

@alrz,

The team have unfortunately made the decision that is T expressions should have the same scope as out var's, so weird scoping is temporary is T bars looks like it's here to stay.

@alrz
Copy link
Contributor

alrz commented Dec 1, 2016

@gafter I think sequence/declaration expressions are more suitable than that.

var state = (var x = complexExression; x.Something) ? a : b;

In my opinion is var is just redundant since it always returns true.

@DavidArno
Copy link

@gafter,

In the absolutely vast majority of cases, they won't be useful as they will not be definitely assigned. Sure, they can be (re)assigned, but they could be declared and assigned if they didn't leak, so that offers no advantage either.

But you want consistency over usefulness and so we'll have to live with that weirdness.

@gafter
Copy link
Member

gafter commented Dec 2, 2016

@alrz The decision to make pattern variables mutable is an assertion that assigning to them isn't "scary". So it would not make sense to base other language design decisions on an inconsistent position about that.

@DavidArno
Copy link

@CyrusNajmabadi,

Not to belabor the point, but as has been pointed out a few times "weirdness" is in the eye of the beholder.

Absolutely agree:

If you come from my position as seeing out parameters as at least a code smell, if not an outright anti-pattern, that you don't use, then is T scoping will appear ridiculous.

If you like out parameters and are excited by out var and that's your focus, then is T behaving the same way will make sense. Such folk presumably are also likely to re-assign variables, rather than treating them as immutable, so will struggle less with the strangeness of an unusable (without (re)assignment) variable being in scope.

Of course, these days, "best practice" says head down the pit of success by eg avoiding mutability, so that second position is one that only folk who do not follow the latest ideas in good practice are likely to take.

It could (and has) been argued that the language team therefore ought to be guiding folk toward good practices. But the language team have made it clear that they take an actively neutral stance on good practice: they provide what people ask for; not what would be good for them. As such, we've ended up with vast amounts of time and energy being put into out var, and scoping rules that a great many C# developers will not thank you for, rather than eg putting the time and effort into the universally popular match expression. But we are where we are...

@DavidArno
Copy link

DavidArno commented Dec 2, 2016

@gafter,

Re:

var x = M(e is int y ? y : y=0);
// x and y are definitely assigned here

Sure, y is definitely assigned here, but can you think of a genuine situation where you'd want to use y obtained in such a way? If y were genuinely useful, then that "y=0" statement shouts "code smell" to me. I'd want to follow the decades-old-but-still-good "avoid assignment in expressions" advice from my C days, and would rewrite it like the following to make it clearer:

var y = e is int i ? i : 0;
var x = M(y);
// x and y are definitely assigned here; 
// i is no longer useful, but sadly is still in scope here. Grr!

I really do not see any real (good practice) use for this is T leakage. Its only reason for being this way, surely, is to be consistent with out var?

@alrz
Copy link
Contributor

alrz commented Dec 2, 2016

"weirdness" is in the eye of the beholder.

In my eyes, compiler's job is to disallow any "bad" code, and it's not just about syntax and semantics, it's also about preventing accidental bugs and that's where language designers should help. I think it should try to produce compile-time errors to prevent potential runtime errors and that's just the necessary and sufficient reason to do so. I believe the more compile-time errors a compiler produces, the more powerful it is. Advertising mutability and such as a "feature" and saying that there is nothing that you cannot do with a language, whether it is shooting yourself in the foot or not, doesn't seem to be the way of designing a language. It's how you sell it.

@qrli
Copy link

qrli commented Dec 2, 2016

var x = M(e is int y ? y : y=0);
// x and y are definitely assigned here

This is a bad example to me. So, to use the leaky matched variable, people have to use this hack. I guess it would become a stackoverflow question&answer. Some hacks are useful, e.g. for performance, but this hack has no good reason to exist except to obfuscate the code IMO.

And unlike if, which can use {if() ... } to reduce the scope, {var x = M(e is int y ? y : 0); } would render x useless.

@alrz
Copy link
Contributor

alrz commented Dec 2, 2016

Oh, "hack" you say, it's not always bad. In case folks want to use if but do not want the variable to leak:

while (e is T t)
{
  // ...
  break;
}

easy peasy.

@DavidArno
Copy link

DavidArno commented Dec 2, 2016

Inspired by @alrz, it occurs to me that local functions can be used to prevent is T variables leaking. So taking @gafter's example:

var x = M(e is int y ? y : y=0);
// x and y are definitely assigned here

That can be rewritten as:

int getY(object o) => o is int y ? y : 0;
var x = M(getY(e));
// y isn't in scope here. Evil leaking variable feature defeated! Yay! :)

😁

@orthoxerox
Copy link
Contributor

@DavidArno, be careful, next LDM meeting notes might change that too ;)

@alrz
Copy link
Contributor

alrz commented Dec 2, 2016

@DavidArno This is so going to be an idiom. 😆

@DavidArno
Copy link

DavidArno commented Dec 2, 2016

Further thoughts on this, again completely inspired by @alrz's if/while hack. I'm guessing most folk would probably not want to use it directly to work around variable scope leakage. However, when (if?) source generators get added to the language, then the while scope hack, along with eg using local functions to encapsulate is T expressions to stop them leaking too, could be indirectly used. A source generator could detect, eg:

public void F()
{
    if (e is int x)
    {
        do something with x;
    }
}

and transform it into:

replace public void F()
{
    when (e is int x)
    {
        do something with x;
        break;
    }
}

According to #15595, the generator will replace the if syntax tree with a while one during compilation, thus any attempt to use x after the if block will then generate an out-of-scope error. Coupled with analysers to eg enforce immutability by default, such generators could then form a tool set that turns C# into a "pit of success" language.

Or am I deluding myself and we'll all just learn to live with variable scope leakage?

@qrli
Copy link

qrli commented Dec 3, 2016

@alrz I mean that specific hack to get y definitely assigned is bad. Not others.
@alrz @DavidArno Why use while when you can simply use { }?

{ if (e is int x) {  DoSomethingWith(x); } }
// x is not in scope here

@qrli
Copy link

qrli commented Dec 3, 2016

I just created #15651 to modify sequence expression a bit to allow

var x = { e is int y ? y : 0 }
// x in scope but y is not here

@iam3yal
Copy link

iam3yal commented Dec 3, 2016

@DavidArno

Or am I deluding myself and we'll all just learn to live with variable scope leakage?

There is only one question here why we need to use heavy machinery at all? this needs to somehow be baked into the language.

It doesn't make sense to me that people will use hacks/tricks or use this feature as is and then in C# 7.X or C# Y they will introduce a new way or a way to deal with scopes and then people will need to revise their code because they really wanted a narrow scope.

Now, I wish I had an elegant way to deal with it, I'd write a proposal for it but atm I don't. 😛

@aluanhaddad
Copy link

aluanhaddad commented Dec 3, 2016

Honestly if we had match expressions people wouldn't even want to use the pattern variable outside of the scope of the pattern. Since match is an expression and therefore has a value, any declaratively builtup state would be available for projection into the result of the case clause.

@alrz
Copy link
Contributor

alrz commented Dec 5, 2016

@gafter

I found a gem in #6182

The scope of any variable declared in a parenthesized-expression is the body of that parenthesized-expression.

I understand that is not being considered at this time, just curious how that would end up being.

var x = e is T t ? t : foo;
// t is in scope

var x = (e is T t ? t : foo);
// t is not in scope

wow.

Although, it would be a breaking change if done later, since currently parentheses don't maintain their own scope. Can parenthesized-expression enable optional narrow scoping for is var and out var?

@iam3yal
Copy link

iam3yal commented Dec 5, 2016

@alrz Nice! :D

Wait, will it allow us to control the scope of expressions inside if statements too?

@orthoxerox
Copy link
Contributor

@eyalsk

if ((e is T t)) {
    //t is not in scope here
}

😛

@alrz
Copy link
Contributor

alrz commented Dec 5, 2016

@orthoxerox Yeah that's unfortunate. But if we want #6182, it should be decided now, otherwise it'll be a breaking change and probably would be a real concern in sequence expressions' design.

@orthoxerox
Copy link
Contributor

@alrz I don't know, I think not leaking to any other scope is a good feature of sequence expressions. I think {if (e is T t) { ... }} is the only currently viable answer for scope control.

@iam3yal
Copy link

iam3yal commented Dec 5, 2016

@orthoxerox Yeah, currently but that's extremely ugly and odd that for one thing we use one approach and for the other we need to use another approach just to express the same thing, I'd also argue that there's a difference between the two, I mean one narrows the scope and the other creates a new scope.

@HaloFour
Copy link
Author

HaloFour commented Dec 5, 2016

@alrz

I found a gem in #6182.

I'm going to make the giant assumption that the scoping rules mentioned there are no longer on the table. Who knows if/how sequence expressions would work now, but I imagine simply wrapping an expression in parenthesis would not be sufficient in affecting scoping elsewhere.

@CyrusNajmabadi
Copy link
Member

@HaloFour Correct.

@DavidArno
Copy link

DavidArno commented Dec 5, 2016

@CyrusNajmabadi,

Is it also correct to assume that @qrli's proposal (#15651) of using {} instead of () to contain scope will not ever happen too?

Also, is it fair to assume that based on what you have been saying on #5561, that generators won't happen any time soon too as they are too difficult, or am I jumping to conclusions there?

@iam3yal
Copy link

iam3yal commented Dec 5, 2016

@CyrusNajmabadi Can you at least tell us how we might narrow the scope in the future? do you guys have some ideas? I hope {if (...) {}} isn't the direction, well, we can do that now but what I meant is I hope there will be a better way.

@CyrusNajmabadi
Copy link
Member

generators won't happen any time soon too

Define "any time soon" :)

It's certainly not happening for VS17 RTM. And it's large enough that i doubt it could make it into the first update for VS17. But after that it's certainly possible. But that's also far enough out that many many many things could happen.

@CyrusNajmabadi
Copy link
Member

Can you at least tell us how we might narrow the scope in the future?

First, i'd want to see how the C# 7 stuff is actually adopted in the wild. And if in the wild this is actually problematic and garners enough feedback to warrant addressing.

@CyrusNajmabadi
Copy link
Member

Is it also correct to assume that @qrli's proposal (#15651) of using {} instead of () to contain scope will not ever happen too?

There's certainly no effort to make that happen for C#7. For a future release, who knows? But, as mentioned above, we're definitely going to be observing how people are using C#7 and evaluating if such work is appropriate.

@DavidArno
Copy link

@CyrusNajmabadi,

When you talk of VS17, I take it you mean VS2017?

@CyrusNajmabadi
Copy link
Member

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Language Design Language-C# New Language Feature - Out Variable Declaration Out Variable Declaration New Language Feature - Pattern Matching Pattern Matching Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

No branches or pull requests