impl Responder for () makes common mistake silent #1108
Comments
|
I don’t have strong opinion on this topic. I’ve never had problems with Responder for () @actix/contributors opinion? |
|
What exactly was the reason that |
|
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) |
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 |
|
I think it makes sense to remove the blanket impl for |
|
|
|
@pythoneer added in #700 but the PR doesn't exactly state the rationale. 🤷♂️ Bit of a shame we didn't all bring our comments to the original PR, but better late than never. |
|
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 { 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 () { } Anyways, the reason I am sharing this detail is so you see how potentially damaging this can be to a newbie like myself. |
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 @actix/contributors |
|
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. |
|
too much drama |
|
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. |
|
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. |
|
I have a branch with this fixed, I just haven't looked it over to submit it. Sorry about that. ^^;; |
Rationale:
- In Rust, one can omit a semicolon after a function's final expression to make
its value the function's return value. It's common for people to include a
semicolon after the last expression by mistake - common enough that the Rust
compiler suggests removing the semicolon when there's a type mismatch between
the function's signature and body. By implementing Responder for (), Actix makes
this common mistake a silent error in handler functions.
- Functions returning an empty body should return HTTP status 204 ("No Content"),
so the current Responder impl for (), which returns status 200 ("OK"), is not
really what one wants anyway.
- It's not much of a burden to ask handlers to explicitly return
`HttpResponse::Ok()` if that is what they want; all the examples in the
documentation do this already.
Rationale:
- In Rust, one can omit a semicolon after a function's final expression to make
its value the function's return value. It's common for people to include a
semicolon after the last expression by mistake - common enough that the Rust
compiler suggests removing the semicolon when there's a type mismatch between
the function's signature and body. By implementing Responder for (), Actix makes
this common mistake a silent error in handler functions.
- Functions returning an empty body should return HTTP status 204 ("No Content"),
so the current Responder impl for (), which returns status 200 ("OK"), is not
really what one wants anyway.
- It's not much of a burden to ask handlers to explicitly return
`HttpResponse::Ok()` if that is what they want; all the examples in the
documentation do this already.
Rationale:
- In Rust, one can omit a semicolon after a function's final expression to make
its value the function's return value. It's common for people to include a
semicolon after the last expression by mistake - common enough that the Rust
compiler suggests removing the semicolon when there's a type mismatch between
the function's signature and body. By implementing Responder for (), Actix makes
this common mistake a silent error in handler functions.
- Functions returning an empty body should return HTTP status 204 ("No Content"),
so the current Responder impl for (), which returns status 200 ("OK"), is not
really what one wants anyway.
- It's not much of a burden to ask handlers to explicitly return
`HttpResponse::Ok()` if that is what they want; all the examples in the
documentation do this already.
Rationale:
- In Rust, one can omit a semicolon after a function's final expression to make
its value the function's return value. It's common for people to include a
semicolon after the last expression by mistake - common enough that the Rust
compiler suggests removing the semicolon when there's a type mismatch between
the function's signature and body. By implementing Responder for (), Actix makes
this common mistake a silent error in handler functions.
- Functions returning an empty body should return HTTP status 204 ("No Content"),
so the current Responder impl for (), which returns status 200 ("OK"), is not
really what one wants anyway.
- It's not much of a burden to ask handlers to explicitly return
`HttpResponse::Ok()` if that is what they want; all the examples in the
documentation do this already.
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
Responderfor(), 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.The text was updated successfully, but these errors were encountered: