Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule Request: Deinit Required #2620

Closed
BenStaveleyTaylor opened this Issue Jan 31, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@BenStaveleyTaylor
Copy link
Contributor

BenStaveleyTaylor commented Jan 31, 2019

New Issue Checklist

New rule request

I would like to have the ability to require classes to have a deinit method.

  1. Why should this rule be added? Share links to existing discussion about what
    the community thinks about this.

When investigating memory leaks it is useful to be able to set a breakpoint on the deinit method. Our house style is that all classes should have a deinit even if it is empty, to facilitate debugging.
Frequently the class does not have a method, so you have to quit, add the method and rebuild in order to proceed.

  1. Provide several examples of what would and wouldn't trigger violations.
// Would trigger
class Apple {
}

class Banana
{
   func peel() {
   }
}

class Cherry: NSObject {
}

class Damson: Protocol1, Protocol2 {
}

// Would not trigger
class Apple {
   deinit {
   }
}

class Banana
{
   func peel() {
   }  
   deinit {
      print("Deinit of Banana: \(self)")
   }
}

struct Cherry {
}

enum Damson {
}

protocol Elderberry {
}
  1. Should the rule be configurable, if so what parameters should be configurable?
    No, I don't think anything beyond the standard error/warning, enable/disable is needed.

  2. Should the rule be opt-in or enabled by default? Why?
    opt-in. Clearly not everybody wants this all the time.

@BenStaveleyTaylor BenStaveleyTaylor changed the title Rule Request: Deinit Required rule Rule Request: Deinit Required Jan 31, 2019

@BenStaveleyTaylor

This comment has been minimized.

Copy link
Contributor Author

BenStaveleyTaylor commented Feb 1, 2019

@marcelofabri I'd be prepared to have a go at this. Before I invest time in it, is this a rule that would be accepted if it worked as described? If the feedback is that people don't want it, that's OK.

@marcelofabri

This comment has been minimized.

Copy link
Collaborator

marcelofabri commented Feb 1, 2019

I think it makes sense as an opt-in rule.

BenStaveleyTaylor added a commit to BenStaveleyTaylor/SwiftLint that referenced this issue Feb 2, 2019

Add deinit_required rule
Classes are required to have a deinit method.

This is a style to help debugging memory issues, when it is common to want to set a breakpoint at the point of deallocation. Most classes don't have a deinit, so the developer ends up having to quit, add a deinit and rebuild to proceed. If all classes have a deinit, debugging is much smoother.

Ref: realm#2620
@marcelofabri

This comment has been minimized.

Copy link
Collaborator

marcelofabri commented Feb 3, 2019

Implemented in #2624.

sjavora added a commit to sjavora/SwiftLint that referenced this issue Mar 9, 2019

Add deinit_required rule
Classes are required to have a deinit method.

This is a style to help debugging memory issues, when it is common to want to set a breakpoint at the point of deallocation. Most classes don't have a deinit, so the developer ends up having to quit, add a deinit and rebuild to proceed. If all classes have a deinit, debugging is much smoother.

Ref: realm#2620
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.