chore: add tfhe dylint#601
Conversation
Consolidated Tests Results 2026-05-22 - 12:26:37Test ResultsDetails
test-reporter: Run #2368
🎉 All tests passed!TestsView All Tests
🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
7f152f9 to
7e53efa
Compare
7e53efa to
1fc14a6
Compare
dvdplm
left a comment
There was a problem hiding this comment.
I am a little skeptical to introducing dylint on a broader scale. I am worried about the maintenance complexity and I think that the dylint we do have has not been very useful to us. We have ended up disabling it in a lot of places. I really like the idea of custom lints, but the actual implementation seems stuck on nightlies forever. Is it one of those things that seem great on paper but ends up being more of a burden in practice?
Setting my own bias aside for a second I think the code here is fine.
Questions:
- I prefer explicit
cargocommands in our CI configs over Makefile. This is just my bias but I think it is good to be consistent and explicit. Why is a make command better? - Does having two sources for dylints mean we have to compile two separate toolchains? Both locally and in CI?
- There's a mention of a "warning-only" initial roll-out. Is that correct? I think warnings are just going to be ignored and that if we do this, we should go all-in.
I think it has been useful and it's catching a few things with the tfhe-rs ones. It's also not a big maintenance burden at the moment as we're not maintaining our own lints, so tfhe-rs needs to make sure the lints are working with whatever toolchain they decide to use. While some are addressed in this PR, I made an issue here https://github.com/zama-ai/kms-internal/issues/3032 for the other stuff we need to look at.
For me, when the cargo command needs a lot of flags or environment variables, it's better to put it in a script or makefile I think. Then it can be reused without copying a long command in many places
Yes, I think two toolchains are used.
Changed it to go all out. The commend might be wrong because I've seen more than one CI failures due to dylint. |
dvdplm
left a comment
There was a problem hiding this comment.
I am still somewhat skeptical to this but the only way we can know if the hassle outweighs the benefit is trying it out. Code is fine, no objections there.
Let's try it!
Description of changes
Integrate tfhe lints to our repo. At the moment I've disabled the
serialize_without_versionizelint because we have many types that are serialized but not versioned, a vast majority of them are false positives, but we should investigate more in future work - https://github.com/zama-ai/kms-internal/issues/3032. Also I renamed XYZVersioned to XYZVersions to ensureinvalid_versionize_dispatchpasses. One final thing is I removed the linting pass for kind-testing, since we're already doing it in the normal CI pipeline.Issue ticket number and link
Closes zama-ai/kms-internal#3027
PR Checklist
I attest that all checked items are satisfied. Any deviation is clearly justified above.
chore: ...).TODO(#issue).unwrap/expect/paniconly in tests or for invariant bugs (documented if present).devopslabel + infra notified + infra-team reviewer assigned.!and affected teams notified.Zeroize+ZeroizeOnDropimplemented.unsafe; if unavoidable: minimal, justified, documented, and test/fuzz covered.Dependency Update Questionnaire (only if deps changed or added)
Answer in the
Cargo.tomlnext to the dependency (or here if updating):More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md