We merged this and did the next step:
8009-Rule-required-for-shadowed-variables #8917
https://github.com/pharo-project/pharo/pull/8917
Thus the Code critique now correctly shows shadowed variables even for the case
where the argument of a closure
shadows an other variables.
• The rule of course can now be further improved to exactly tell the
user what is shadowed
• We should add a rule that checks of instance or class vars shadow
globals (which needs further refinement on the shadow model)
• We should add a release test to make sure we have no shadowed vars in
the system
— both on the level of methods and on the level of Classes
but that is for the future
> On 29 Mar 2021, at 16:26, Marcus Denker <[email protected]> wrote:
>
> 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.
>
