See also my proposal in [1] to make a preference (or even a per-package rule) for displaying a pop-up for each shadowed variable. There is already ShadowedVariableNotification which could be caught in the parser to display a dialog window.

Best, Christoph

--- Sent from Squeak Inbox Talk

On 2022-12-26T17:17:17-06:00, asqueaker@gmail.com wrote:

...

Right now when a variable name is shadowed we only get a line appearing in the Transcript, which is alarmingly easy to miss. Given how many other alerts we get popping up for undeclared or unused variables, maybe we need to add one more for shadowing.

Shadowed variables are not an issue, technically, just from the code-clarity sense, which syntax-highlighting has now largely resolved. I think such fixes like these should be handled by the existing batch process in the development cycle where one cleans the Lint out of one's code. If you must add yet another pop up for this, make sure loading a large package with a lot of shadowed variables doesn't produce hundreds of such pop ups. Which, of course, I guess you'd be back to a bunch of code with shadowed variables with no way to identify them except the Transcript notifications again. So, maybe that's another reason to relegate this to the deLint'ing stage.

Best, Chris

...

I loaded the change set into my trunk image and nothing horrible happened. I also reviewed the changes for a while until I got bored, and the ones I looked at were all good, with well-chosen variable names.

+1 for adding to trunk.

Dave

On Fri, Dec 30, 2022 at 04:24:31PM -0800, tim Rowledge wrote:

...

On 2022-12-25, at 10:33 AM, tim Rowledge tim@rowledge.org wrote:

VariableNode>>variableGetterBlockIn: (index is shadowed) ContextVariablesInspector>>streamTemporaryVariablesOn: (context is shadowed) ContextVariablesInspector>>fieldAllTempVars (object is shadowed) ContextInspector>>streamTemporaryVariablesOn: (context is shadowed) ContextInspector>>streamStackVariablesOn: (object is shadowed) TableLayoutTest>>testShrinkWrapIssue (container is shadowed) TableLayoutTest>>testScrollPaneBarUpdate (container is shadowed) BasicInspector>>fieldObjectClass (object is shadowed) BasicInspector>>fieldObjectSize (object is shadowed) BasicInspector>>streamIndexedVariablesOn: (object is shadowed) TTFontFileHandle>>printOn: (filename is shadowed) CompiledCodeInspector>>fieldHeader (object is shadowed) CompiledCodeInspector>>fieldByteCodes (object is shadowed) SqueakTheme class>>createDuller (name is shadowed) Inspector>>streamError:on: (object is shadowed) Inspector>>getContents (object is shadowed) Inspector>>streamIndexedVariablesOn: (object is shadowed) Inspector>>streamOn:truncate:collectFields: (object is shadowed) Inspector>>valuePane (object is shadowed) Inspector>>elementGetterAt: (object is shadowed) SHTextStylerST80>>createTextAttributesForPixelHeight: (font is shadowed) IndentingListItemMorph>>charactersOccluded (icon is shadowed) FileList2 class>>endingSpecs (category is shadowed) VersionsBrowser>>fileOutSelection (file is shadowed)

...

InspectorField class>>generateExpressionFrom:argumentNames: (name is shadowed) FtpUrl>>retrieveContents (password is shadowed) ClassBasedHelpTopic>>updateSubtopics (priority is shadowed) LazyListMorph>>drawOn: (color is shadowed) PluggableTreeMorph>>setSelectedPath: (setSelectedPathSelector is shadowed) StringTest>>testFindLiteral (string is shadowed) StringTest>>testFindSelector (string is shadowed) StringTest>>testFindSymbol (string is shadowed) SqueakMessageCategoriesHelp class>>addAllCategoriesTopicTo: (category is shadowed) Ascii85ConverterTest>>testEncodeDecode (encoded is shadowed) MVCMenuMorph>>invokeAt:in:allowKeyboard: (originalFocusHolder is shadowed) HtmlEntityInspector>>fieldHtml (object is shadowed) ReleaseBuilder class>>clearCaches (environment is shadowed) ReleaseBuilder class>>cleanUpBitstreamVeraSans (name is shadowed) ReleaseBuilder class>>checkForNilCategories (category is shadowed) RxMatcher>>copy:replacingMatchesWith: (stream is shadowed) Random>>next:into: (index is shadowed)

...

Random>>nextBytes:into:startingAt: (index is shadowed)

That's not correct so far as I can see?

...

Random>>hashSeed: (index is shadowed) PluggableMultiColumnListMorph>>selectionIndex: (listMorph is shadowed) PluggableMultiColumnListMorph>>hoverRow: (listMorph is shadowed) PluggableMultiColumnListMorph>>textColor: (listMorph is shadowed) PluggableMultiColumnListMorph>>getFilteredList (fullList is shadowed) PluggableMultiColumnListMorph>>rowAtLocation: (listMorph is shadowed) PluggableMultiColumnListMorph>>updateColumns (listMorph is shadowed) PluggableMultiColumnListMorph>>rowAboutToBecomeSelected: (listMorph is shadowed) DockingBarMenuMorph>>popUpAdjacentTo:forHand:from: (owner is shadowed) SystemWindow>>paneColorToUseWhenNotActive (color is shadowed) CommunityTheme class>>createDark (name is shadowed)

