Home Archives Search Feed About

Modeling Variable Shadowing, part II

We merged the prior PR from Part I. Now we can add a Critique Rule. And we need to think about what is needed to be able to analyse variables defined on the level of classes.

Addign a Code Critique Rule

Now the next step is using the new #isShadowing in the Rule:

8009-Rule-required-for-shadowed-variables #8917

Thus the Code critique now correctly shows shadowed variables even for the case where the argument of a closure shadows other variables.

This PR adds a nice helper method: getting all nodes from the AST that are defines by a method. Maybe we should add an API to get all defined variables from a CompiledMethod, too, but for now, the solution to go via the AST node is sufficient.

Adding a Release Test

This whole exploration of Variable Shadowing started with a bug report. It would be nice to never have cases of shadowed variables. To make sure we never introduce them again, we can add a test to the ReleaseTest package.

Just looking a bit at other tests checking something for each method and starting with it as a template, we can easily now implement #testNoShadowedVariablesInMethods:

testNoShadowedVariablesInMethods
    "Fail if there are methods who define shadowed temps or args"
    | found validExceptions remaining |
    found := SystemNavigation default allMethodsSelect: [ :m | 
        m ast variableDefinitionNodes anySatisfy: [ :node | node variable isShadowing ] ].
    
    "No other exceptions beside the ones mentioned here should be allowed"  
    validExceptions := { 
        RBDummyRefactoryTestDataApp>>#tempVarOverridesInstVar.
        RBRefactoryTestDataApp>>#tempVarOverridesInstVar.
        RBSmalllintTestObject>>#tempVarOverridesInstVar.
        ReTempVarOverridesInstVarRuleTest>>#sampleMethod:}. 
    
    remaining := found asOrderedCollection 
                                removeAll: validExceptions;
                                yourself.
                                
    self 
        assert: remaining isEmpty 
        description: ('the following methods have shadowing variable definitions and should be cleaned: ', remaining asString)

Of course, I first ran with empty validExceptions, then I added all as these methods. These are examples from tests that are testing the handling of variable shadowing. The test is green! PR

Shadowed Variables on the level of the class

We should think about Shadowed Variables on the level of a class

Classes define instance variables (modeled by sub-instances of Slot) and Class Variables (sub-instances of ClassVariable). They can shadow variables from the environment that the class is in (in the usual case this is Smalltalk Globals).

We should extend the concept of Shadowed variables to cover these cases, too.

Imagine we have a class that has a class variable “Person”. Then we add a class named “Person”: The class variable is now shadowing the class. #isShadowing, for now, is just checking for the property. But here we run into a problem: who could set the property? The class builder could do it. But adding a class would mean that we would need to check all existing classes if the new global is shadowed somewhere.

And we would not just have to check the classes: nothing forbids us to use a block argument “Person”… (even though bad style). Thus we would need to re-analyze all code globally. Not a good idea in a system where a user might have >100K classes…

Thus: we need to rethink our solution. It was not wrong: we were concerned with just modeling shadowing for methods, the solution to tag the variables was a good one for that use. But now we need to step back. What is shadowing again? It means that the outer scope of a variable has already a variable of the same name.

Interestingly, the variable model is already modeling all the needed information. Variables have a scope, the scope has a parent scope. We can lookup vars by name with #lookupVar. We can just change #isShadowing to implement exactly “I shadow a variable if looking up my name in my outer scope returns a variable”.

isShadowing
    "I shadow a variable if looking up my name in the outer scope returns a variable"
    ^(self scope outerScope ifNotNil: [:outer | outer lookupVar: self name]) notNil

We just have to add a missing #outerScope method to Behavior (as the outer scope is called the “environment” here), and one to SystemDictonary returning nil. And it works! The tests we did for shadowing are still green.

Pull request is here: https://github.com/pharo-project/pharo/pull/8931

With this in place, we can add a new Critique Rule. But that is for next time.

TODO NEXT

• We should add a rule that checks of instance or class vars shadow globals
• We should add a release test to make sure we have no shadowed vars on the class level in the system
• All the rules should be further improved to exactly tell the user what is shadowed where

Read Part III

Posted on April 2, 2021   #Pharo  





← Next post    ·    Previous post →