Writing linter rules:

why, how and when

Jeroen Engels

@jfmengels

Jeroen Engels

@jfmengels everywhere

jfmengels.net

Elm Radio podcast

elm-review

Aim: provide instinct

 

Not a tutorial

Elm

Disclaimer

Examples are going to be in JavaScript / TypeScript / Elm

Linter

Guarantees

function shout(sentence) {
    return sentence.toUpperCase();
}

shout(100);
// Uncaught TypeError: sentence.toUpperCase is not a function
function shout(sentence) {
    return sentence.toUpperCase();
}

shout(100);
function shout(sentence: string) {
    return sentence.toUpperCase();
}

shout(100);
// Argument of type 'number' is not assignable to parameter of type 'string'.

Type error?

Static type checker!

Type checking:

Comparing values

and sets of values

function printAverage(scores: number[]) {
    console.log("Your average score:", average(scores));
}

function average(array: number[]): number {
    return sum(array) / array.length;
}

function sum(array: number[]): number {
    // Correct implementation of a sum
}

printAverage([]);
function printAverage(scores: number[]) {
    console.log("Your average score:", average(scores));
}

function average(array: number[]): number {
    return sum(array) / array.length;
}

function sum(array: number[]): number {
    // Correct implementation of a sum
}

printAverage([]);
// "Your average score: NaN"
function average(array: number[]): number {
    return sum(array) / array.length;
}

printAverage([]);
// "Your average score: NaN"

Can we use types?

test('average() returns 0 when the array is empty', () => {
    assert.equals(average([]), 0);
});

function average(array: number[]): number {
    if (array.length === 0) {
        return 0;
    }
    return sum(array) / array.length;
}

Tests: checking

specific values in specific scenarios

array average(array)
[ ] 0
[ 1, 2 ] 3
... ... 
import Colors from './colors';

const buttons =
    [
        button({ color: Colors.primary }, "First button"),
        button({ color: "#FF1122" }, "Second button"),
    ];
const buttons =
    [
        button({ color: Colors.primary }, "First button"),
        button({ color: "#FF1122" }, "Second button"),
    ];

test("The second button's color is primary", () => {
    assert.equals(buttons[1].getColor(), Colors.primary);
});
<no-input> color buttons[0] color buttons[1] ...
Colors.primary Colors.primary ...

Triangle

button({ color: "#FF1122"      }, "Second button"),
         ^^^^^^^^

 

using grep

grep -e 'color: "' src/**/*.ts

Building a linter

#!/bin/bash

MATCH=$(grep -e 'color: "' src/**/*.ts --with-filename --line-number --color=always)
if [ -n "$MATCH" ]; then
    echo "Found linting problems:"
    echo ""
    echo $MATCH
    exit 1
fi

Building a linter

using grep

// 2 spaces
button({ color:  "#FF1122" }, "..."),

// No spaces
button({ color:"#FF1122" }, "..."),

// Space before the colon
button({ color : "#FF1122" }, "..."),

// Uses single-quotes
button({ color: '#FF1122' }, '...'),

False negatives

grep -e 'color: "' src/**/*.ts
/*
Don't ever write 

    button({ color: "#FF1122" }, "..."),

Instead use

    button({ color: Colors.primary }, "..."),
*/

False positives

Abstract Syntax Trees

(a + b) / 2
Integer 2
Binary expression
    using "+"
Reference to "a"
Binary expression
    using "/"
Reference to "b"
Parenthesized

Pattern matching on the AST

a / 0
Integer 0
Reference to "a"
Binary expression
    using "/"

Rule: No division by 0

If I see a binary expression using "/" where on the right side there's an integer 0 , then report a problem.

Pattern matching on the AST

{ color: "#FF1122" }
String literal
Field "color"
Property

What's the problem?

Communication

And what's the problem with hardcoded colors anyway?

What should I do instead?

People need to understand

  • What they did wrong

  • Why it is a problem

  • How to move forward

Not understanding reports

leads to workarounds

🤬

const buttons =
    [
        button({ color: Colors.primary }, "First button"),
        // linter-disable
        button({ color: "#FF1122" }, "Second button"),
    ];

Context

Problem

Suggestion

Example

Applications

Enforce using custom solutions instead of built-in ones

TODO Example HTTP library that sets all the right headers

A lot of the code reviews comment that people say that are not too complex but that people just have to know, can probably be automated using linter rules

Imposing limits

TODO Making sure that one module does not import another one. That one does not import internals of another module.

TODO console.log(context) example?

Deprecate code

TODO Forbid the usage of a v1 of something and promote a v2 instead

Automate documentation

Gain new guarantees

TODO Safe unsafe operations

Generate code

