Supporting team coding standards: Crafting Lint rules


In a previous post we learned how to craft a simple check using the already present test facilities of the language. We also mentioned the existence of Lint tools: in VA Smalltalk this tool is called SmallLint and in Pharo it is called Quality Assistant. Both tools — following the open ecosystem philosophy — allow to expand the provided rules in an easy way.

One advantage of automating the team coding standards is that when a new team member arrives, every time a unit of work is closed these checks are run, and this helps the newcomer adapt to the team idiosyncrasy. In that sense it is essential that every check or rule implemented is accompanied by a rationale explaining why the team considers it a good or bad practice.

There are some examples of this ideas on the wild. For example the Seaside web framework comes with its own program checker called Slime, which expands the SmallLint rules.


Let's see now some of the rules that our team added and their rationale.

This examples will make use of the Rewrite Engine Syntax, so a minimal understanding of that is a plus. If you don’t know it: in a nutshell `@variable matches almost any expression in the parse tree.

The first rules we will see have the same rationale: It is not a good idea to return (or assign) nil as the result of a message send. Because in general it causes a lot of unmaintainable "nil checks" on the senders (or beautiful errors in runtime :P ).

Uses #ifTrue: or #ifFalse: individually

The rationale here is to avoid using ifTrue: or ifFalse: individually and returning, assigning to a variable or passing as a parameter the result, because in case the block is not evaluated the result will be nil .

ParseTreeLintRule class>>usesIfTrueOrIfFalseIndividually
  ^self createParseTreeRule: #(
‘^`@object ifTrue: ``@block
‘^`@object ifFalse: ``@block
‘`var := ``@object ifTrue: ``@block
‘`var := ``@object ifFalse: ``@block
``@object `message: (``@anotherObject ifTrue: ``@block)’
``@object `message: (``@anotherObject ifFalse: ``@block)’
) name: ‘Uses #ifTrue: or #ifFalse: individually’
Every line inside the Array is a parse tree matching expression.

Uses #ifNotNil: individually

The rationale here is similar to the previous case. Because in case the receiver is nil the block is not evaluated and the result will be nil .

ParseTreeLintRule class>>usesIfTrueOrIfFalseIndividually
^self createParseTreeRule: #(
‘^`@object ifNotNil: ``@block
‘`var := ``@object ifNotNil: ``@block
``@object `message: (``@anotherObject ifNotNil: ``@block)’
) name: ‘Uses #ifNotNil: individually’

Returns or assigns #detect:ifFound: result

This method is an extension we made to the Collection hierarchy (Pharo includes it now in the base class library) to help when you want to do something with the result of a detect: but just in case a matching object is found.

The rationale here is similar to the previous case (note that in Pharo it will return the collection and not nil as our original implementation does). In any case if you're sending this message you shouldn't be using the result.

ParseTreeLintRule class>>returnsOrAssignsDetectIfFound
^self createParseTreeRule: #(
'^`@receiver detect: `@detectBlock ifFound: `@foundBlock'
'`var := `@receiver detect: `@detectBlock ifFound: `@foundBlock'
) name: ‘Return or Assigns #detect:ifFound:’

Intention revealing rules

The next cases don't uncover bugs but improve the readability of the resulting code.

Uses #detect:ifNone: instead of #anySatisfy: / #noneSatisfy:

This rule catches some newbie mistakes, mostly because they missed some message on the Collection API that better suits the situation.

ParseTreeLintRule class>>returnsOrAssignsDetectIfFound
^self createParseTreeRule: #(
'(``@col detect: [:`e| |`@temps| ``@.exp] ifNone: [nil]) isNil'
'(``@col detect: [:`e| |`@temps| ``@.exp] ifNone: [nil]) = nil'
'(``@col detect: [:`e| |`@temps| ``@.exp] ifNone: [nil]) == nil'
'(``@col detect: [:`e| |`@temps| ``@.exp] ifNone: [nil]) notNil'
'(``@col detect: [:`e| |`@temps| ``@.exp] ifNone: [nil]) ~= nil'
'(``@col detect: [:`e| |`@temps| ``@.exp] ifNone: [nil]) ~~ nil' )
name: ‘Uses #detect:ifNone: instead of #anySatisfy:/#noneSatisfy:’

