Nice article with a lot of depth. While disable comments are something to pay close attention to in a code review, I disagree that their inclusion makes static analysis tools worse. In fact, I would argue not including them makes static analysis tools worse.

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...

Aye - on my current (Java) project, my general rule of thumb is if I'm suppressing a compiler/IDE warning or codestyle rule, I comment as to why.

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_

A little discipline will go a long way here:

- 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.

Isn't it useful for the tool to warn about something that is often incorrect but sometimes intentional? The parent's example of implicit constructors is a good one, and a sibling comment mentioned fallthrough as another example. None of your suggestions help with that:

- 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.

> Isn't it useful for the tool to warn about something that is often incorrect but sometimes intentional?

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.

Your comment drives the discussion in a rather circular fashion: your objection is addressed by code comments that selectively disable warnings at specific locations in code, which is the subject of the original article. So long as you make careful use of those then it doesn't matter if you have 10,000 of them in your 100 million lines of code; you still only get one (visible) warning if you make a change that triggers it again, which is below your 10 warnings threshold.

You need to regularly audit all your warnings with ever tool upgrade to make sure they still apply. Auditing 10000 issues is too hard, so people will soon not be careful. When there are 10 you can check each quickly enough and ensure they are still valid.

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.

At that point, why not just make them errors? Making them warnings just makes it easier to put off the decision about whether to disable the rule for a specific case.

Since C++20, you can write `explicit(false)` to indicate that a single argument constructor is intentionally implicit.

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.

While explicit(false) does seem like a good way to indicate that, it seems like it's just a happy accident that it exists. The intended purpose of explicit(xyz) is that xyz is a compile-time constant that depends on the template parameters of the current (template) class. A literal true or false isn't needed by the language (just include explicit or don't), but the language would be more complex if they were disallowed so it's easier to allow them.

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.)

> 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.

Agree completely. Whilst it’s true you can use disable comments to make code worse, removing them is a technological solution to a social problem. Static analysis helps with implementing coding standards and good practice, but you still need to actually have the users care.

Like setting up rules enforcing pull requests to merge to production branch and having all PR needed to be reviewed by at least 2 ppl with no escape mechanism in place..

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.

Depends on your domain. I'm in embedded systems, with some safety critical parts. It costs millions of dollars to upgrade production after the release is done - we are not interested in your weekend fix, it better be right in the first place, or it is millions more to deploy the next update. That we are safety critical means that we wouldn't release anyway as we don't dare risk your change resulting in some worse safety issue than the one you are trying to fix. The minimum time for a critical fix to make it into production is 6 weeks of tests. (Yes we have extensive automated tests, they still don't catch everything - as anyone use uses the popular web platforms that release to production often has figured out)

> Great until something goes terrible wrong

Or when you have four team members, but one is on vacation and another called in sick today.

And the flipside is: don't abuse escape hatches.

I do support the forbidding of "disable all" comments as the author discusses, but you must have an escape hatch. If your tool doesn't have one, I won't use the tool.

Disable comments are most useful when you’re changing the way a codebase is linted: adding new rules, setting up lint on a legacy codebase, etc.

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.

I get the feeling that a lot of people think building small things starting from scratch is the only way anything is ever built.

Are they good? bad? Yes. It depends on your circumstances and priorities. Saying this doesn't make a very satisfactory article though.

> As far as I know, there is no static analysis tool out there that has disable comments and allows making some rules not disableable (please let me know if you know one!).

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.

In my experience, you do not want to force your developers to fight with QC tools. If they consider it a battle, they will win and your software will lose. You want them to consider them as tools to help them improve their work.

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.

People will find other ways around it, maybe they'll just grep your error output and return a zero return code if it's in there instead.

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.

No, you need tools that recognize that those are third party and so should not be analyzed. You identified there is a missing feature in your tools, but not that right missing feature. This mistake is all too easy to make, and the results are costly when you get the wrong feature.

Why is the interface to a third party library not to be analyzed?

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.

Because you can't safely change it and also take the next upstream release. Either you live with whatever interface they give you, or you don't use it at all. Any change you make will be lost when they release an update with something else you need (perhaps a security fix).

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.

Sometimes static analysis tools have bugs and misunderstand what's happening in otherwise perfectly fine code. I think that's ultimately what disable comments are intended for.

Yes, but not only. You can think of the rules as classifiers with non zero false positive rates. They were extracted by going backwards from errors that were made often. There may be some cases where you want to break the rule for a reason. Then you better document why, because that this reason still holds must ideally be reviewed with any relevant change.

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.

