Modeling Variable Shadowing, part III

In Part II, we enhanced Variables to model Shadowed Variables on the level of the class. Now we should use that and add a rule and a release test.

Adding a Release test to check for shadowed vars defined at the level of a class

Imagine we make a class like this:

Object subclass: #MyClass
    instanceVariableNames: 'Array i'
    classVariableNames: 'Object'
    package: 'MyPackage'

We can ask the class for all the variables it defines with #definedVariables. Thus is easy to filter for the shadowing variables:

MyClass definedVariables select: [ :variable | variable isShadowing  ] 

"{#Array => InstanceVariableSlot. #Object->nil}"

With this, we now can easily scan the whole system for problematic classes:

Smalltalk globals allBehaviors select: [ :class | 
        class definedVariables anySatisfy: [:var | var isShadowing]].

And interestingly, this returns results! Of course, our class MyClass as defined above, but also, we do have six classes where class variables shadow globals. We should fix them at some point. I added an issue tracker entry: https://github.com/pharo-project/pharo/issues/8941

Maybe someone wants to try to fix one?

For now, we can add a release test that ensures that we do not add more cases:

testClassesShadow
    |  classes validExceptions remaining |

    classes := Smalltalk globals allBehaviors select: [ :class | 
        class definedVariables anySatisfy: [:var | var isShadowing]].

    validExceptions := #(). 
    
    remaining := classes asOrderedCollection reject: [ :each  | validExceptions includes: each name].
    "6 left that we need to fix"
    self assert: remaining size <= 6.

See https://github.com/pharo-project/pharo/pull/8939

Code Critique

What we need next is a Code Critique rule. We did this already for method level shadowed vars, we need it for classes, too.

This for once warns developers early, but besides, it will make it much easier to fix the six problem cases seen earlier.

Checking for current senders of #definedVariables leads us to ReVariableAssignedLiteralRule, we can use this as a template. The main method doing the check looks like this:

check: aClass forCritiquesDo: aCriticBlock

    aClass definedVariables
        select: [ :variable | variable isShadowing ]
        thenDo: [ :variable | aCriticBlock cull: (self critiqueFor: aClass about: variable name) ]

This was done in two PRs: https://github.com/pharo-project/pharo/pull/8940 and https://github.com/pharo-project/pharo/pull/8965

(I often do things in two steps: first, do it, then come back later and clean or simplify. I often keep a backlog with ideas to be checked later)

Cleanup from the backlog

We can simplify OCUndeclaredVariableWarning

OCShadowVariableWarning does not need the variable, the defining node is enough In OCASTSemanticAnalyzer: simplify #declareVariableNode:as: add #shadowingVariable: to be more in sync with the other semantic warning methods (like #undeclaredVariable:)

Trivial change, but doing these kinds of cleanups regularly really improves understandability.

https://github.com/pharo-project/pharo/pull/8943/files

What did we see?

I have to say I still find this fascinating!

How improving the meta-model of a system leads to better tools, which leads to a cleaner system… it’s a feedback loop!

But it requires of us to be able to improve both the tools and the language. Nothing we did here would have been possible just “on top” of an Unchangeable System.

“Only that which can change can continue” (James P. Carse)

Posted on April 7, 2021   #Pharo