Modeling Variable Shadowing

The issue tracker entry “Rule required for shadowed variables” #8009

It reads:

---

In the CI output log for PRs on Pharo 9 issues I see the following warnings on Roassal classes:

SystemNotification: RSCanvas>>numberOfEdges(edges is shadowed) SystemNotification: RSCanvas>>numberOfEdges(edges is shadowed) SystemNotification: RSCanvas>>numberOfNodes(nodes is shadowed) SystemNotification: RSCanvas>>numberOfNodes(nodes is shadowed) SystemNotification: RSComposite>>numberOfEdges(edges is shadowed) SystemNotification: RSComposite>>numberOfEdges(edges is shadowed) SystemNotification: RSComposite>>numberOfNodes(nodes is shadowed) SystemNotification: RSComposite>>numberOfNodes(nodes is shadowed) SystemNotification: RSShape>>printOn:(model is shadowed) SystemNotification: RSShape>>printOn:(model is shadowed)

which means we have local variables (or block variable) in a method with the same name as an existing ivar in the class.

These cases should be cleaned in Rossal3 for the Pharo 9 image - for that I opened https://github.com/ObjectProfile/Roassal3/issues/321 on Roassal3 issue tracker.

But it would be good if we could have a Lint rule that shows that problem so one can avoid it:

Side note: Yes - there already is a rule called ReTempVarOverridesInstVarRuleTest - but this is only for temps and does not cover variables in blocks like in

numberOfEdges
    "Return the number of edges contained in the container"
    ^ self privateEdges
        ifNil: [ 0 ]
        ifNotNil: [ :edges | edges size ]

when “edges” is already an instance variable

---

So how is ReTempVarOverridesInstVarRuleTest implemented?

check: aMethod forCritiquesDo: aCriticBlock
    | ivarNames problemTemps|
    ivarNames := aMethod methodClass instVarNames.
    ivarNames ifEmpty: [ ^ self ].
    
    problemTemps := ((aMethod ast arguments, aMethod ast temporaries)
        select: [ :node |ivarNames includes: node name]).
    problemTemps do: [ :node |
        aCriticBlock cull: (self critiqueFor: aMethod about: node) ] 

So, this test is done purely on the results of the compilation, it just checks if temps are overriding the instance vars. It just does not care about blocks, nor class variables or globals.

We could now, for this issue, recurse into the compiled block closures, but that would make the code quite complex.

To see a better solution, let’s check who actually prints out the log message in the build. Just searching for a substring will lead us to OCShadowVariableWarning>>#variable:shadows:

variable: varNode shadows: semVar
    compilationContext interactive
        ifTrue: [ 
            OCSemanticError new
                node: node;
                compilationContext: compilationContext;
                messageText: self stringMessage;
                signal ]
        ifFalse: [ self showWarningOnTranscript ].
        

This is an exception raised by the AST visitor that does name analysis, OCASTSemanticAnalyzer:

variable: variableNode shadows: semVar
    compilationContext optionSkipSemanticWarnings ifTrue: [ ^semVar ].
    ^ OCShadowVariableWarning new
        node: variableNode;
        shadowedVar: semVar;
        compilationContext: compilationContext;
        signal

This method is called whever we lookup a Variable name to create a Variable object describing it:

declareVariableNode: aVariableNode as: anOCTempVariable
    | name var |
    name := aVariableNode name.
    var := scope lookupVarForDeclaration: name.
    var ifNotNil: [ 
        "Another variable with same name is visible from current scope.
        Warn about the name clash and if proceed add new temp to shadow existing var" 
        self variable: aVariableNode shadows: var ].
    var := scope addTemp: anOCTempVariable.
    aVariableNode binding: var.
    ^ var

This means: in non-interactive mode, we just log the error and compile while allowing shadowing. In interactive mode we forbid it (we raise a OCSemanticError exception that will be displayed in the code editor).

What could we now do to improve the model of shadowing variables? The best thing would be if we could add the fact that a variable shadows another to the semantic information. That is, it should be modeled as part of the variable.

We do not need much: just add this information as a property would be perfectly enough. This way we can add a test method (that check if the property exists). On the level of the Code Critique rule it will be very simple: just get all variables that are defined (args, temps, args of blocks) and check if they are shadowing. That’s easy!

So what is needed to implement it?

We first need to make sure we hand over one more parameter to #variable:shadows: and call it later: right now, in interactive mode, we never create a Variable at all. There is no problem to delay that and tag the variable correctly:

variable: aVariable shadows: shadowedVariable inNode: variableNode
    aVariable shadowing: shadowedVariable.
    compilationContext optionSkipSemanticWarnings ifTrue: [ ^aVariable ].
    ^ OCShadowVariableWarning new
        node: variableNode;
        shadowedVar: aVariable;
        compilationContext: compilationContext;
        signal

with:

declareVariableNode: aVariableNode as: anOCTempVariable
    | name var shadowed |
    name := aVariableNode name. 
    var := scope lookupVarForDeclaration: name.
    var ifNotNil: [ 
        "Another variable with same name is visible from current scope"
        shadowed := var.
        ].
    var := scope addTemp: anOCTempVariable.
    aVariableNode binding: var.
    shadowed ifNotNil: [self variable: var shadows: shadowed inNode: aVariableNode].
    ^ var

To be able to tag Variables, we implement

shadowing: anotherVariable
    self propertyAt: #shadows put: anotherVariable

and

"testing"
isShadowing
    ^self hasProperty: #shadows

We do this on Variables, which means that we can later introduce Variable shadow tagging even for instance variables and Class Variables

How do we now test this?

One idea is to rewrite ReTempVarOverridesInstVarRule>>#check:forCritiquesDo: to use #isShadowing. ReTempVarOverridesInstVarRuleTest is green, we have something that is reado to commit.

Let’s try

check: aMethod forCritiquesDo: aCriticBlock

    | problemTemps |
    problemTemps := aMethod temporaryVariables select: [ :var | 
                        var isShadowing ].
    problemTemps do: [ :var | 
        aCriticBlock cull:
            (self critiqueFor: aMethod about: var definingNode) ]

And: Success! This is now a first PR to be done see here. This does not yet provide the Rule and the Release Test for shadowing vars in general, but it is a very nice first step that we can build upon next.

Read Part II

Posted on March 29, 2021   #Pharo