typecheck logs disabled by filter #974
Conversation
de-vri-es
force-pushed
the
typecheck-disabled-logs
branch
2 times, most recently
from
c3da86d to
58ae56e
Compare
Without this change, the body of log statements is completely removed
from the code if the log statement is disabled by the `DEFMT_LOG` env
variable. This means that even invalid log statements will compile.
This change ensures that the code logging code is still visible to the
compiler, but inside an `if false { ... }` block, so it can easily be
thrown away by the compiler after type checking.
de-vri-es
force-pushed
the
typecheck-disabled-logs
branch
from
58ae56e to
8beb74a
Compare
|
CI is now picking up an error in the |
|
Added a UI test to verify that disabled logs are type checked, and run UI tests twice: once with |
|
Let me know if you think this needs more tests regarding binary size or compile times! |
|
This is very interesting! I hope to find time to look at it soon. |
|
Polite ping :) |
There was a problem hiding this comment.
My initial impression is that this makes sense. Checking things even when the logs are disabled means the code stays correct, and that's always good.
I'm very glad you tested binary size impact, thank you. I believe @thejpster and I discussed spending a bit of time checking this a tad more before a merge.
This change doesn't appear to break any code that was not already subtly broken. I'm not sure how this interacts with the stability promises of https://ferrous-systems.com/blog/defmt-1-0-released/. Technically, making this change would only reveal already existing bugs in tested with some logs disabled, so we need to ask ourselves if our stability promise gives us permission to break code that already would break, provided the right build time arguments...
|
Perhaps one path forward is to merge this, plan the next release, and do a blog post telling everyone what we're about to release. Then leave it some period of time for anyone to object, and we can then do the release with people having been warned. |
|
Circling back, I did some testing with https://github.com/ferrous-systems/rust-exercises/tree/main/nrf52-code/radio-app and noted that the binary sizes did not change, debug or release, or because of |
|
Thanks for testing! <3 |
Absent any other ideas, let's do this. |
|
Thank you for this PR! |
This PR makes sure that disabled log statements are still typechecked.
Essentially, it just substitutes
falsefor the log filter instead of outputting different code for build-time disabled logs.I've done some quick verification in a sample project to ensure that the code is still completely removed from the binary: the size of the binary code did not change with or without this commit applied, tested both with the log statements enabled and disabled.
It will almost certainly increase compile times a little for projects with many normally disabled logs, but I believe the trade-off of type-checking disabled logs is worth it.