The big missing element of this discussion is the context of what kind of application are we static analyzing.

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.

> 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)

Yes, you're correct of course - choose the right static analysis tools and rulesets for your project. But then follow them strictly if it's safety critical.

Disable comments are my form of political protest. If you take them away then i will only find a significantly more obnoxious end to my mean and i will optimize for wasting your time

Static analysis tools can really be very useful, and they can detect potential errors. But there are always boundary cases. Some rules simply state "Don't do this", which might be a wise rule in general but frequently there are cases where the developer knows the general rule but decides that it does not apply in this particular case.

I was very skeptical after reading this article last night but I checked some hobby code I am writing to see if I really needed the 7 golangci disable comments.

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.

> elm-review doesn’t have severity levels, meaning that all enabled rules are enforced. Yes, no warnings

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.

Just to clarify: Since elm-review is a static analysis tool separate from the compiler, you can run and compile your program just fine without having to handle all the dead code warnings (which elm-review is actually very good at detecting https://jfmengels.net/safe-dead-code-removal/).

I myself personally tend to run elm-review when I'm ready to make a PR most of the times.

In his example about console.log he explicitly mentions that it never happened again... Which makes sense to me, because even with the ability to disable the rule, you can't accidentally forget it. Sure, if a programmer is careless, it might still slip through because they disabled it, but with such a careless programmer you'll have bigger issues.

I certainly wouldn't mind if rules could only be disabled in context (or globally) and potentially even with a mandatory explanation though.

Disable comments are the only sensible way to start using static analysis in an existing codebase.

IME "thou shalt not introduce a new violation" tests are an even better way to do that if you can set it up. The article mentions this as a strategy and links to a tool called betterer that tries to do so generically.

I have found an overwhelming amount of "static analysis" (really linting) serves only to pile more stupid busy work onto developers and soak up CI cycles. Who actually (and I mean ACTUALLY) cares if my imports are not in alphabetical order? Who cares (I mean REALLY) if I have ( spaces ) between my brackets and their contents (or not). Stop wasting my time with this nonsense.

> Who actually (and I mean ACTUALLY) cares if my imports are not in alphabetical order? Who cares (I mean REALLY) if I have ( spaces ) between my brackets and their contents (or not).

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.

Or just not solved at all because they're not problems.

This is why your project should be set up so these kind of checks can be performed easily and quickly locally. We auto format on compile for example.

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.

The biggest reason to use an auto formatter is take all those nits off the table for gripes during code review. I think the consistency claim is overrated. Seeing ){ vs ) { doesn't help me read a codebase better, not does see un-alphabetized imports. The biggest culprit of whitespace only changes is somebody running an auto-formatter on a file where that isn't enforced by CI.

While I think there’s value in tooling that tidies these things up automatically, I agree that enforcing CI rules around these is a waste of time.

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.

Yeah tools like linters (or strict compiler warning rules) can cause a lot of distraction. Sometimes the rules are dictated, with no possibility for appeal. Sometimes people don’t dare to to be that guy who suggests to dial down on quality. It can be counterproductive if an important bug is hidden in a tsunami of warnings. Best to turn on warnings one by one, and check if the effect is as intended.

Completely agree. Ctrl-f works whether your imports are sorted or not. I'm of the opinion that unless a codebase supports using an auto-formatter (as in, if I run the autoformatter on save to any file I touch, it's guaranteed to only adjust my changes), these kind of rules have no business being part of a the code style rules.

You can have these checks run before CI.

one fun issue is what to do with test cases. I know of at least one project that uses precommit hooks to test with eslint (or something), etc but has tests that are explicitly checking for things that eslint disallows. Tests like that end up requiring disable comments by design.

(Also remember that time the webkit repo got hosed by someone committing the sha collision as a test case?)

There are always reasons to use the escape hatch. For example, I recently needed to cast to a known type and access a property, but didn't want to reference that type directly, so as to retain the ability to tree shake it. This requires asserting that the property exists, and turning off eslint for that line.

You could use a type only import for the type, gets taken out completely when compiled, so no tree shaking issue.

https://www.typescriptlang.org/docs/handbook/release-notes/t...

The section "FORBID CONSTRUCTS THAT LEAD TO FALSE RESULTS" perfectly summarizes the Elm community: "my tool is smarter than you, so it will tell you how to bend your code so that it understands it"

This advice was for custom rules for a team, so it's about enforcing what the team has already agreed upon and understood the trade-offs, not anything the tool decided upon on its own and imposes on the team.