Disable comments make static analysis tools worse
jfmengels.net
jfmengels.net
As an example, one clang-tidy rule we use is the explicit-constructor rule [1]. In C++, single argument constructors allow implicit conversions by default, which is almost never what you want since it hides type errors. This rule is very useful since it forces explicit constructors to be used.
However, in some (rare) cases you *do* want implicit constructors. In that case, you need a way of disabling this rule.
The author provides two alternatives to disable comments for this scenario:
* Globally disable the rule - Undesirable, now a useful rule must be removed because of 1-2 locations where it is not wanted
* Locally disable the rule using a global config file - Essentially the same as disable comments, but worse.
Disable comments are useful since they are localized in the code and hence more likely to remain up to date. They can contain explanations for why they are required in a specific location, and their inclusion is obvious which makes it easy to detect and check during a code review.
In an ideal world we would not need them, but sadly we do not live in an ideal world.
[1] https://clang.llvm.org/extra/clang-tidy/checks/google-explic...
Similarly, the language adds the [[fallthrough]] statement attribute to indicate intentional fallthrough between switch case labels.
As languages improve, we should need disable comments less and less.
So in general, improvements to languages don't necessarily remove the need for comments over time.
(On the other hand, more radical improvements might remove needs for warning comments by removing the original problem. In this case, in a language where constructors were explicit by default and you had to opt in to implicit, you wouldn't need the warning to exist in the first place.)
A warning that authors of new languages should account for. However most of us have millions of lines of working code, and experience in our current language. As such changing to your new better language isn't trivial. That is before we get into how old languages have had a lot of effort into tools and optimizations that new ones don't have [yet?] thus making the old one better in some potentially important dimensions.
- If the annotation is thought to be necessary because of a bug in the tool, add a link to the bug next to the disable comment. This makes it easy to get rid of the comment when being alerted that the bug is fixed.
- If you happen to disagree with the tool in a specific instance, and there's no way to override just that case, mention why in the commit message.
- Check for disable comments at peer review.
- It's not a bug in the tool (because the warning is often useful).
- A mention in a commit message doesn't help when you run the tool in future and get false alarms (oh I'll just read all the historic commit messages to see if any say these should be ignored).
- If you find such a comment at peer review then that doesn't give you anything actionable to do if the comment is being used correctly in one of these casees.
The solution in these cases is not "a little discipline" but simply to use the disabling comment and accept that this is reasonable.
Only if the total number of warnings across your entire codebase is tiny - 10 is the [arbitrary] number I like to use. That is 10 warnings, it doesn't matter if you are writing 5 lines of hello world or a 100 million lines of a self driving car. When the number of ignored warnings get above that number people start to ignore the tool and then it becomes useless.
With 10000 warnings that means that people have gotten in the habit of disabling warnings, so how many of them are real. I've tracked production bugs to someone disabling a warning - we regularly disabled that warning for correct reasons, but in the one case it wasn't a false warning.
Thus your disabled warning levels need to be small. Small is somewhat arbitrary, but it needs to be small enough that when the tool says something is wrong you trust it.
E.g., am I suppressing raw-types warning because I couldn't be arsed getting the type signatures right? Or because some library we're using only accepts a `List<Future>` when every other usage of `Future` in the method is a `Future<T>`, so you need to use a raw `List` at some point?
I try to suppress as few warnings as possible, so only suppress warnings that are due to third party limitations like the above, or because I think that the warning is, in this instance, not helpful if obeyed.
But once again, I'll always comment to explain this _why_
Great until something goes terrible wrong and some absolute critical bug has to be fixed when you are on-call alone on a weekend.
Never disallow escape hatches. You may not need both your arms in your everyday job or in your life in the ideal case, but keep cool and wait going to the doctor for an amputation just in case.
Or when you have four team members, but one is on vacation and another called in sick today.
The value is in making it possible to get to “lint clean” quickly as a first pass, while marking the parts of the code that are tricky to refactor to conform with the style guidelines.
Once the code is lint-clean, you can start reviewing the disable comments and coming up with a more permanent fix.
Are they good? bad? Yes. It depends on your circumstances and priorities. Saying this doesn't make a very satisfactory article though.
Rust has four levels for lints: allow, warn, deny and forbid. You can nest these to override the level inside an item, except for forbid, which means “deny, and don’t allow any overriding”. This is most commonly seen with #[forbid(unsafe_code)].
See https://doc.rust-lang.org/rustc/lints/levels.html for more info.
Disable comments are useful but must be reviewed carefully. It would be good if code review tools like Gitlab could highlight certain regexps to flag them in code reviews. Often there is a simple solution and the comment will be removed in the code review. Other times, a ticket should be logged to address the problem later (in a refactor, for example).
Off the top of my head, some times when these comments are useful:
* You're doing a hotfix and don't have time to do the necessary refactor to fix the problem,
* You're certain that a subsequent refactor will remove the current code anyway,
* The code is legacy and fixing the problem now would cause much bigger changes,
* You just know you're right. For example, there are a few legitimate reasons for doing `import *` in Python, even if the QA tools can't see that.
You need disable comments, but they need to be very fine grained by default.
I'm thinking of several very relevant cases for disable comments. Like your first example of the camelCase issue is very real to me.
We are served JavaScript from a third party that we have little to no control over, I want to type that JS with Typescript. So I created some .d.ts files. But they use snake_case for their function names.
When I then type those functions, I get complains from my linter that it's not camel case. This is where I need my escape hatch.
I'll have to use it anyway so this way at least I can be more sure when calling the API what I need to pass or not.
I mean all of that applies to any library you ever import. You still have control over it in the means of commit hashes etc, so it won't randomly break. So it makes sense documenting the API for yourself if there is no documentation.
Now you can wrap the library if you want so that only the wrapper as to use the bad API. There are pros and cons to this.
And then there are rules that target i.e. portability and warn you that the code may easily break on change, although currently it is not broken.
If it’s an internal CRUD webapp then who cares if a few pedantic lint rules (max length line anyone?) are disabled.
If it’s a safety critical application written in c that controls an airplane avionics then all static analysis rules should be followed to the letter.
But all static rules should be carefully reviewed as well. Those who wrote those C rules (probably MIRSA, but there are other sets of rules) have a long list of considerations and whys. Many of the rules of exceptions built in so that they can avoid the problems while allowing the useful case. (Ie no memory allocation EXCEPT at startup)
It turns out I didn't really need them and that I had not understood the lints correctly.
I recommend anyone else who is also skeptical to try to remove some of the disable comments -- you might be surprised.
Imo, this misses an important use case for warnings: reporting issues that I don't want in committed code, but that are fine while developing. For example, dead code — I don't want to commit a function that's never called, but I do want to be able to compile/run my code and run any tools over it when I've written a new function but not yet called it.
I myself personally tend to run elm-review when I'm ready to make a PR most of the times.
I certainly wouldn't mind if rules could only be disabled in context (or globally) and potentially even with a mandatory explanation though.
Enforcing code style is not nonsense when automated. It ensures consistency and it helps preventing things like changes in white space without any semantic change.
Manual checking of the above is silly indeed. Review comments should be about the code and intent. Not the syntax.
I think so many engineers have been burned by bad code that we now instinctively latch onto anything that appears to quantitatively improve the code, but then get so focused on those metrics that the real fundamental problems are ignored.
So we end up running around in circles pleasing CI instead of spending that time thinking more about the architecture and patterns that make code bad.
Code quality theatre.
These are the kind of things that are best solved with an automatic code formatter like Prettier or gofmt instead of a linter. This is one of things that Go got right: gofmt eliminates squabbling over code quality issues and is fast enough that you can run it every time you save a file and not notice the overhead.
(Also remember that time the webkit repo got hosed by someone committing the sha collision as a test case?)
https://www.typescriptlang.org/docs/handbook/release-notes/t...