The issue tracker entry "Rule required for shadowed variables" [#8009](https://github.com/pharo-project/pharo/issues/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 ```Smalltalk 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? ```Smalltalk 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: ```Smalltalk 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: ```Smalltalk 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: ```Smalltalk 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: ```Smalltalk variable: aVariable shadows: shadowedVariable inNode: variableNode aVariable shadowing: shadowedVariable. compilationContext optionSkipSemanticWarnings ifTrue: [ ^aVariable ]. ^ OCShadowVariableWarning new node: variableNode; shadowedVar: aVariable; compilationContext: compilationContext; signal ``` with: ```Smalltalk 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 ```Smalltalk shadowing: anotherVariable self propertyAt: #shadows put: anotherVariable ``` and ```Smalltalk "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 ```Smalltalk 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](https://github.com/pharo-project/pharo/pull/8909). 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.