Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Empty value vs Option/Result vs panic #2512

Closed
medvednikov opened this issue Oct 23, 2019 · 45 comments
Closed

Empty value vs Option/Result vs panic #2512

medvednikov opened this issue Oct 23, 2019 · 45 comments

Comments

@medvednikov
Copy link
Member

medvednikov commented Oct 23, 2019

Hi,

This is a very important topic.

numbers[large_index]
os.ls('bad_path')
map[bad_key]
str.index('bad string')

All of these can result in errors, and there are 3 ways to handle them: return empty values, return a Result type with an error, or panic.

It's pretty standard across all languages to panic in out of bounds array access. The rest of these are not as clear.

Go returns zero values or -1, Rust uses Result, C++ panics. Each approach has its pros and cons.

Recently os.ls() []string was changed to os.ls ?[]string, and I'm not sure it was the right choice. Do we really need to handle this error? Does it matter whether there are 0 files in the directory, or the directory doesn't exist? In most cases people do something like

files := os.ls(path) or { panic(err) }
for file in files {

}

which works the same without an error check.

Making maps return optionals makes code like config[foo][bar] = true impossible.

What do you think?

@Trivaxy
Copy link
Contributor

Trivaxy commented Oct 23, 2019

I personally think that depending on what you are asking to be retrieved you should either return an optional or an empty value or panic.

numbers[large_index] should panic because otherwise the user might do ugly cleanup code for a bad index. Better to panic and encourage them to always ensure there is no way their index is wrong.

os.ls should return an empty value. It's a directory that doesn't exist, so technically the correct thing to do here is to return an empty directory, not panic or return an optional.
os.ls should return an optional describing the status of its directory as e.g non-existent or perhaps insufficient permissions to access.

map[bad_key] should also panic for the same reason as numbers[large_index] to help force the user to ensure that their keys are correct, probably better off having a function to check if a key exists or not.

str.index('bad_index') should panic for the same reason as the two above.

IMO, it's just a matter of logic behind what you should return, but I'm not sure if it'd be very consistent.

@avitkauskas
Copy link
Contributor

Yes, this is important and interesting question. It defines part of philosophy of the language, it's "character". We want V to be "safe language". But what does that really mean. We should define safety first and agree on the level of the safety we want, because everything has it's own cost.

For the mentioned cases, the most "safe" way, probably, would be to return Option in all cases and enforce developers to handle this. But then all the code becomes like

files := os.ls(path) or { panic(err) }

and this is not very nice. But you would be sure that you are handling all the possible undesirable outcomes and your program would not panic at run time if you handle situations in some better way.

A bit "less safe" mode would be to panic in every undesirable/wrong situation. This is still much better than silently producing undefined result. With panic, your program can fail at run-time, but if panic() gives you good information on what happend, this is easy to debug and fix. Can we consider this behaviour also "safe"?

What if we make all operations or functions that can fail on something returning Options, but we do not enforce developer explicitly handle this in the code, if he/she is fine the program to panic at run-time in these situations. If you are not fine with that, then please handle this explicitly in you code in a way that suits you.

Examples:

// panics if `index` is out of bounds
x := numbers[index]

// deal with that if you want
x := numbers[index] or default_value

// panic if no directory or no access
files := os.ls('bad_path')

// deal with these cases
files := os.ls('bad_path') or {
    match err {
        .not_exists { println('directory not found') }
        .no_access { println('cannot access directory') }
    }
}

// panic if map key does not exist
element := map[bad_key]

// deal with that if panic is not what you want
element := map[bad_key] or default_value

// on assigment, replace if exists, create new if does not exist
map[bad_key] = element
// created if did not exist

// default if not found
idx := str.index('bad string') or -1

Am I missing some important things here that would not work well for some reason?
I have a feeling that this could be safe enough, but also flexible for developers.

@avitkauskas
Copy link
Contributor

Some other languages go with the variants of

x := numbers[large_index]!

to force unwrap optionals when you are sure that it has a value, but I personally am not a fan of this.

@medvednikov
Copy link
Member Author

medvednikov commented Oct 23, 2019

