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: new builtin to pick from one of two values #36303

Closed
mdevan opened this issue Dec 29, 2019 · 14 comments
Closed

proposal: new builtin to pick from one of two values #36303

mdevan opened this issue Dec 29, 2019 · 14 comments
Labels
Milestone

Comments

@mdevan
Copy link

mdevan commented Dec 29, 2019

Consider a builtin called pick that can pick from one of two given values of the same type, depending on a boolean expression:

// before
var v string
if expr {
    v = "value1"
} else {
    v = "value2"
}

// after
v := pick(expr, "value1", "value2")

Yes, it is yet another ternary operator proposal :-), but this time suggested as a builtin similar to append or delete.

The signature of pick would look like:

func pick(cond bool, lhs, rhs Type) Type

where Type is any(?) type and the compiler would enforce that lhs, rhs and the return value are of the same type - similar to how the 2nd and later arguments of append and the return value are of the same type.

Only one of the arguments would be evaluated depending on the result of the boolean expression, similar to an if-else statement.

As for evidence that this can simplify code, here are some examples, all taken from the single Go source code file fmt/format.go:

lines 76-80:

// before
	// Decide which byte the padding should be filled with.
	padByte := byte(' ')
	if f.zero {
		padByte = byte('0')
	}

// after
    padByte := byte(pick(f.zero, '0', ' '))

lines 124-132:

// before
func (f *fmt) fmtBoolean(v bool) {
	if v {
		f.padString("true")
	} else {
		f.padString("false")
	}
}

// after
func (f *fmt) fmtBoolean(v bool) {
    f.padString(pick(v, "true", "false"))
}

lines 370-374:

// before
	length := len(b)
	if b == nil {
		// No byte slice present. Assume string s should be encoded.
		length = len(s)
	}

// after
    length := pick(b == nil, len(s), len(b))

lines 419-423:

// before
		if b != nil {
			c = b[i] // Take a byte from the input byte slice.
		} else {
			c = s[i] // Take a byte from the input string.
		}

// after
    c = pick(b != nil, b[i], s[i])

lines 454-458, also lines 481-485:

// before
	if f.plus {
		f.pad(strconv.AppendQuoteToASCII(buf, s))
	} else {
		f.pad(strconv.AppendQuote(buf, s))
	}

