Simplify SimpleType::from_nir for UnionType

[scottmcm]

Spotted in crater using a prototype of a has-breaking-changes version of rust-lang/rfcs#3058

TBD whether this change will ever be required, but I think it's a good change even if not.

[scottmcm]

Looks like everything's green except for a clippy failure that's not related to this change.

[steffahn]

How about using flat_map instead?

How about using and_then instead?

[Nadrieril]

Interesting! Is this because the new Try won't allow conversion from Option to Result<_, NoneType>?
I don't think this is a correct transformation however. If from_nir returns None I want to abort because there was a type error. I realize I should have used an actual error type for that. I'll try that now.

[Nadrieril]

The fact that tests passed anyways is because serde_dhall is nowhere near as well-tested as dhall, since it's mostly a facade. Not great though

[Nadrieril]

Should be fixed in 846c14f . This should no longer cause a break with the new Try trait

[steffahn]

FYI, this is not about the new Try trait actually introducing any breaking changes. Some limited ways to convert option into result with ? were accidentally introduced and there are measures in place that it stays this way with the new try_trait version. The mentioned crater run was done with a version not including these measures and doesn't represent any actual problems arising from the code that's being “fixed” by this PR in the future. Nonetheless, conversion of option into result with ? is using an accidentally stabilized feature, so it could count as being quite unidiomatic.

[scottmcm]

👍 to the change to use an error type.

The main thing here was this:

.map(|v| Ok(Self::from_nir(v)?))

This isn't the only codebase where we've seen that, but it's a subtle mixing of ?-on-Option-in-a-Result-method that wasn't ever supposed to be stabilized, and only works because of inference being cleverer than was realized in the previous Try RFC.

So we're looking to see if we can just remove it (perhaps over an edition). So far it's looking promising, since the error messages would be better and the cases I've found so far have been clearer without the mixing -- either using Result like here, or using Option all the way through like in https://github.com/rust-analyzer/rust-analyzer/pull/7735/files

[Nadrieril]

Thanks for spotting that, that was indeed a confusing mix