I'm against Rust's unwrap() (Swift's !), there will be no unwrap for sure.

Typing or { panic(err) } does not result in a lot of extra keystrokes, and keeps this simple and clear.

@medvednikov
Copy link
Member Author

medvednikov commented Oct 23, 2019

// panic if no directory or no access
files := os.ls('bad_path')

// deal with these cases
files := os.ls('bad_path') or {

This goes against the one way philosophy. Go has a nice policy that no library should panic. I agree with that.

I think keeping the optional and adding the ? syntax sugar is the best option.

@spytheman
Copy link
Member

I think just adding the syntax for ? at the end (which is syntax sugar for or {panic(err)} of the optionals will be enough - it is easy to type and read imo.

@medvednikov
Copy link
Member Author

It's actually going to be error propagation for all functions except fn main.

In fn main (and V scripts) it will lead to a panic.

@spytheman
Copy link
Member

About the maps returning optionals, that is also ok imo.
I also think that setting a value in a nested map that is several levels deep, is not very well defined and has too many special cases to be easy to reason about.

@medvednikov
Copy link
Member Author

Yeah it's more of a Python/Perl programming style. Using nested maps for all objects.

@spytheman
Copy link
Member

It's actually going to be error propagation for all functions except fn main.

What will happen if the containing function does not return an optional ?

@spytheman
Copy link
Member

panic(err) ?

@medvednikov
Copy link
Member Author

Then using ? won't be an option. You'll have to use or { return } or or { panic(err) }

@spytheman
Copy link
Member

hm, but scripts (and main programs) do not return an option too...

@spytheman
Copy link
Member

that will lead to an inability to use ? exactly where it will be most useful, where brevity is important

@medvednikov
Copy link
Member Author

@spytheman

It's actually going to be error propagation for all functions except fn main.

In fn main (and V scripts) it will lead to a panic.

@spytheman
Copy link
Member

oh, so it will be special cased for main .. ok

@avitkauskas
Copy link
Contributor

avitkauskas commented Oct 23, 2019

Sorry if I miss some important point here, but how the syntactic sugar of

os.ls()? // ==> os.ls() or { panic() }

is different from the "syntactic sugar" of

os.ls() // panic if optional is unhandled

For me it looks that the only difference is that in the first place you say "yes, I know it returns optional and panic (or error propagation) is ok for me", and in the second you say "I don't care to handle optionals, please panic (or propagate error) if my optional does not have value somewhere".

Why V has to ask me to put ? at the end instead of handling all optionals with panic by default?

@shsr04
Copy link
Contributor

shsr04 commented Oct 23, 2019

I really like the ? idea. Instead of a 100% decision on "handle" or "panic", you can pass errors around as long as you need.

fn main() {
  f := find_files or {
    eprintln('Something went wrong when finding the files.')
    // :)
  }
}

fn find_files() []string? {
  ...
  ls := os.ls()?
  ...
}

@avitkauskas
Copy link
Contributor

avitkauskas commented Oct 23, 2019

With my proposal os.ls() would just panic if your return type in find_files() is []string and would propagate an error up (to main() in this case) if you make your function return type Optional. Why we need ? at all? And how would this be against the one way principal? It's always one way - all calls to functions returning optionals or other usages of optional always panic, unless you use them in a function that returns optional - in this case the error is returned from this function and is propagated up the call stack. No?

@lcddh
Copy link
Contributor

lcddh commented Oct 23, 2019

if “or {panic ()} everywhere” ,Looks and writes like a headache.

@shsr04
Copy link
Contributor

shsr04 commented Oct 23, 2019

os.ls()? // ==> os.ls() or { panic() }

is different from the "syntactic sugar" of

os.ls() // panic if optional is unhandled

For me it looks that the only difference is that in the first place you say "yes, I know it returns optional and panic (or error propagation) is ok for me", and in the second you say "I don't care to handle optionals, please panic (or propagate error) if my optional does not have value somewhere".

Well, what if you do care to handle optionals but forgot that ls returns an optional?

@lcddh
Copy link
Contributor

lcddh commented Oct 23, 2019

Well, what if you do care to handle optionals but forgot that ls returns an optional?
@shsr04 then check api

@avitkauskas
Copy link
Contributor

@shsr04 Valid question! That could be a reason to use ?. Agree on that.

@joe-conigliaro
Copy link
Member

joe-conigliaro commented Oct 23, 2019

For maps I'm thinking it should panic... you should always check the key exists before access, like array:
if !(key in map) {
It feels like optional is too heave use case for maps, hrmmm I dunno :)

@Delta456
Copy link
Member

Delta456 commented Oct 24, 2019

Or just have UB behavior for maps like arrays :P

But I definitely think that bad index and bad key should only panic and there shouldn't be optional for it.

@elimisteve
Copy link
Member

elimisteve commented Oct 24, 2019

Idea:

files := os.ls(path)?

remains short for

files := os.ls(path) or { return err }

but

files := os.ls(path)!

becomes short for

files := os.ls(path) or { panic(err) }

Making maps return optionals makes code like config[foo][bar] = true impossible. --@medvednikov

If users don't want

config[foo][bar] = true

to panic, they could do

config[foo]?[bar] = true

Or maybe the potential panic is made more explicit by disallowing

config[foo][bar] = true

altogether and forcing

config[foo]![bar] = true

Making the unsafe thing ugly like this might be a good idea, given that users should do something more like

config[foo] = {
    ...
    bar: true,
}

instead.

I see #2512 (comment) but I thought I'd try anyway :-), given that or { panic(err) } doesn't solve the config[foo][bar] = true scenario. Plus, I think that having an explicit ! is safer than having invisible potential panics around, and that these ! and ? conventions would show up so frequently that I suspect it's not a significant problem to force people to learn and use them.

Furthermore, making ! and ? distinct removes the need for the inconsistency of ? being shorthand for return err except in main, where it becomes shorthand for panic(err).

@elimisteve
Copy link
Member

I think the Golden Rule of Safety in V should be: no invisible panics.

I think everything @avitkauskas said in #2512 (comment) is pretty brilliant, especially

x := numbers[index] or default_value

and

idx := str.index('bad string') or -1

but with one exception: let's remove the invisible panics by disallowing

// panic if map key does not exist
element := map[bad_key]

and instead make people do

element := map[bad_key]!

if they're willing to allow the panic. Then users can watch out for ! in their code and know they're playing with fire/doing something unwise.

@joe-conigliaro
Copy link
Member

joe-conigliaro commented Oct 25, 2019

You cant have invisivble panic, its no longer a panic then

@avitkauskas
Copy link
Contributor

avitkauskas commented Oct 25, 2019

I think @elimisteve meant that in my initial proposal there was nothing in the code that would suggest a statement could panic. But with ! in the end we make it visible.

@joe-conigliaro
Copy link
Member

I see, what I mean is I dont think there should be any way to disable a panic

@elimisteve
Copy link
Member

@avitkauskas Exactly. I should have said: potential panics should be explicit in the code. No land mines allowed!

Possible exception: division by zero. Even there I suppose forcing the user to check that the denominator != 0 could be enforced. A generalized version of this idea is another way to approach all this that I wonder if Alex may like if it's feasible enough to implement: forcing users to ensure that, for example, a key exists in a map before it is used, where for key in my_map is sufficient to permit my_map[key] without trailing symbols nor the chance of a panic. Then v := my_map[key]! would rarely be needed.

@ntrel
Copy link
Contributor

ntrel commented Oct 25, 2019

@medvednikov

It's pretty standard across all languages to panic in out of bounds array access

So indexing an array should panic. In generic programming, operations like indexing should behave the same whatever the container. For consistency, indexing a map should also panic. This makes generic code work the same whatever container is passed to it.

Making maps return optionals makes code like config[foo][bar] = true impossible.

Exactly, we need to be able to assign to an indexing expression.

@elimisteve

config[foo]?[bar] = true

But how do we assign to an unwrapped optional value and have that update the map value? The optional would have to contain a reference to the mutable value. If doing this, it shouldn't be magic for maps, it must work for container methods that return an optional too.

Or maybe the potential panic is made more explicit by disallowing config[foo][bar] = true altogether and forcing config[foo]![bar] = true

If ! is required any time a (here implicit) function is called that may panic, that sounds good. The ! itself shouldn't be able to create a panic, it just indicates that the code to the left may panic. So config[foo] would not be an optional.

the inconsistency of ? being shorthand for return err except in main, where it becomes shorthand for panic(err)

I don't think we need that exceptional case - make the user write or {panic(err)} in main. If they don't want to keep writing that in main, move that code to another function returning an optional and use ?.

@elimisteve
Copy link
Member

elimisteve commented Oct 26, 2019

It's pretty standard across all languages to panic in out of bounds array access

So indexing an array should panic. ... For consistency, indexing a map should also panic.

But V is trying to be more safe, not merely as safe, as other languages; let us please not commit the same design errors of the past!

Making V Safe(r)

I have programmed in Go for 9 years now, and even though Go code is extremely safe compared to any scripting language or memory-unsafe language like C/C++, there are still some issues, namely with:

  1. (Lack of im)mutability
  2. Errors one forgets to check
  3. Errors that one remembers to check but forgets to do anything about
  4. Occasional unexpected nils

V doesn't a brilliant job of solving #1, #2, and #3, and partially solves #4.

One problem that almost all languages have that Go doesn't is exploding maps; that is, maps that throw exceptions when you try to look up a value with a non-existent key.

If maps throw exceptions in V, I think this would be a clear step backward.

When I write Python, I've trained myself to always do my_dict.get(key) rather than my_dict[key] so that shit doesn't explode and cause my program to crash unnecessarily.

Years ago I wrote a 4500-line API in Go that never crashed in production. On the test server, it did crash once, and I remember exactly why: I was accessing a nested map of stock quotes by symbol:

quotes := map[string]map[string]{...}
// It may have technically been a
quotes := map[string]*struct{Sym string, ...}{...}

I only checked to see if the outer map was nil, forgetting to check whether the inner map/pointer-to-struct was nil.

Bad:

if quotes != nil {

Good:

if quotes != nil && quotes[sym] != nil {

I believe that, in the case of V, it is very important to design this problem out of existence. This could be done in several ways, but if we don't even agree that it is important to protect V programs against this source of instability, then we will not try hard enough to solve this problem.

Part of the brilliance of Go, to paraphrase Rob Pike, is its built-in realization of the idea that errors are not exceptional. Trying to read a file that doesn't exist? Return an error rather than blowing up/throwing an exception/panicking, because that's not that weird, and destabilizing our programs doesn't help to solve it.

Purposely Destructive Considered Harmful

I once had a colleague tell me that it is a good idea for an unexpected error in our production servers to crash production for all of our users, because that would "get our attention" more than internally throwing an error that could loudly notify us without taking down production.

His suggestion is completely insane; allowing an edge case that affects 0.01% of requests to destroy 100% of the value obtained by users is a profoundly irrational trade-off.

I think that the "exceptions/panics are actually good" school of thought has a lot of takers, and that this is A Bad Thing that V need not perpetuate.

A Smarter, Safer Path

v := my_map[key]?

v := my_map[key] or ''

v := my_map[key] or {
    // Return, panic(err), whatever
}

I think these should be the 3 valid ways to access a map. All of them are either safe or clearly unsafe, none of them are unclearly unsafe, which is what must be avoided.

v := my_map[key]

If this innocuous-looking line can panic and thus unexpectedly kill your program, I think this would be a serious mistake that severely compromises the supposed safety guarantees of V.

@elimisteve
Copy link
Member

I'm thinking that my proposal to have

files := os.ls(path)!

be syntactic sugar for

files := os.ls(path) or { panic(err) }

may result in people constantly asking, "when should I use ? and when should I use !?" We could say, "? returns the error in the Optional, and ! panics; see these docs for details", and that may be good enough; they would learn quickly, and we could avoid or { panic(err) } everywhere, which could become the V equivalent of Go's if err != nil.

Or, maybe we have

v := my_map[key]!!!

become shorthand for

v := my_map[key] or { panic(err) }

so that it feels more different from and (correctly) looks more dangerous than ?

@medvednikov Maybe this is crazy, but I think that literally making nested maps invalid in V is a totally fine thing to do; make people use structs, so that

config[foo][bar] = true

would have to be

config.SomeFoo[bar] = true

or

config.SomeFoo.SomeBar = true

@elimisteve
Copy link
Member

elimisteve commented Oct 26, 2019

Making maps return optionals makes code like config[foo][bar] = true impossible.

@medvednikov @ntrel Good points regarding ^^that^^. maps could behave like they return an Optional, where the error part of the Optional becomes err = error('key not found') or err = error('cannot access uninitialized map'), depending on the problem faced. Then the user wouldn't need to manually define an Optional at all -- setting a map key's value to true is good enough -- just like users don't need to manually define an Optional within functions, they just return an error or a useful value.

@netmute
Copy link
Contributor

netmute commented Oct 26, 2019

Edit:
Disregard all that. A few thoughts on the original question.

Empty value
This is almost as bad as nil. That value will be passed around in your program, and bite you when you least expect it. It's a pain to debug.

panic
Goes against the V philosophy of "no unhandled runtime errors".

Option/Result
I really prefer this. It results in more verbose code, but the program behavior is obvious at all times.

@joe-conigliaro
Copy link
Member

joe-conigliaro commented Oct 26, 2019

I just realised we need to use optional or some default value, to avoid an extra map lookup we would need to do if it panics

@ntrel
Copy link
Contributor

ntrel commented Oct 26, 2019

v := my_map[key]
If this innocuous-looking line can panic

@elimisteve I actually suggested that if map indexing can panic, V should require a ! [at the call site] to indicate that.

My point was that indexing should behave the same regardless of container. If map indexing should return an optional, then so should array indexing, so that generic code doesn't have to handle arrays specially with compile time introspection.

@medvednikov
Copy link
Member Author

@netmute this discussion has been going on since the very beginning.

I think it's clear now that option/result should be used, and ? for easy propagation/panic in main should be implemented.

That gives us safety and a clean readable way to do things like os.ls()?

@elimisteve
Copy link
Member

@ntrel Awesome; I missed that you had suggested that and really like your reasoning 👍 🙂 .

@M4SSD35TRUCT10N
Copy link
Contributor

My 2 cents are; go with options/results like Rust did.
This way has proven to be one of the best ways to handle errors and forces developers to think about the outcome of their code. Yes, some things may look a bit weird but all the decisions made so far point into the direction tthat v should be easy to write (there is only on way to it), simple to use (the compiler do most of the stuff and there is only one way to do it) and safe (there is only one way to do it). So capture everything with option/results is simply exact that kind of way the language promises - there is only one way to do it and if you do it this way it's safe and performs as fast as it would when programmed with C (minus the-easy-to-shoot-off-your-loower-body-thing).

@ntrel
Copy link
Contributor

ntrel commented Oct 28, 2019

go with options/results like Rust did

Indexing a HashMap in rust actually panics:
https://doc.rust-lang.org/std/collections/struct.HashMap.html#impl-Index%3C%26%27_%20Q%3E
get returns an Option. (Just pointing this out, not arguing for unexpected panics).

@ntrel
Copy link
Contributor

ntrel commented Oct 28, 2019

it's safe and performs as fast as it would when programmed with C

With an array of integers, if indexing gives an Option then this has to be less efficient than C, because it needs (at least) another byte for the None state. (When the Option element type is a pointer, we can use the null address as an optimization).

@dumblob
Copy link
Contributor

dumblob commented Nov 4, 2019

I'm kind of confused - are you @avitkauskas and @medvednikov proposing removal of both panic() and recover() and implementing the behavior described in #2030 (comment) ?

With the linked approach there is no syntax/semantics conflict and you can "catch" and react to the error at any depth (this is the very initial motivation behind the concept of exceptions). All use cases should be covered including l-values handling:

// programmer says it's a valid state in her app, that key1 or key2 doesn't need to be present
if key1 in my_map {
  my_map[key1][key2] = ...
}

// programmer says it's an invalid state in her app, if key1 or key2 are not present
my_map[key1][key2] = ...

// programmer says I'm not sure, let's be safe
my_map[key1]or{my_map[key1]=map.new()}[key2] = ...

Let's leave the expressiveness up to the programmer as it's highly situational. Don't try to make rules which become obstacles by definition.

@shsr04: Well, what if you do care to handle optionals but forgot that ls returns an optional?

In case of the above solution it's easy - the code will not compile unless you either catch it with or { ... } or you propagate it (by adding ? to the return type in the function signature - i.e. making it visually explicit).

To distinguish errors coming from different function calls fn f() int? { might_error(); might_error2(); return 5 }; a = f() or { println( err ) } /* what is in err? */ the compiler will by default list all functions propagating indistinguishable errors (i.e. errors which originate from their body due to at least two unhandled optionals). But only those which will reach main().

There might be a compiler argument to suppress this verbosity for certain functions or modules or any other logical unit or the whole app if the time will prove, that people like to write code with too many optionals.

This approach also leaves a lot of room for potential future syntax extensions if the compiler argument for suppression won't be enough (which I hope will be enough if safety in V is really that important). E.g. implementing the proposal of others to additionally require ? to suppress error at the end of each call returning optional sounds good, but it feels a bit premature.

@medvednikov medvednikov unpinned this issue Nov 16, 2019
elimisteve referenced this issue May 18, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@trufae
Copy link
Contributor

trufae commented Mar 16, 2021

My opinion goes for having ! suffix as an alias for or {panic(err)}

@vlang vlang locked and limited conversation to collaborators Jul 22, 2022
@medvednikov medvednikov converted this issue into discussion #15180 Jul 22, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests