search

Nadrieril / dhall-rust

Maintainable configuration files, for Rust users
Other
303 stars 27 forks source link

Simplify SimpleType::from_nir for UnionType #213

Closed scottmcm closed 3 years ago

scottmcm commented 3 years ago

Spotted in crater using a prototype of a has-breaking-changes version of https://github.com/rust-lang/rfcs/pull/3058

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

scottmcm commented 3 years ago

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

steffahn commented 3 years ago

How about using flat_map instead?

How about using and_then instead?

Nadrieril commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Should be fixed in https://github.com/Nadrieril/dhall-rust/commit/846c14f92bda2fb3e68c3debf940414628013574 . This should no longer cause a break with the new Try trait

steffahn commented 3 years ago

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 commented 3 years ago

👍 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 commented 3 years ago

Thanks for spotting that, that was indeed a confusing mix