TODO Html decoder

Enforcing code style

Consistency is good

Code formatters

module MyModule exposing (Model, Msg, update, view)

import Html exposing (Html, button, div, text)
import Html.Events exposing (onClick)


type alias Model =
    { count : Int }


type Msg
    = Increment
    | Decrement


update : Msg -> Model -> Model
update msg model =
    case msg of
        Increment ->
            { model | count = model.count + 1 }

        Decrement ->
            { model | count = model.count - 1 }


view : Model -> Html Msg
view model =
    div []
        [ button [ onClick Increment ] [ text "+1" ]
        , div [] [ text <| String.fromInt model.count ]
        , button [ onClick Decrement ] [ text "-1" ]
        ]

Not all rules are good

Concert with colleagues

Should you publish your rule?

🤬

Linter

//nolint:rule
# pylint: disable=rule
# rubocop:disable rule
// NOLINT
// phpcs:disable rule
@SuppressWarnings("rule")
@Suppress("rule")
// eslint-disable rule

You don't trust your linter

elm-review

elm-review.com

False positives

Communication
Automatic fixes

Configuration

Ignoring reports

False positives

when the tool reports issues that it shouldn't have

when the tool reports issues that it shouldn't have

🤬

🫶

// linter-disable
// linter-disable
// linter-disable

How should I fix this issue?

Should I ignore this report?

Causes of false positives

Bugs

Missing information

Presumptions

False positives/negatives