// after
    f.pad(pick(f.plus, strconv.AppendQuoteToASCII(buf, s), strconv.AppendQuote(buf, s))

lines 464-467, also lines 476-479:

// before
	r := rune(c)
	if c > utf8.MaxRune {
		r = utf8.RuneError
	}

// after
    r := pick(c > utf8.MaxRune, utf8.RuneError, rune(c))

lines 526-531:

// before
			digits = prec
			// If no precision is set explicitly use a precision of 6.
			if digits == -1 {
				digits = 6
			}

// after
    digits = pick(digits == -1, 6, prec)
@gopherbot gopherbot added this to the Proposal milestone Dec 29, 2019
@smasher164 smasher164 added v2 A language change or incompatible library change LanguageChange labels Dec 29, 2019
@mvdan
Copy link
Member

mvdan commented Dec 29, 2019

I worry that this would hurt readability. If one sees pick(cond, foo, bar), it's not immediately obvious which value you'll end up when cond == true. You could read the arguments like ifTrue, ifFalse, but you could also read them like ifFalse, ifTrue or zeroValue, nonZeroValue.

The only way I can see this being obvious to the reader is with a better name (which, if longer, probably defeats the purpose of the addition), or with added syntax, which I believe has been rejected in the past.

I personally don't think this is common and painful enough to warrant adding complexity to the language or builtin package. This is just my opinion, though.

Only one of the arguments would be evaluated depending on the result of the boolean expression, similar to an if-else statement.

I strongly disagree with this part. Function calls always evaluate all of their arguments before the call takes place, and that's as specified by the language. pick would be just a builtin function, so breaking this rule would be very unintuitive.

Edit: to quote the spec:

Except for one special case, arguments must be single-valued expressions assignable to the parameter types of F and are evaluated before the function is called.

The special case is when one does F(g()) and g returns multiple values, so it's not related to the evaluation order of parameters.

@alanfo
Copy link

alanfo commented Dec 29, 2019

This is a similar idea to the iif (or iff) statement found in many dialects of the BASIC programming language. It has also been suggested before for Go using cond as the built-in name.

Personally, I'm strongly in favor of it as I'm fed up of writing five or six lines of code when one will do. I even prefer it to the ternary operator found in many C family languages (C, C++, Java, C# etc) both on basic readability grounds and because I think it makes it less likely that it will be heavily nested as one often finds in the latter languages.

However, having said that and despite the good examples you've dug out, I think the chance of it being adopted is virtually nil. The Go team have consistently opposed any form of ternary operator and the last time this was seriously discussed (in #33171) there were more people against it than for it.

You can, of course, do something like this already using interface{} for the type. However, it's not type safe, there is no short circuit evaluation and you have to cast the result to the correct type so it's hardly 'nice'.

If and when we get generics, it'll be possible to write something like this yourself. There'll still be no short-circuit evaluation but it'll be fine where the arguments are cheap to evaluate which would account for the majority of use cases.

@RodionGork
Copy link

RodionGork commented Dec 29, 2019

@mdevan thanks for taking efforts and gathering these examples! I suspect it is still hopeless, but it would be good to keep trying :)

However I'd prefer conditional operator to look like operator. It is similar to what @mvdan says - proposed pick looks too much like function and it is misguiding that one of arguments really is not evaluated (which will lead to different behavior in case any of them have side-effects).

So perhaps some other syntax could be used? E.g.

c = on b != nil pick b[i] or s[i]

though this looks a bit ugly especially without highlighting even for me. anyway it is not as ugly as dedicated if-else with variable

@ianlancetaylor
Copy link
Contributor

Seems pretty similar to #31659 (comment).

@mdevan
Copy link
Author

mdevan commented Dec 30, 2019

About lazy evaluation

It's just what you're used to. Do you find anything "wrong" in this code?

if len(a) > 0 && a[0] == 42 {
    // ..
}

Chances are you understood the intention of the author perfectly well, and so did the compiler. However, it was only because you've been relying on and using short-circuit evaluation for god knows how long now.

By the same logic as in https://golang.org/doc/faq#Does_Go_have_a_ternary_form, it can be argued that this form:

if len(a) > 0 {
    if a[0] == 42 {
        //..
    }
}

although longer, is unquestionably clearer.

@mvdan
Copy link
Member

mvdan commented Dec 30, 2019

It's just what you're used to.

This is precisely what I mean. All Go developers are used to function calls evaluating all arguments. If you suddenly break that rule for special cases that look exactly the same, my guess would be that this will cause pain while people readjust and learn the new rules.

I'm not saying it's not possible. My point is that I don't think it's worth it, and proposals like this one should generally be clear net wins solving an important problem.

I also agree with @alanfo and people in previous threads. Given that generics are a real possibility, why not just wait for that? You can re-open this proposal if generics end up getting rejected.

Edit: also remember that changing the language spec to alter function argument evaluation rules would break a lot of tools out there, such as linters, in subtle ways. It seems to me like the bar for such subtle changes to the spec should be high.

@deanveloper
Copy link

This proposal isn't technically possible with generics. This is because only one of the LHS/RHS is evaluated, however with generics, both sides will be evaluated since it would be a normal function call.

This also brings in the issue of the confusion of something like pick(cond, onTrue(), onFalse()). pick looks like an ordinary function call, so one may think that both onTrue() and onFalse() would be evaluated.

The "generic function" solution is inconsistent with the original ?: ternary operator, however the "builtin function" solution is inconsistent with the idea that function calls evaluate all of their arguments.

@ianlancetaylor
Copy link
Contributor

As several people said above, in Go the function call syntax suggests that all arguments are evaluated before the function call is made. It would be unusual to change that for a single function, even one that is built into the language.

Also, there is no strong support for this proposal based on the emoji voting.

For these reasons, this seems like a likely decline. Leaving open for four weeks for final comments.

@Fryuni
Copy link

Fryuni commented Feb 7, 2020

The use case that I feel makes me miss this the most is when defining multiple fields in a struct. For example, when configuring a database connection:

type DbConfig struct {
    CACertificate *tls.Certificate
    ClientCertificate *tls.Certificate
    user string
    password string
}

How it could be configured with pick (same with cond as it was proposed in some other issues):

func getConfig(mtlsEnabled, externalCa bool) DbConfig {
    return DbConfig{
        user: os.Getenv("DB_USER"),
        password: os.Getenv("DB_PASSWORD"),
        CACertificate: pick(
            externalCa,
            external.GetCACertificate(),
            local.GetCACertificate(),
        ),
        ClientCertificate: pick(mtlsEnabled, credentials.GetClientCertificate(), nil),
    }
}

How it could be configured with the ternary operator:

func getConfig(mtlsEnabled, externalCa bool) DbConfig {
    return DbConfig{
        user: os.Getenv("DB_USER"),
        password: os.Getenv("DB_PASSWORD"),
        CACertificate: externalCa ?
            external.GetCACertificate():
            local.GetCACertificate(),
        ClientCertificate: mtlsEnabled ? credentials.GetClientCertificate(): nil,
    }
}

One way to do it today:

func getConfig(mtlsEnabled, externalCa bool) DbConfig {
    config := DbConfig{
        user: os.Getenv("DB_USER"),
        password: os.Getenv("DB_PASSWORD"),
    }
    if externalCa {
        config.CACertificate = external.GetCACertificate()
    } else {
        config.CACertificate = local.GetCACertificate()
    }
    if mtlsEnabled {
        config.ClientCertificate = credentials.GetClientCertificate()
    }
    return config
}

To me this option is the worst one and the hardest to maintain. It can grow to become a huge function with lots of ifs setting fields of a variable defined dozens or even a hundred lines before.
For every conditional field you'll need 5 more lines of code (3 if one of the options is the zero value).

For a perspective, if you have a redis cluster with go-redis and did a fine tuning of its 22 options for multiple environments, you'll add up to 110 lines of ifs to your code (plus blank lines in a desperate attempt to make it somewhat readable).


Another way to do it today:

func getConfig(mtlsEnabled, externalCa bool) DbConfig {
    return DbConfig{
        user: os.Getenv("DB_USER"),
        password: os.Getenv("DB_PASSWORD"),
        CACertificate: getCACertificate(externalCa),
        ClientCertificate: getClientCertificate(mtlsEnabled),
    }
}

func getCACertificate(externalCa bool) *tls.Certificate {
    if externalCa {
        return external.GetCACertificate()
    }
    return local.GetCACertificate()
}

func getClientCertificate(mtlsEnabled bool) *tls.Certificate {
    if mtlsEnabled {
        return credentials.GetClientCertificate()
    }
    return nil
}

This option is better then the previous one in terms of readability, but the logic and rules to return one single configuration struct is scattered in multiple functions.
Each one of those functions require 6 more lines of code (excluding blank lines around them), so from the same example with the redis cluster you would add 22 new functions and 132 new lines of code to your codebase.
Those functions would be available inside your package and would appear in the autocomplete all the time, an excellent bait for newcomers of the project to misuse.

pick/cond can take up to 4 new lines and the ternary operator up to 2, both keep the condition and the choices right inside the struct definition. If the condition and options are short, they can all fit in one line, adding no new lines at all to your code, keeping it clean and with all the logic close well defined in a single place.

Now, regarding whether it should be an operator or a special function with lazy evaluation, I agree with @RodionGork that an operator would be better. All the logical operators have short-circuit and behave similar to this case of lazy evaluation, we use logical operators to combine conditions, it sounds reasonable to me that it should be a conditional operator not a function.

Edit: Fixed the examples

@earthboundkid
Copy link
Contributor

I think the operator ?? should be added to mean "short-circuit evaluate until value is not the blank value", just as && short-circuits booleans.

If Go gets generics, you could trivially write first(ts ...T) T, so the operator is only worth having if it adds in short-circuit evaluation.

As for the FAQ, it says

The if-else form, although longer, is unquestionably clearer.

Ternary is notoriously unclear, but ?? would not be any more unclear than &&. I believe it would be more clear than an if-statement.

A language needs only one conditional control flow construct.

There is already another form of control flow in &&. This would just be making && work for non-bool types.

If there's interest, I can break this into a separate issue. I looked to see if anyone else had proposed ?? and this was the closest I could find.

@ianlancetaylor
Copy link
Contributor

I don't know what ?? means (Go doesn't have "blank values"). But it doesn't sound this this proposal at all, which is about a builtin with three arguments that evaluates to the value of one of the arguments. The ?? idea should be a different proposal.