When we detect something like that in a code review, if it's possible we add a new rule suggesting the replacement code so it should never happen again.

Assignment and testing

We found that sometimes it is hard to grasp the code intention when an object is assigned to some variable and then immediately sent a message on the same expression. So for the sake of clarity we prefer to make the assignment first and send the message in the following statement.

ParseTreeLintRule class>>assignmentAndTesting
^self  createParseTreeRule: #(‘( `var := `@object ) `message’)
name: ‘Assigns and tests an object’

Uses 1 / object instead of #reciprocal

This one seems to be superfluous, but in Mercap we develop financial software and we have a lot of math abstractions besides numbers, like prices, quotes, money measures, etc. Therefore it is clearer to send the reciprocal message to a math object to get the inverse, for example instead of doing 1 / (20.45 ARS/USD) we do (20.45 ARS/USD) reciprocal to get a reverse currency quote. When you have a long financial formula this provides better readability.

ParseTreeLintRule class>>assignmentAndTesting
^self  createParseTreeRule: #('1 / `@object') 
name: 'Uses 1 / object instead of #reciprocal'

Bugs and Possible Bugs

Uses cascading in Array/OrderedCollection creation. Remove the extra “;”

This rule comes from bad experiences converting some #add: cascade to a simpler method send. For example turning:

OrderedCollection new 
add:1;
add:2;
add:3;
yourself

into

OrderedCollection with: 1 with: 2 with: 3

It happened a lot that we did some search for #add: then replaced it with #with: forgetting to remove the ; and only noticed it when some test fails later.

ParseTreeLintRule class>>avoidCascadingAnArrayCreation
^self createParseTreeRule: #(
‘Array with: `@firstObject ; with: `@secondObject’
‘Array with: `@firstObject ; with: `@secondObject ; with: `@thirdObject’
‘Array with: `@firstObject with: `@secondObject ; with: `@thirdObject’
‘Array with: `@firstObject ; with: `@secondObject with: `@thirdObject’
‘Array with: `@firstObject ; with: `@secondObject ; with: `@thirdObject ; with: `@fourthObject’
‘Array with: `@firstObject with: `@secondObject ; with: `@thirdObject ; with: `@fourthObject’
‘Array with: `@firstObject with: `@secondObject with: `@thirdObject ; with: `@fourthObject’
‘Array with: `@firstObject with: `@secondObject ; with: `@thirdObject with: `@fourthObject’
‘Array with: `@firstObject ; with: `@secondObject with: `@thirdObject ; with: `@fourthObject’
‘Array with: `@firstObject ; with: `@secondObject ; with: `@thirdObject with: `@fourthObject’
‘Array with: `@firstObject ; with: `@secondObject with: `@thirdObject with: `@fourthObject’
)
name: ‘Uses cascading in Array/OrderedCollection creation. Remove the extra “;”’
The previous example is using Array, but the original rule is a bit more complex including other collections. I've simplified it here for didactic reasons.

Class implements #doesNotUnderstand: but misses #respondsTo:

This rule covers a possible bug and checks that if a class implements #doesNotUnderstand: it probably needs to implement #respondsTo: as well, because these methods are supposed to work in tandem. We don't use it a lot but found some cases when we missed the #respondsTo: implementation and noticed it too late.

BlockLintRule>>implementorsOfDoesNotUnderstandMustImplementRespondsTo
^self new 
resultClass: ClassEnvironment;
name: ‘Class implements #doesNotUnderstand: but misses #respondsTo:’;
classBlock: [:context :result | | currentClass |
currentClass := context selectedClass.
((currentClass includesSelector: #doesNotUnderstand:)
and: [(currentClass includesSelector: #respondsTo:) not])
ifTrue: [result addClass: currentClass]];
yourself

Thank you for reading and feel free to share your own rules or practices. If you haven’t automated these rules so far, start now!.