(when the tool doesn't report an issue that it should have reported)

Providing information

  • Contents of all files in the project

  • Type information

  • Dependencies

  • Files from different languages

How easy is it to analyze the language?

Compiled languages

Statically typed languages 

Explicit patterns

Dynamic constructs

https://www.youtube.com/watch?v=_rzoyBq4hJ0

Effective false positives

true positives that are considered to be false positives

Automatic fixes

Prompt for fixes

Configuring linters

Accidental configuration

Essential configuration

Accidental configuration

  • What language features are available/enabled?

  • What tools does this project work with? Build tools, macros...

False positives

because of the configuration

Tell the tool how to work with your project because it can't figure some information on its own

Accidental configuration

All necessary information

in standard files

// elm.json
{
    "type": "application",
    "source-directories": [
        "src"
    ],
    "elm-version": "0.19.1",
    "dependencies": {
        "...": "..."
    }
}

Essential configuration

Tell the tool what you want from it

 

 

Which rules do you want to enable?

Configuring elm-review

config : List Rule
config =
    [ NoBadThing.rule
    , NoOtherBadThing.rule { some = "options" }
    -- ...and more rules
    ]

Enable rules

you agree with

false positive ≈ error you disagree with

Advice on adding a rule

Be ready to disable the rule if it feels painful

Understand the problem well

Get your team's approval

Understand not all rules are good

Ignoring reports

//nolint:rule
# pylint: disable=rule
# rubocop:disable rule
// NOLINT
// phpcs:disable rule
@SuppressWarnings("rule")
@Suppress("rule")
// eslint-disable rule

Disable comments

Warnings

For rules you don't want to enforce...?

Errors

For rules you want to enforce

Line 10: Don't do this thing
Line 11: Don't do this thing
Line 12: Don't do this thing
Line 13: Don't do this thing
Line 14: Don't do this thing
Line 15: Don't do this thing
Line 16: Don't do this thing
Line 17: Don't do this thing
Line 18: Don't do this thing
Line 19: Don't do this thing
Line 20: Don't do this thing
Line 21: Don't do this thing
Line 22: Don't do this thing
Line 23: Don't do this thing
Line 24: Don't do this thing
Line 25: Don't do this thing
Line 26: Don't do this thing
Line 27: Don't do this thing
Line 28: Don't do this thing
Line 29: Don't do this thing
Line 30: Don't do this thing
Line 31: Don't do this thing
Line 32: Don't do this thing
Line 33: Don't do this thing
Line 34: Don't do this thing
Line 35: Don't do this thing
Line 36: Don't do this thing
Line 37: Don't do this thing
Line 38: Don't do this thing
Line 39: Don't do this thing

Line 23: Don't do this thing
Line 38: Don't do this thing

Line 16: Don't do this thing

Line 23: Don't do this thing
Line 38: Don't do this thing

Advice on how to use

// linter-disable
this.code.will.crash();

😨

Not available in elm-review

Warnings

-- elm-review-disable rule

❌

❌

😱

Rules with exceptions

Higher quality rules

Don't write  the rule

Disable comments and warnings

enable bad quality rules

Rule doesn't make sense somewhere

config : List Rule
config =
    [ NoUnused.Variables.rule
    , NoDebug.Log.rule
    ]
        |> Rule.ignoreErrorsForDirectories [ "tests/" ]

Allowing existing errors

gradually adopt a rule

Deprecating a function

{-| Does something.

@deprecated Use someBetterFunction which does it better.

-}
someFunction input =
    -- do something with input

Deprecating a function

//nolint:rule
# pylint: disable=rule
# rubocop:disable rule
// NOLINT
// phpcs:disable rule
@SuppressWarnings("rule")
@Suppress("rule")
// eslint-disable rule
#![deny(clippy::all)]

Line 10: Don't do this thing
Line 11: Don't do this thing
Line 12: Don't do this thing
Line 13: Don't do this thing
Line 14: Don't do this thing
Line 15: Don't do this thing
Line 16: Don't do this thing
Line 17: Don't do this thing
Line 18: Don't do this thing
Line 19: Don't do this thing
Line 20: Don't do this thing
Line 21: Don't do this thing
Line 22: Don't do this thing
Line 23: Don't do this thing
Line 24: Don't do this thing
Line 25: Don't do this thing
Line 26: Don't do this thing
Line 27: Don't do this thing
Line 28: Don't do this thing
Line 29: Don't do this thing
Line 30: Don't do this thing
Line 31: Don't do this thing
Line 32: Don't do this thing
Line 33: Don't do this thing
Line 34: Don't do this thing
Line 35: Don't do this thing
Line 36: Don't do this thing
Line 37: Don't do this thing
Line 38: Don't do this thing
Line 39: Don't do this thing

Suppressing errors

{
  "version": 1,
  "automatically created by": "elm-review suppress",
  "learn more": "elm-review suppress --help",
  "suppressions": [
    { "count": 7, "filePath": "src/Api.elm" }
  ]
}

review/suppressed/NoDeprecated.json

$ elm-review suppress

No new errors

Introducing a new error

Fixing a suppressed error

To summarize...

False positives and

effective false positives

lead to losing trust

Remove false reports

by providing more information

Communication

  • What is the problem?

  • Why is it a problem?

  • How to solve the problem?

Avoid accidental configuration

as much as possible

Only enable rules

you agree with

Better alternatives than

disable comments and warnings

to ensure rules are enforced

Tools can only gain trust by

correctly and transparently

doing their tasks

over and over again

Please open issues

to improve your linter

Thank you!

Jeroen Engels

@jfmengels

Learn more:

Slides: https://slides.com/jfmengels/why-you-dont-trust-your-linter

Linter has a lot of rules

Takes time to configure

Use someone else's configuration

Rules you don't agree with

🤬

🫶

// linter-disable
// linter-disable
// linter-disable

How should I fix this issue?

Should I ignore this report?

🤬

🫶

// linter-disable
// linter-disable
// linter-disable

Ignoring per file

config : List Rule
config =
    [ NoDeprecated.rule
        |> Rule.ignoreErrorsForFiles
            [ "src/Api.elm"
            , -- ...and other files
            ]
    ]

except here

New issues will be reported

Elm is knowable

No dynamic function calls

No side-effects

No mutations

No macros

...

No memory management

No race conditions

...

Elm is knowable

Better understanding of the code

(Almost) no false positives

Generated/vendored code

Simpler and knowable

languages make for

better analysis

Deceitful linters

False negatives

You don't trust your linter

firm belief in the reliability, truth, or ability of someone or something.

Trust

firm belief in the reliability, truth, or ability of someone or something.

Fix Don'ts

Never provide fixes that lead to compiler errors

 

Don't provide a fix if the user needs to do a follow up

Automatic fixes

🤬

code style

TODO Give example of code fix where style becomes not like what you wished

Automatic fixes

❤️

code formatters

Linting

Linting?

Unnecessary ESLint rules for Elm

92% (56/61) of the recommended rules

87% (228/263) of all the rules

People enable rules they don't agree with

It's hard to configure so many rules

Large use of premade configurations

Linters are about

enforcing code style

Code formatters

module MyModule exposing (Model, Msg, update, view)

import Html exposing (Html, button, div, text)
import Html.Events exposing (onClick)


type alias Model =
    { count : Int }


type Msg
    = Increment
    | Decrement


update : Msg -> Model -> Model
update msg model =
    case msg of
        Increment ->
            { model | count = model.count + 1 }

        Decrement ->
            { model | count = model.count - 1 }


view : Model -> Html Msg
view model =
    div []
        [ button [ onClick Increment ] [ text "+1" ]
        , div [] [ text <| String.fromInt model.count ]
        , button [ onClick Decrement ] [ text "-1" ]
        ]

Linters are

NOT about

formatting

Linters are

NOT

about code style

Writing a

Static Code Analysis Rule 

Written in Elm, for Elm

  • Every Elm developer knows Elm

  • No runtime errors

Abstract Syntax Trees

(a + b) / 2
Integer 2
OperatorApplication
   operator +
FunctionOrValue a
OperatorApplication
   operator /
FunctionOrValue b
ParenthesizedExpression
rule : Rule
rule =
    Rule.newSchema "NoUsingHtmlButton" ()
        |> Rule.withExpressionVisitor expressionVisitor
        |> Rule.fromSchema


expressionVisitor : Node Expression -> Context -> ( List Error, Context )
expressionVisitor node context =
    case Node.value node of
        Expression.FunctionOrValue [ "Html" ] "button" ->
            ( [ -- Report an error
              ]
            , context
            )

        _ ->
            ( [], context )
expressionVisitor : Node Expression -> Context -> ( List Error, Context )
expressionVisitor node context =
    case Node.value node of
        Expression.FunctionOrValue [ "Html" ] "button" ->
            ( [ Rule.error
                    "Do not use Html.button"
                    (Node.range node)
              ]
            , context
            )

        _ ->
            ( [], context )

Creating guarantees

in a project

We have a great product,

it's working well and it never crashes.

But we have an inconsistent UI.
Let's improve that.

-- Page/Checkout.elm
viewConfirmButton : Html msg
viewConfirmButton =
    Html.button
        [ Attr.style "height" "35px"
        , Events.onClick UserClickedOnConfirmButton
        ]
        [ Html.text "Confirm" ]
-- Page/Payment.elm
viewPayButton : Html msg
viewPayButton =
    Html.button
        [ Attr.style "height" "33px"
        , Events.onClick UserClickedOnPayButton
        ]
        [ Html.text "Pay" ]

module Ui.Button exposing (confirm, cancel, andSomeOtherButtons)
  
confirm : msg -> String -> Html msg

cancel : msg -> String -> Html msg
-- Page/Checkout.elm
import Ui.Button as Button
viewConfirmButton : Html msg
viewConfirmButton =
    Button.confirm UserClickedOnConfirmButton "Confirm"
-- Page/Payment.elm
import Ui.Button as Button
viewPayButton : Html msg
viewPayButton =
    Button.confirm UserClickedOnPayButton "Pay"

"We now have great modules for every UI element.

Now we are sure to have a consistent UI across the application."

New pull request comes in...

view : Model -> Html Msg
view model =
    Html.button
        [ Attr.style "height" "34px"
        , Events.onClick UserClickedOnRemoveButton
        ]
        [ Html.text "Remove" ]

Providing information

  • Type information

  • Dependencies

  • Project manifest, README

  • Collecting data while traversing

False positive in

Ui.Button

type alias Context =
  { isUiButtonModule : Bool }

rule : Rule
rule =
    Rule.newSchema "NoUnusedDeclarations" { isUiButtonModule = False }
    	-- visitors...
        |> Rule.withModuleDefinitionVisitor moduleDefinitionVisitor
        |> Rule.fromSchema

moduleDefinitionVisitor : Node Module -> Context -> ( List Error, Context )
moduleDefinitionVisitor (Node _ { moduleName }) context =
    ( [], { isUiButtonModule = (moduleName == "Ui.Button") } )
module Ui.Button exposing (confirm, cancel, andSomeOtherButtons)
expressionVisitor : Node Expression -> Context -> ( List Error, Context )
expressionVisitor node context =
    case Node.value node of
        Expression.FunctionOrValue [ "Html" ] "button" ->
            if not context.isUiButtonModule then
                ( [ Rule.error {- ... -} ]
                , context
                )

            else
                ( [], context )

        _ ->
            ( [], context )

Gathering context from multiple files

module A exposing (used, neverUsed)

used =
    "some thing"

neverUsed =
    "other thing"
module B exposing (thing)

import A

thing =
    A.used ++ " is exposed"

Report unused exports

elm-review

Creating guarantees

Communication

Reducing false positives

Ignoring reports

Trustworthy automatic fixes

Developers need to trust their tools

How do we prevent this from happening?

Types...? Tests...?

view : Model -> Html Msg
view model =
    Html.button
        [ Attr.style "height" "34px"
        , Events.onClick UserClickedOnRemoveButton
        ]
        [ Html.text "Remove" ]

If you can detect the problem just by reading the code, static code analysis can help.

We need a linter

with custom rules

Are we getting the

message across?

File path

Rule name

Location

Message

Are we getting the

message across?

File path

Rule name

Location

Message

Good rule documentation

What is the target problem?

 

What are the consequences of the problem?

 

When should you (not) enable the rule?

Made with Slides.com