Allow explicit self in STYLEGUIDE.md

[phillmv]

Hey there!

I just got pinged for @sampart's PR here where explicit self was being auto-removed from the codebase, and I experienced a negative reaction to it.

As a means of discussion, this PR proposes removing the avoid explicit use of self rule – but I don't think I actually care about it as a styleguide preference per se, but I feel like I do disagree with it being auto-enforced.

Here is my reasoning:

As the styleguide suggests, sometimes you need to explicitly refer to self, i.e. "unless to specify a method shadowed by a variable". Sometime in the distant past, earlier in my career, I got bit by variable shadowing: I updated a local variable instead of referencing a class method, and introduced a bug.

Ever since then I have adopted a defensive posture: whenever I am referring to a classes' method, or for example, an ActiveRecord attribute, I always explicitly use self.foo. By always being explicit, it is impossible to accidentally, later on, in an unrelated change, introduce a bug because a variable got shadowed. In some of these code we work on it's simply impossible for every developer to know every class method.

OK Phill, you might say, but why does this matter? Just because you've developed trauma-informed behaviours doesn't mean it's a concern for the rest of us.

Here's the kicker for making it auto-enforced: if you almost always get dinged by the linter, after you've written the code, for using self. you will reflexively adopt the opposite stance and avoid self. as much as possible. This means that when you do actually shadow a variable you might be less likely to pick it up. It imposes a cognitive burden on the developer.¹

(If I were god-empress of the world, I'd go the full other direction and suggest enforcing always using explicit self as a means of removing this class of bug altogether, but here I would be satisfied by simply not making MY life more annoying 😉.)

tl;dr:

• enforcing implicit self does not catch bugs or improve performance
• enforcing implicit self can, occasionally, introduce bugs
• i reflexively always use explicit self and this would impose a burden on me personally, which we can all agree is a great tragedy

Thank you kindly for your time & consideration,

Footnotes

1. An analogy here might be making semi-colons optional in pure Javascript. Yes, it works, except in occasions where it introduces a bug. So to support removing semi-colons, you need to be perfectly aware of the edge cases where it introduces a bug. That sounds a lot harder than just using semi-colons! ↩

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

[bensheldon]

I think this is a case where a consistent style across the codebase reduces cognitive load.

I think we either use implicit receiver everywhere, in which case usage of self implies a shadowing. Or we use an explicit self everywhere, in which case it's absence implies a local variable. Mixing them in the same codebase is means that everything must be inspected more deeply.

My preference is to maintain the current implicit receiver. That said, I'm bummed that the matching guardrails of detecting shadow variables are currently disabled:

rubocop-github/config/default.yml

Lines 656 to 663 in 5db0a50

	Lint/ShadowedArgument:
	Enabled: false
	Lint/ShadowedException:
	Enabled: false
	Lint/ShadowingOuterLocalVariable:
	Enabled: false

Do you think enabling the shadow warnings would address (some) of your concerns?

[phillmv]

If I may I'd push back gently on this assertion:

I think this is a case where a consistent style across the codebase reduces cognitive load. … Mixing them in the same codebase is means that everything must be inspected more deeply.

I feel like you need to inspect it carefully in both the do_nothing AND the enforce_implicit options?

Maybe if we turned on the shadow warnings and therefore could trust that variables are never shadowed it wouldn't be an issue but I don't think that's something we can guarantee.

In terms of cognitive_load(option), I would sort them like this?

cognitive_load(enforce_implicit) > cognitive_load(do_nothing) > cognitive_load(enforce_explicit)

--

I guess enabling the shadow warnings is better than not having them turned on at all?

I also totally understand if for whatever reason I am in the minority for this aesthetic preference, the world isn't organized around my pet peeves 😉.