[Style] Explicit self when referencing instance member

(Pavel Ivashkov) #21

I did stop on the code before reading your comment, and pointed the issue right away. But the line I pointed to is the open() call. Is the open() a call-out in your terms? Would you miss it review?

You are not modeling infinitely many things during review either. If the function needs to be reentrant, then are you missing tests for reentrancy?

(Johannes Weiss) #22

No, the self.delegate?..... is.

In this particular (made up) example a re-entrancy test is indeed very simple and should be present but in more complex scenarios it's often a huge amount of work (and imagination) to come up with the very scenario that causes issues.
You're right, that review can't model all things either but if I'm confident there's no state mutation after a callout, I'm much less worried about re-entrancy and can move on, I don't need to think about it anymore. If there are local state accesses however I will need to consider re-entrancy. Therefore, for me, it's important to see all local state accesses and I do that by searching for self.. I just haven't found another good enough approximation for code bases without explicit self. yet. That's all :slight_smile:

(Pavel Ivashkov) #23

Then it's down to this specific project specific needs. "It is critical, but the resources needed are beyond acceptable." I hope you would not choose this style blindly for other projects.

(Johannes Weiss) #24

As said above no preference in a global style guide seems to be the only viable option.

(David Hart) #25

I miss Ruby's prefix characters that let you know the scope of variables: https://www.techotopia.com/index.php/Ruby_Variable_Scope

(Matias Pequeno) #26

The “ self. becomes noise” argument I really can’t follow. I use self. as a search term in Xcode and I explicitly look for self. in code reviews.

What can't you follow about the argument? Could you be more specific?

You mention your rigorous and diligent approach to self., right after, but I fail to understand how does that factor into the argument. It's just that if most people reasoned about code as you do, and most projects had the same challenges as SwiftNIO, this simply wouldn't be a problem, right?

Not trying to be cheeky here. I really want to understand where you're coming from, and I'm sure you're not alone in this either.

Today my strategy often is to make a given part of a method temporarily a static func which requires me to list all the “inputs” explicitly as function parameters, all that just to be sure nothing uses self. without me noticing...

Is it possible that you might be focusing too much on SwiftNIO as your primary case study? I don't know much about that project, but at least I can infer a few things:

  1. The average level of an app dev is probably lower than that of a SwiftNIO developer.
  2. Ensuring sound architectural integrity is probably more important to SwiftNIO than the majority of iOS apps.
  3. That will probably translate in a more rigorous development policy than the average iOS app.
  4. From what you describe, the challenges around state and memory management seem to be significantly harder than the average iOS app.
  5. I'd venture to say SwiftNIO is an unusual Swift project to the point it may very well be unique.

(Stern) #27

I don't see the difference between searching for self.delegate?. and searching for delegate?.

(Tino) #28

I guess it is not about single members, but finding all uses of all instance variables (self.*).

I think the whole topic is a tooling issue: A linter to enforce custom rules, and idially an option to highlight uses of members in the editor — that should be enough to make everyone happy.

(Johannes Weiss) #29

Indeed. the self.* rule works on all objects, even if you can't remember the whole set of properties.

Well, I most crucially need it in the github.com review UI. But yes, an automatic linter that would add all self. would be absolutely fine for me. The only reason I didn't try to get us to adopt one yet is that they all want to rewrite your whole project which obliterates git blame and makes it useless. I want a linter that only applies the linting fixes to lines changed in a given PR anyway. I don't ever want the linter to changes lines just for stylistic changes.

(Johannes Weiss) #30

As mentioned above, I usually use self. as a search term. If there's no explicit self. that doesn't work. So maybe it's noise to some eyeballs but definitely not to the search function.

I've just seen way too many bugs that came down to missing that something was a property access. As soon as you have multi-threaded code with mutable reference types or re-entrancy, it's crucial to know if something's a property access or not in my view.

SwiftNIO is definitely my primary case study and sure maybe I focus too much on it but it's definitely not the only application where I've seen explicitly calling out self. being very important to me. That has been the case with other Swift projects as well as ObjC (properties there require self. but ivars don't) and Java before.

I guess I do care a lot about correctness in all libraries/applications I work on and making it explicit what's a property access and what's just a local variable access has often proven to be very important and a source of bugs if omitted. Please note however that I am not saying that software is less correct if it doesn't use explicit self., people smarter than me might still be able to reason about what's going on but I struggle with that.

(Pierre Lorenzi) #31

It's funny because your code has a bug which is somewhat caused by the omission of self.

In the length setter, you use length as a constant whereas it is a computed property, and it changes between the two lines of the setter.

Generally when you write instance members like local members, you tend to see them as constant. It is pretty but it is deceiving. That's why I use self all the time.

(Steven Van Impe) #32

Yes, that's a bug indeed (thanks for noticing it), but it's a logic error on my part, it has nothing to do with self. I meant to refer to the property, so even if self was required, I still would have ended up with the same bug. It would only be harder to spot, because of all the self references ;)

(Stern) #33

And then there's code that converts an ivar to a local var. Swift promotes stuff like this because of unwrapping of optionals. So if you have this

guard let optional = self.optional else { return }

and use of optional in your PR but the guard statement isn't in your PR then you'll probably not realize that 'optional' is really an ivar. So to put it another way 'If you want to understand your code you have to understand your code.' In part this is a problem with PRs and the way they're reviewed in GitHub and similar githosts but in part it's your rule of self.blah not being valid.

And IMO explicit self is just noise and solves nothing.

(Tino) #34

Good point, but optional isn't an ivar. It's a constant initialzed with the value of an ivar, and depending on the exact details, this can eliminate severe problems (at least for a member with value semantics).

For me, there are situations where implicit self is beneficial, but there are other situations where it's harmful — and the rules where to draw that line have a complexity that I don't want to be part of the compiler.