OK, that's the list checked over. Now the problem is that 23 packages get touched. So just post the changeset right now and hope that does for today.

Marcel, quite a lot appear to be artefacts from a morph related cleanup blast you did a while back.

tim

tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: BSO: Branch Sort Of

On 2022-12-31, at 10:24 AM, tim Rowledge tim@rowledge.org wrote:

Thanks for taking the time Dave. It's really easy to sort of space out when running through a long list of 'just this tiny change' so a double-check is important.

The tricky part with some cases is trying to work out if the shadowing is simply an oversight and the ivar can be used, or is it a place where a separate variable is really needed and the name choice was simply unfortunate.

tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: LJD: Lock Job on Disk

Hi Tim,

I have got no problem with shadowed variables. It's a feature to be able to shadow those variables. If you want to get rid of them, then you should consider making the compiler raise an error instead of spamming the transcript with warning-looking messages. If you don't do so, they will creep back despite the current wave of cleanup. But, since I like the feature, a better option would be to adjust the highlighting of such variables to make shadowing more obvious.

On Sat, 31 Dec 2022, tim Rowledge wrote:

An excellent example is Random>>#nextBytes:into:startingAt:

It refers to a temp named 'index' which shadows an instvar. Should the method be just using that instvar, or a different named temp? I *think* the latter but it's possible Levente would suggest otherwise. All I can be sure of is that the RandomTest class reports all ok.

In Random, shadowing is pretty much intentional.

The index instance variable is only accessed by private methods implementing the PRNG logic itself. If you think the name is too generic, then you may want to rename it to nextStateIndex, as it holds to the index of the next state to generate the next random values.

From the Random object's point of view, the name index is okay IMO.

In #nextBytes:into:startingAt:, index is obviously the running index of the ByteArray to be filled up with random values. So, it clearly should not touch the instance variable. If you don't like the name index, I suggest using currentIndex instead.

For the other methods of Random, I do not fancy calling variables "i". I understand why it may be tempting to use single letters or abbreviations, especially without code completion, but I prefer descriptive names. Also, RIP my beautiful methods :(. :)

Levente

Hi Tim, Levente,

I considered shadowing a feature until I noticed it’s being frowned upon.

But, since I like the feature, a better option would be to adjust

the highlighting of such variables to make shadowing more obvious.

+1

I may have missed some discussion about this; isn’t it a similar situation as when you intentionally use an undefined variable and get a warning when accepting the method? You get a warning a that’s it.

In case of Process tests it’s an oversight on my part, apologies; I’m enclosing fixed versions of the tests:

ProcessTest>>testResumeTerminatingProcess (semaphore is shadowed) ProcessTest>>testTerminateTerminatingProcess (semaphore is shadowed)

And yes, cases like this create an unnecessary confusion whether shadowing is intentional or an oversight, so at least a comment stating the reason for using a shadowed variable would be helpful, I guess.

Best, Jaromir

From: Levente Uzonyimailto:leves@caesar.elte.hu Sent: Sunday, January 1, 2023 1:20 To: The general-purpose Squeak developers listmailto:squeak-dev@lists.squeakfoundation.org Subject: Re: [squeak-dev] A lot of shadowed variable names in Squeak 6.0 needing clearing up

Hi Tim,

I have got no problem with shadowed variables. It's a feature to be able to shadow those variables. If you want to get rid of them, then you should consider making the compiler raise an error instead of spamming the transcript with warning-looking messages. If you don't do so, they will creep back despite the current wave of cleanup. But, since I like the feature, a better option would be to adjust the highlighting of such variables to make shadowing more obvious.

On Sat, 31 Dec 2022, tim Rowledge wrote:

An excellent example is Random>>#nextBytes:into:startingAt:

It refers to a temp named 'index' which shadows an instvar. Should the method be just using that instvar, or a different named temp? I *think* the latter but it's possible Levente would suggest otherwise. All I can be sure of is that the RandomTest class reports all ok.

In Random, shadowing is pretty much intentional.

The index instance variable is only accessed by private methods implementing the PRNG logic itself. If you think the name is too generic, then you may want to rename it to nextStateIndex, as it holds to the index of the next state to generate the next random values.

From the Random object's point of view, the name index is okay IMO.

In #nextBytes:into:startingAt:, index is obviously the running index of the ByteArray to be filled up with random values. So, it clearly should not touch the instance variable. If you don't like the name index, I suggest using currentIndex instead.

For the other methods of Random, I do not fancy calling variables "i". I understand why it may be tempting to use single letters or abbreviations, especially without code completion, but I prefer descriptive names. Also, RIP my beautiful methods :(. :)

Levente