@earthboundkid
Copy link
Contributor

earthboundkid commented Feb 11, 2020

Sorry, I meant “zero value” rather than “blank”.

I will start a new issue. See #37165

@mdevan
Copy link
Author

mdevan commented Feb 14, 2020

The main objections to the proposal so far are:

  • poor readability
  • unexpected behavior in that it looks like a function but has short-circuit evaluation

I further propose a modification, backed by a grammar change. It still is a ternary operator in disguise but hopefully more readable and telegraphs short-circuit evaluation too.

Best illustrated by examples:

// before
	r := rune(c)
	if c > utf8.MaxRune {
		r = utf8.RuneError
	}

// after
    r := pick utf8.RuneError if c > utf8.MaxRune else rune(c)
// or
    r := pick rune(c) if c <= utf8.MaxRune else utf8.RuneError
// before
			digits = prec
			// If no precision is set explicitly use a precision of 6.
			if digits == -1 {
				digits = 6
			}

// after
    digits = pick 6 if digits == -1 else prec
// example from a comment above
func getConfig(mtlsEnabled, externalCa bool) DbConfig {
    return DbConfig{
        user: os.Getenv("DB_USER"),
        password: os.Getenv("DB_PASSWORD"),
        CACertificate: (pick external if externalCa else local).GetCACertificate(),
        ClientCertificate: pick credentials.GetClientCertificate() if mtlsEnabled else nil,
    }
}

Formally, the proposal is to add a new grammar rule:

expr := PICK expr IF expr ELSE expr

where PICK is a new token, and IF and ELSE are existing ones.

Semantically:

  • the IF expr has a boolean type, and the PICK/ELSE/result exprs have the same type
  • the IF expr is evaluated first, if the value is true:
    • then the PICK expr is evaluated and used as the result expr
    • else the ELSE expr is evaluated and used as the result expr

@ianlancetaylor
Copy link
Contributor

No change in consensus, so closing.

The pick proposal seems like an entirely different proposal. Let's do that in a separate issue if someone wants to pursue it.

@golang golang locked and limited conversation to collaborators Feb 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

10 participants