> This is the multi-million dollar .unwrap() story.

That's too semantic IMHO. The failure mode was "enforced invariant stopped being true". If they'd written explicit code to fail the request when that happened, the end result would have been exactly the same.

▲

AdieuToLogic an hour ago | parent | next [-]

>> This is the multi-million dollar .unwrap() story.

> That's too semantic IMHO. The failure mode was "enforced invariant stopped being true". If they'd written explicit code to fail the request when that happened, the end result would have been exactly the same.

Problem is, the enclosing function (`fetch_features`) returns a `Result`, so the `unwrap` on line #82 only serves as a shortcut a developer took due to assuming `features.append_with_names` would never fail. Instead, the routine likely should have worked within `Result`.

▲

jcalvinowens an hour ago | parent [-]

> Instead, the routine likely should have worked within `Result`.

But it's a fatal error. It doesn't matter whether it's implicit or explicit, the result is the same.

Maybe you're saying "it's better to be explicit", as a broad generalization I don't disagree with that.

But that has nothing to do with the actual bug here, which was that the invariant failed. How they choose to implement checking and failing the invariant in the semantics of the chosen language is irrelevant.

	▲	20 minutes ago \| parent [-]
		[deleted]

▲

navidmafi an hour ago | parent | prev | next [-]

And dare I say, an exhibition of hindsight bias.

▲

chrisweekly 2 hours ago | parent | prev | next [-]

semantic? or pedantic?

	▲	an hour ago \| parent [-]
		[deleted]

▲

echelon 2 hours ago | parent | prev [-]

[flagged]

▲

dap an hour ago | parent | next [-]

I'm not sure if this is serious or not, but to take it at face value: the value of this sort of thing in Rust is not that it prevents crashes altogether but rather that it prevents _implicit_ failures. It forces a programmer to make the explicit choice of whether to crash.

There's lots of useful code where `unwrap()` makes sense. On my team, we first try to avoid it (and there are many patterns where you can do this). But when you can't, we leave a comment explaining why it's safe.

▲

echelon an hour ago | parent [-]

The language semantics do not surface `unwrap` usage or make any guarantees. It should be limited to use in `unsafe` blocks.

> There's lots of useful code where `unwrap()` makes sense. On my team, we first try to avoid it (and there are many patterns where you can do this). But when you can't, we leave a comment explaining why it's safe.

I would prefer the boiler plate of a match / if-else / if let, etc. to call attention to it. If you absolutely must explicitly panic. Or better - just return an error Result.

It doesn't matter how smart your engineers are. A bad unwrap can sneak in through refactors, changing business logic, changing preconditions, new data, etc.

▲

aw1621107 an hour ago | parent | next [-]

> It should be limited to use in `unsafe` blocks.

That would be a fairly significant expansion of what `unsafe` means in Rust, to put it lightly. Not to mention that I think doing so would not really accomplish anything; marking unwrap() `unsafe` would not "surface `unwrap` usage" or "make any guarantees", as it's perfectly fine for safe functions to contain `unsafe` blocks with zero indication of such in the function signature and.

▲

echelon an hour ago | parent [-]

> fairly significant expansion of what `unsafe` means in Rust

I want an expansion of panic free behavior. We'll never get all the way there due to allocations etc., but this is the class of error the language is intended to fix.

This turned into a null pointer, which is exactly what Rust is supposed to quench.

I'll go as far as saying I would like to statically guarantee none of my dependencies use the unwrap() methods. We should be able to design libraries that provably avoid panics to the greatest extent possible.

Unwrap is an easy loss on a technicality.

	▲	aw1621107 an hour ago \| parent [-]
		> I want an expansion of panic free behavior. Sure, and I'd hardly be one to disagree that a first-party method to guarantee no panics would be nice, but marking unwrap() `unsafe` is definitely not an effective way to go about it. > but this is the class of error the language is intended to fix. Is it? I certainly don't see any memory safety problems here. > This turned into a null pointer, which is exactly what Rust is supposed to quench. There's some subtlety here - Rust is intended to eliminate UB due to null pointer dereferences. I don't think Rust was ever intended to eliminate panics. A panic may still be undesirable in some circumstances, but a panic is not the same thing as unrestricted UB. > We should be able to design libraries that provably avoid panics to the greatest extent possible. Yes, this would be nice indeed. But again, marking unwrap() `unsafe` is not an effective way to do so. dtolnay's no_panic is the best we have right now IIRC, and there are some prover-style tools in an experimental stage which can accomplish something similar. I don't think either of those are polished enough for first-party adoption, though.

▲

bigstrat2003 an hour ago | parent | prev [-]

Restricting unwrap to unsafe blocks adds negative value to the language. It won't prevent unwrap mistakes (people who play fast and loose with it today will just switch to "foo = unsafe { bar.unwrap() };" instead). And it'll muddy the purpose of unsafe by adding in a use that has nothing to do with memory safety. It's not a good idea.

	▲	echelon an hour ago \| parent [-]
		> And it'll muddy the purpose of unsafe by adding in a use that has nothing to do with memory safety. Then we need more safety semantics around panic behavior. A panic label or annotation that infects every call. Moreover, I want a way of statically guaranteeing none of my dependencies do this.

▲

abigailphoebe an hour ago | parent | prev [-]

blaming the language is not the way to approach this. if an engineer writes bad code that’s the engineers fault, not the languages.

this was bad code that should have never hit production, it is not a rust language issue.

▲

echelon an hour ago | parent | next [-]

No. Don't say "you're holding it wrong". The language says "safe" on the tin. It advertises safety. This shouldn't be possible.

This is a null pointer. In Rust.

Unwrap needs to die. We should all fight to remove it.

	▲	aw1621107 an hour ago \| parent \| next [-]
		> The language says "safe" on the tin. It advertises safety. Rust advertises memory safety (and other closely related things, like no UB, data race safety, etc.). I don't think it's made any promises about hard guarantees of other kinds of safety.
	▲	dafelst an hour ago \| parent \| prev [-]
		panics are safe, what are you talking about? It is nothing like a null pointer.

▲

anonnon an hour ago | parent | prev [-]

> blaming the language is not the way to approach this. if an engineer writes bad code that’s the engineers fault, not the languages.

A Rust zealot unironically typed this. I actually thought:

> the utter lack of self-awareness; this must be a troll

But doing some googling, it appears they're a TRUE and HONEST transgendered/nonbinary ("they/them pronouns"), anime avatar-using Rust zealot:

https://nl.linkedin.com/in/abigailphoebe