Actix-web: impl Responder for () makes common mistake silent

[jimblandy]

This is a tentative suggestion, since I'm not too familiar with the Actix API, but:

I think it may be a mistake to implement Responder for (), because that means that a stray semicolon at the end of a responder's code is a silent error. Even when caught in testing, it can take a while to notice the source of the problem.

Rust's "omit a semicolon to return the value" rule would be intolerable for programmers - when I explain it to people, they always look skeptical - except that the type checker always catches the mistake. By permitting () as a return type, Actix has removed that safety net.

Without that impl, people would have to return HttpResponse::Ok(), which doesn't seem like much of a burden. All the examples in the documentation do this already.

[fafhrd91]

I don’t have strong opinion on this topic. I’ve never had problems with Responder for ()

@actix/contributors opinion?

[pythoneer]

What exactly was the reason that Responder was implemented for () in the first place?

[robjtede]

It does make sense for endpoints where actions are performed and, if no errors are returned, result in 200 and empty response, implicitly.

But it’s a compelling case. An explicit requirement to return something can only be a net win in my opinion.

(Obviously this is would be a breaking change)

[onelson]

It does make sense for endpoints where actions are performed and, if no errors are returned, result in 200 and empty response, implicitly.

This is an interesting example to use since I'd almost always be responding with a 204 status if I had an empty body, which would mean never just using () as my return value from my handler.

[cetra3]

I think it makes sense to remove the blanket impl for () as it is not explicit enough when trying to troubleshoot what's happening.

[fafhrd91]

200 definitely is wrong status code, should be 204. () responder could be use in Result<(), Error> form, it might be more useful. But i am fine with removing ().

[onelson]

@pythoneer added in #700 but the PR doesn't exactly state the rationale. :man_shrugging:

Bit of a shame we didn't all bring our comments to the original PR, but better late than never.

[daveb68]

I am very new to both Rust and Actix. But I was also shocked that a language with such a strong compiler would allow the following to compile and then silently fail.

fn index2() -> impl Responder {
HttpResponse::Ok().body("Hello world again!");
}

Of course I spotted the issue reasonably quickly in this case. But this scared me that Rust would have such a glaring hole in it. Had I wasted all of my time pouring over "The book"? Of course I went and experimented with my own struct and traits and the compiler let me know that this would not work unless I had implemented this for the unit type, (). From here it was a pretty quick find to find the following.

impl Responder for () {
type Error = Error;
type Future = FutureResult;

fn respond_to(self, _: &HttpRequest) -> Self::Future {
    ok(Response::build(StatusCode::OK).finish())
}

}

Anyways, the reason I am sharing this detail is so you see how potentially damaging this can be to a newbie like myself.
I am cutting my teeth with Rust by creating a web server and inspecting the Actix crates to see how things are done.

[pythoneer]

@daveb68

But this scared me that Rust would have such a glaring hole in it.

There is no glaring hole in Rust at this place. Its exactly all happening as it is implemented. You could argue that the implementation of Responder for () is confusing. If you really want to see glaring holes in Rust here you go but i think its unhelpful to confuse things here.

@actix/contributors
But on that note i am glad that this has come up again, thanks. As people noted here there is a good reason to make the jump to Actix 2.x and this could be a good opportunity for the removal of Responder for () as a breaking change.

[cetra3]

Yep, it's a breaking change so would need to bump up to version 2.0, which is tentatively blocked on tokio async/await AFAIK.

I think we can wait until then before resolving this paper cut.

[fafhrd91]

too much drama

[daveb68]

@pythoneer

If you read my full post, you will see I had eventually realized that the hole was not with Rust, but was with the API choice, which while convenient, is error prone IMO. But thank you for the link to the real current problems with Rust.

I believe that the language Rust and the library Actix-Web have goals and objectives that are owned by two different groups and I am still learning them both. I lack any practical experience whatsoever right now and can only map experience from other languages and libraries onto what I have seen so far.

My post was to present to @fafhrd91 how I was initially confused and then concerned about any accidental semicolons I leave in my code or logic holes that the compiler will no longer inform me about. If my desires of libraries that prioritize error free code and clarity over convenience and reduction in every possible piece of code do not match @fafhrd91, then I simply have to respect that. And for all I know, there may be a very good and practical reason that outweighs my desire. As of this writing, I lack the experience to know.

On your other note about futures, I almost went with hyper directly or the tower stack for the direct async/.await support. But the numbers adopting actix as well as the frequency of updates (at least compared to tower) seemed to dictate actix_web as the best place to start.

[robjtede]

Seems to me that @daveb68's points about beginner-use point a likely outcome; that going forward this will continue to be a confusion for newcomers and a footgun for others.

I am willing to PR this removal but it may be worth discussing how v2 will be handled first.

[jimblandy]

I have a branch with this fixed, I just haven't looked it over to submit it. Sorry about that. ^^;;