{"id":1568,"date":"2018-06-02T07:14:20","date_gmt":"2018-06-02T07:14:20","guid":{"rendered":"http:\/\/dlang.org\/blog\/?p=1568"},"modified":"2021-10-08T11:03:59","modified_gmt":"2021-10-08T11:03:59","slug":"driving-continuous-improvement-in-d","status":"publish","type":"post","link":"https:\/\/dlang.org\/blog\/2018\/06\/02\/driving-continuous-improvement-in-d\/","title":{"rendered":"Driving Continuous Improvement in D"},"content":{"rendered":"

Jack Stouffer is a member of the Phobos team and a contributor to dlang.org. You can check out more of his writing on his blog<\/a>.<\/em><\/p>\n

\n

\"\"In my previous article<\/a>, I went over the techniques we use in the D Standard Library (a.k.a Phobos) to develop a wide variety of testing mechanisms. I also briefly mentioned our style checker, dscanner<\/a>. In this article, I\u2019ll detail how we use dscanner to prevent style, documentation, and best practices regressions in the code, none of which can be covered by standard unit tests.<\/p>\n

Keeping a level of quality in software projects is a neverending battle. Bad practices and shortsighted design decisions make their way into code over time, whether from poor oversight, rushing things through, or simple code rot. There are three things we do in Phobos, other than tests, to fight this entropy.<\/p>\n

First, Pull Requests are required to be about one thing and one thing only. What counts as \u201cone thing\u201d is subjective, but, for example, a PR to fix a bug in a specific piece of code mustn\u2019t also edit the documentation of that function. This allows Phobos reviewers to merge the documentation change quickly if there\u2019s an issue in the bug fix preventing it from being merged (or vice versa). At the same time, it keeps the reviewers focused on one set of changes.<\/p>\n

Second, PRs need to be small; ideally less than 100 lines of code changed. If that\u2019s not possible, they need to be broken into multiple commits of smaller changes. This really helps reviewers keep D best practices in mind, while also fully understanding and internalizing the new code.<\/p>\n

Third, we continuously improve the existing code with dscanner, which, among other things, is D\u2019s official linting tool.<\/p>\n

What dscanner Can Do<\/h2>\n

As an example of the checks that dscanner can provide, let\u2019s take a look at some code that contains a very hard-to-spot bug. The following code creates a type that mimics an int<\/code>, but allows a null<\/code> state:<\/p>\n

struct NullableInt\r\n{\r\n    private int value;\r\n    bool isNull;\r\n\r\n    int get()\r\n    {\r\n        assert(!isNull, \"can't get a null\");\r\n        return value;\r\n    }\r\n\r\n    void nullify() { isNull = true; }\r\n\r\n    bool opEquals(T rhs) \/\/ D's equality overloading function\r\n    {\r\n        if (isNull && rhs.isNull)\r\n            return true;\r\n        if (isNull != rhs.isNull)\r\n            return false;\r\n        return value == rhs.value;\r\n    }\r\n}<\/pre>\n
The bug is not in the code that\u2019s there, it\u2019s in the code that\u2019s not there. All struct<\/code>s in D have default versions of the standard operator overloading functions if they aren\u2019t defined by the user. One of those functions provides a hash to represent the value to use in D\u2019s built-in associative arrays. The default version uses all the type fields to make the hash, which is a problem for NullableInt<\/code>, as we\u2019ve decided that all instances of the type that are null<\/code> are equal. Here\u2019s an illustration of the bug:<\/p>\n
void main()\r\n{\r\n    auto a = NullableInt(10, false);\r\n    auto b = NullableInt(10, false);\r\n    auto c = NullableInt(10, true);\r\n    auto d = NullableInt(0, true);\r\n\r\n    assert(a == b);\r\n    assert(b != c);\r\n    assert(c == d);\r\n\r\n    import core.internal.hash : hashOf; \/\/ D's internal hashing function\r\n    assert(c.hashOf != d.hashOf);\r\n}<\/pre>\n
dscanner will emit a message every time it finds a type that defines a custom opEquals<\/code>, but doesn\u2019t define a custom toHash<\/code> as well, alerting us to this bug.<\/p>\n
Continuous Improvement and \u201cRatcheting\u201d Quality<\/h2>\n
dscanner ties into continuous improvement, the philosophy that improvements to processes or designs should be made in a periodic, timely manner, rather than as one-time \u201cbreakthroughs\u201d. Such large breakthrough changes tend to be pushed back infinitely; in a phrase, it\u2019s letting the perfect be the enemy of the good. This easily fits with the continuous delivery or rapid release philosophies, and can vastly reduce bugs in software given enough time.<\/p>\n
By using the warnings from dscanner, we can ratchet Phobos\u2019s quality over time. If you\u2019ve never used a socket wrench, a ratchet is a mechanism that allows the user to freely move the wrench back and forth, but allows the bolt to spin only when the wrench moves clockwise. Similarly, we can move the quality of Phobos forward, while not letting it ever slip backwards, with dscanner. It works as follows:<\/p>\n
    \n
  • Run dscanner with every check but one turned off on all files.<\/li>\n
  • Populate a black list for that check in dscanner\u2019s configuration file containing each of the files that issued a warning.<\/li>\n
  • Repeat until the current code passes with all checks turned on with the black-list enabled.<\/li>\n<\/ul>\n
    Once we have our list-of-blacklists, new PRs will trigger warnings for issues detected by dscanner for files that weren\u2019t included in the blacklists, thereby keeping the status quo of quality.<\/p>\n
    Next, we ratchet quality by making a PR that fixes one issue in one file, and then removing that blacklist entry from the configuration. This way, that file will be checked in the future for every new PR. Over time, quality issues will be removed from Phobos, and they won\u2019t crop up again. For example, from the release of 2.076 to now, Phobos has gone from 624 public symbols without examples, to 328.<\/p>\n
    Currently, we\u2019re using this ratchet technique with dscanner to<\/p>\n
      \n
    • remove unused variables.<\/li>\n
    • add const<\/code> or immutable<\/code> to variables that aren\u2019t modified after their construction.<\/li>\n
    • make sure every public symbol is documented.<\/li>\n
    • make sure every symbol in Phobos that has documentation, has a code example in that documentation.<\/li>\n
    • force every assert to have an error message to print in case it fails.<\/li>\n
    • make sure only Exceptions<\/code>, and not Error<\/code>s, are caught in try\/catch<\/code> blocks.<\/li>\n<\/ul>\n
      among other things.<\/p>\n
      This process also has the added benefit of dogfooding dscanner, which helps us find bugs and know which features would be helpful to add from a user perspective. If you have a project that isn\u2019t using a linting tool as part of your test suite, it\u2019s only a matter of time before code rot creeps in.<\/p>\n","protected":false},"excerpt":{"rendered":"
      Keeping a level of quality in software projects is a neverending battle. Bad practices and shortsighted design decisions make their way into code over time, whether from poor oversight, rushing things through, or simple code rot. There are three things we do in Phobos, other than tests, to fight this entropy.<\/p>\n","protected":false},"author":13,"featured_media":0,"comment_status":"closed","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":[],"categories":[26,12],"tags":[],"_links":{"self":[{"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/posts\/1568"}],"collection":[{"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/users\/13"}],"replies":[{"embeddable":true,"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/comments?post=1568"}],"version-history":[{"count":5,"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/posts\/1568\/revisions"}],"predecessor-version":[{"id":1573,"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/posts\/1568\/revisions\/1573"}],"wp:attachment":[{"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/media?parent=1568"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/categories?post=1568"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/dlang.org\/blog\/wp-json\/wp\/v2\/tags?post=1568"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}