deinit and failable initializers


(Chris Eidhof) #1

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.

Chris


(Douglas Gregor) #2

Let’s re-order the statements in your example:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int

    init?(capacity: Int) {
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        pointer = UnsafeMutablePointer.alloc(capacity)
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

If the initializer returns ‘nil’ and we still call deinit, we end up destroying a pointer that was never allocated.

If you come from an Objective-C background, you might expect implicit zeroing of the allocated block to help here. However, Swift doesn’t have that, because many Swift types don’t have a “zero” state that’s safe to destroy. For example, anything with a member of non-optional reference type, e.g.,

class ClassWrapper {
  var array: MyArray<String>

  init(array: MyArray<String>) {
    self.array = array
  }

  deinit {
    print(array) // array is a valid instance of MyArray<String>
  }
}

A valid ClassWrapper instance will always have an instance of MyArray<String>, even throughout its deinitializer.

The basic property here is that one cannot run a deinitializer on an instance that hasn’t been fully constructed (all the way up the class hierarchy).

  - Doug

···

On Jan 26, 2016, at 9:15 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org> wrote:

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.


(Joe Groff) #3

A simple deinitOnNil/deinitOnError will be insufficient as soon as you have to allocate more than one resources with a failable point in between.

How so? In general, you want to clean up these resources unless you've already committed them to a fully-initialized object. This should do the right thing:

init() throws {
  self.foo = alloc(); deferOnError { dealloc(self.foo) }
  try something()
  self.bar = alloc(); deferOnError { dealloc(self.bar) }
  try something()
  super.init()
}

-Joe

I think that it could best be solved with move semantics but that's far off Swift 3.

You don't need move semantics; you could also factor out your resources into dedicated owner classes with non-failable initializers, which would allow for automatic memory management to do the right thing. The problem only arises with unsafe resources or other invariants that require manual management, and it's a general issue with implicit early exits.

-Joe

···

On Jan 26, 2016, at 9:58 AM, Félix Cloutier via swift-evolution <swift-evolution@swift.org> wrote:

Félix

Le 26 janv. 2016 à 12:15:46, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> a écrit :

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.

Chris
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Félix Cloutier) #4

A simple deinitOnNil/deinitOnError will be insufficient as soon as you have to allocate more than one resources with a failable point in between.

I think that it could best be solved with move semantics but that's far off Swift 3.

Félix

···

Le 26 janv. 2016 à 12:15:46, Chris Eidhof via swift-evolution <swift-evolution@swift.org> a écrit :

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.

Chris
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Félix Cloutier) #5

I thought it was a separate magic method. This pattern is fine.

Félix

···

Le 26 janv. 2016 à 13:03:47, Joe Groff <jgroff@apple.com> a écrit :

On Jan 26, 2016, at 9:58 AM, Félix Cloutier via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

A simple deinitOnNil/deinitOnError will be insufficient as soon as you have to allocate more than one resources with a failable point in between.

How so? In general, you want to clean up these resources unless you've already committed them to a fully-initialized object. This should do the right thing:

init() throws {
  self.foo = alloc(); deferOnError { dealloc(self.foo) }
  try something()
  self.bar = alloc(); deferOnError { dealloc(self.bar) }
  try something()
  super.init()
}

-Joe

I think that it could best be solved with move semantics but that's far off Swift 3.

You don't need move semantics; you could also factor out your resources into dedicated owner classes with non-failable initializers, which would allow for automatic memory management to do the right thing. The problem only arises with unsafe resources or other invariants that require manual management, and it's a general issue with implicit early exits.

-Joe

Félix

Le 26 janv. 2016 à 12:15:46, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> a écrit :

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.

Chris
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution


(Chris Eidhof) #6

Absolutely, I agree with you. Of course, this was a simplified example, you
can easily construct an example where just reordering isn’t going to cut it.

It’s not hard to do things right in the current model. We can reorder
statements, write wrappers, or think of different solutions altogether.
Once you understand how it works, it’s very easy to write correct code.

However, my complaint is that it’s now also easy to make a mistake. In the
previous model, it wasn’t so easy (the previous model was simpler: an init
would always be matched by a deinit).

···

On Tue, Jan 26, 2016 at 6:39 PM, Douglas Gregor <dgregor@apple.com> wrote:

On Jan 26, 2016, at 9:15 AM, Chris Eidhof via swift-evolution < > swift-evolution@swift.org> wrote:

Now that we can return nil from a failable initializer without having
initialized all the properties, it’s easier to make a mistake. For example,
consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int

    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words,
duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity

    }

    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by
the pointer. Or in other words, we need to duplicate the behavior from the
deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we
could count on deinit being called, *always*. With the current behavior,
return `nil` is easier, but it does come at the cost of accidentally
introducing bugs. As Joe Groff pointed out, a solution would be to have
something like “deferOnError” (or in this case, “deferOnNil”), but that
feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t
like that it goes at the cost of safety, but I realize it’s probably only
making things less safe in a small amount of edge cases.

Let’s re-order the statements in your example:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int

    init?(capacity: Int) {
        if capacity > 100 {
            // Here we should also free the memory. In other words,
duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        pointer = UnsafeMutablePointer.alloc(capacity)
    }

    deinit {
        pointer.destroy(capacity)
    }
}

If the initializer returns ‘nil’ and we still call deinit, we end up
destroying a pointer that was never allocated.

If you come from an Objective-C background, you might expect implicit
zeroing of the allocated block to help here. However, Swift doesn’t have
that, because many Swift types don’t have a “zero” state that’s safe to
destroy. For example, anything with a member of non-optional reference
type, e.g.,

class ClassWrapper {
  var array: MyArray<String>

  init(array: MyArray<String>) {
    self.array = array
  }

  deinit {
    print(array) // array is a valid instance of MyArray<String>
  }
}

A valid ClassWrapper instance will always have an instance of
MyArray<String>, even throughout its deinitializer.

The basic property here is that one cannot run a deinitializer on an
instance that hasn’t been fully constructed (all the way up the class
hierarchy).

- Doug

--
Chris Eidhof


(Charles Srstka) #7

In Swift, this sort of pattern is pretty rare. In the cases where you do need to manually allocate memory, you can always wrap the initializer in a “do” block, and release the memory in “catch”.

I don’t think this is a big issue.

Charles

···

On Jan 26, 2016, at 11:15 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org> wrote:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}


(Joseph Lord) #8

I think that there is risk here but I don't think it is serious enough one to be with making changes for.

1. For most classes there is no problem because everything is managed by ARC anyway so will clean up without special consideration (few of my classes require a deinit).
2. It doesn't seem surprising that if init did not succeed that deinit will not run.

I suppose the risk is greatest where the nil is a result of a returned value from a call made being passed up as explicit `return nil` or `throw` are more visible.

If something were to be changed I wonder if in cases where a failable init exists in a class with a deinit the init could still be responsible for the clean up but must also call an special empty function to confirm to compiler that the situation has been handled. Not sure on name but maybe `deinited()`. Maybe it would just be a warning if not called. Treatment could maybe be similar to warnings where super calls are not made.

Joseph

···

On Jan 26, 2016, at 9:39 PM, Chris Eidhof via swift-evolution <swift-evolution@swift.org> wrote:

Absolutely, I agree with you. Of course, this was a simplified example, you can easily construct an example where just reordering isn’t going to cut it.

It’s not hard to do things right in the current model. We can reorder statements, write wrappers, or think of different solutions altogether. Once you understand how it works, it’s very easy to write correct code.

However, my complaint is that it’s now also easy to make a mistake. In the previous model, it wasn’t so easy (the previous model was simpler: an init would always be matched by a deinit).

On Tue, Jan 26, 2016 at 6:39 PM, Douglas Gregor <dgregor@apple.com> wrote:

On Jan 26, 2016, at 9:15 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org> wrote:

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.

Let’s re-order the statements in your example:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int

    init?(capacity: Int) {
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        pointer = UnsafeMutablePointer.alloc(capacity)
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

If the initializer returns ‘nil’ and we still call deinit, we end up destroying a pointer that was never allocated.

If you come from an Objective-C background, you might expect implicit zeroing of the allocated block to help here. However, Swift doesn’t have that, because many Swift types don’t have a “zero” state that’s safe to destroy. For example, anything with a member of non-optional reference type, e.g.,

class ClassWrapper {
  var array: MyArray<String>

  init(array: MyArray<String>) {
    self.array = array
  }

  deinit {
    print(array) // array is a valid instance of MyArray<String>
  }
}

A valid ClassWrapper instance will always have an instance of MyArray<String>, even throughout its deinitializer.

The basic property here is that one cannot run a deinitializer on an instance that hasn’t been fully constructed (all the way up the class hierarchy).

  - Doug

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Kostiantyn Koval) #9

In my opinion deinit should be called only for objets that we fully initialized successfully (as it works now).
How about allowing early return only when no properties were initialized.

The possible problem with this solution is that complex initializers would need to store intermediate results in local variables before assigning them to properties.
Example:

struct MyArray<T> {
  var pointer: UnsafeMutablePointer<T>
  var capacity: Int

  init?(capacity: Int) {
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    if capacity > 100 {
      return nil // Error. Can't return nil after partial initialization.
    }
    self.capacity = capacity
  }

  init?(capacity: Int) {
    if capacity > 100 {
      return nil // This is ok, early return before initializing any property
    }
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    self.capacity = capacity
  }
}

Kostiantyn

···

On 27 Jan 2016, at 02:40, Joseph Lord via swift-evolution <swift-evolution@swift.org> wrote:

I think that there is risk here but I don't think it is serious enough one to be with making changes for.

1. For most classes there is no problem because everything is managed by ARC anyway so will clean up without special consideration (few of my classes require a deinit).
2. It doesn't seem surprising that if init did not succeed that deinit will not run.

I suppose the risk is greatest where the nil is a result of a returned value from a call made being passed up as explicit `return nil` or `throw` are more visible.

If something were to be changed I wonder if in cases where a failable init exists in a class with a deinit the init could still be responsible for the clean up but must also call an special empty function to confirm to compiler that the situation has been handled. Not sure on name but maybe `deinited()`. Maybe it would just be a warning if not called. Treatment could maybe be similar to warnings where super calls are not made.

Joseph

On Jan 26, 2016, at 9:39 PM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Absolutely, I agree with you. Of course, this was a simplified example, you can easily construct an example where just reordering isn’t going to cut it.

It’s not hard to do things right in the current model. We can reorder statements, write wrappers, or think of different solutions altogether. Once you understand how it works, it’s very easy to write correct code.

However, my complaint is that it’s now also easy to make a mistake. In the previous model, it wasn’t so easy (the previous model was simpler: an init would always be matched by a deinit).

On Tue, Jan 26, 2016 at 6:39 PM, Douglas Gregor <dgregor@apple.com <mailto:dgregor@apple.com>> wrote:

On Jan 26, 2016, at 9:15 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.

Let’s re-order the statements in your example:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int

    init?(capacity: Int) {
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        pointer = UnsafeMutablePointer.alloc(capacity)
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

If the initializer returns ‘nil’ and we still call deinit, we end up destroying a pointer that was never allocated.

If you come from an Objective-C background, you might expect implicit zeroing of the allocated block to help here. However, Swift doesn’t have that, because many Swift types don’t have a “zero” state that’s safe to destroy. For example, anything with a member of non-optional reference type, e.g.,

class ClassWrapper {
  var array: MyArray<String>

  init(array: MyArray<String>) {
    self.array = array
  }

  deinit {
    print(array) // array is a valid instance of MyArray<String>
  }
}

A valid ClassWrapper instance will always have an instance of MyArray<String>, even throughout its deinitializer.

The basic property here is that one cannot run a deinitializer on an instance that hasn’t been fully constructed (all the way up the class hierarchy).

  - Doug

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Jean-Daniel) #10

Forcing usage of local variable don’t solve the code duplication issue.
You will still have to replicate the deinit logic in the initializer (to release resources used by temporary local variable instead of member variables, but this is the same).

···

Le 27 janv. 2016 à 10:26, Kostiantyn Koval via swift-evolution <swift-evolution@swift.org> a écrit :

In my opinion deinit should be called only for objets that we fully initialized successfully (as it works now).
How about allowing early return only when no properties were initialized.

The possible problem with this solution is that complex initializers would need to store intermediate results in local variables before assigning them to properties.
Example:

struct MyArray<T> {
  var pointer: UnsafeMutablePointer<T>
  var capacity: Int

  init?(capacity: Int) {
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    if capacity > 100 {
      return nil // Error. Can't return nil after partial initialization.
    }
    self.capacity = capacity
  }

  init?(capacity: Int) {
    if capacity > 100 {
      return nil // This is ok, early return before initializing any property
    }
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    self.capacity = capacity
  }
}

Kostiantyn

On 27 Jan 2016, at 02:40, Joseph Lord via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

I think that there is risk here but I don't think it is serious enough one to be with making changes for.

1. For most classes there is no problem because everything is managed by ARC anyway so will clean up without special consideration (few of my classes require a deinit).
2. It doesn't seem surprising that if init did not succeed that deinit will not run.

I suppose the risk is greatest where the nil is a result of a returned value from a call made being passed up as explicit `return nil` or `throw` are more visible.

If something were to be changed I wonder if in cases where a failable init exists in a class with a deinit the init could still be responsible for the clean up but must also call an special empty function to confirm to compiler that the situation has been handled. Not sure on name but maybe `deinited()`. Maybe it would just be a warning if not called. Treatment could maybe be similar to warnings where super calls are not made.

Joseph

On Jan 26, 2016, at 9:39 PM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Absolutely, I agree with you. Of course, this was a simplified example, you can easily construct an example where just reordering isn’t going to cut it.

It’s not hard to do things right in the current model. We can reorder statements, write wrappers, or think of different solutions altogether. Once you understand how it works, it’s very easy to write correct code.

However, my complaint is that it’s now also easy to make a mistake. In the previous model, it wasn’t so easy (the previous model was simpler: an init would always be matched by a deinit).

On Tue, Jan 26, 2016 at 6:39 PM, Douglas Gregor <dgregor@apple.com <mailto:dgregor@apple.com>> wrote:

On Jan 26, 2016, at 9:15 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.

Let’s re-order the statements in your example:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int

    init?(capacity: Int) {
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        pointer = UnsafeMutablePointer.alloc(capacity)
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

If the initializer returns ‘nil’ and we still call deinit, we end up destroying a pointer that was never allocated.

If you come from an Objective-C background, you might expect implicit zeroing of the allocated block to help here. However, Swift doesn’t have that, because many Swift types don’t have a “zero” state that’s safe to destroy. For example, anything with a member of non-optional reference type, e.g.,

class ClassWrapper {
  var array: MyArray<String>

  init(array: MyArray<String>) {
    self.array = array
  }

  deinit {
    print(array) // array is a valid instance of MyArray<String>
  }
}

A valid ClassWrapper instance will always have an instance of MyArray<String>, even throughout its deinitializer.

The basic property here is that one cannot run a deinitializer on an instance that hasn’t been fully constructed (all the way up the class hierarchy).

  - Doug

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(David Sweeris) #11

Wouldn’t local variables go away on their own when the function returns? Unless you’re thinking of, say, unhooking yourself from Core Location or something, in which case, yes, you are correct. What if only “@pure” (has that proposal gone through yet?) functions could be called before an early return? Alternately, what if functions that must be “paired” (startMonitoringForRegion, for example) were marked as such? Part of me doesn’t want functions with 20 @whatevers hanging off their declaration, but another (bigger) part of me really wants to see what the language can do when the compiler knows more about what’s going on.

- Dave Sweeris

···

On Jan 27, 2016, at 06:24, Jean-Daniel Dupas via swift-evolution <swift-evolution@swift.org> wrote:

Forcing usage of local variable don’t solve the code duplication issue.
You will still have to replicate the deinit logic in the initializer (to release resources used by temporary local variable instead of member variables, but this is the same).

Le 27 janv. 2016 à 10:26, Kostiantyn Koval via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> a écrit :

In my opinion deinit should be called only for objets that we fully initialized successfully (as it works now).
How about allowing early return only when no properties were initialized.

The possible problem with this solution is that complex initializers would need to store intermediate results in local variables before assigning them to properties.
Example:

struct MyArray<T> {
  var pointer: UnsafeMutablePointer<T>
  var capacity: Int

  init?(capacity: Int) {
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    if capacity > 100 {
      return nil // Error. Can't return nil after partial initialization.
    }
    self.capacity = capacity
  }

  init?(capacity: Int) {
    if capacity > 100 {
      return nil // This is ok, early return before initializing any property
    }
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    self.capacity = capacity
  }
}

Kostiantyn

On 27 Jan 2016, at 02:40, Joseph Lord via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

I think that there is risk here but I don't think it is serious enough one to be with making changes for.

1. For most classes there is no problem because everything is managed by ARC anyway so will clean up without special consideration (few of my classes require a deinit).
2. It doesn't seem surprising that if init did not succeed that deinit will not run.

I suppose the risk is greatest where the nil is a result of a returned value from a call made being passed up as explicit `return nil` or `throw` are more visible.

If something were to be changed I wonder if in cases where a failable init exists in a class with a deinit the init could still be responsible for the clean up but must also call an special empty function to confirm to compiler that the situation has been handled. Not sure on name but maybe `deinited()`. Maybe it would just be a warning if not called. Treatment could maybe be similar to warnings where super calls are not made.

Joseph

On Jan 26, 2016, at 9:39 PM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Absolutely, I agree with you. Of course, this was a simplified example, you can easily construct an example where just reordering isn’t going to cut it.

It’s not hard to do things right in the current model. We can reorder statements, write wrappers, or think of different solutions altogether. Once you understand how it works, it’s very easy to write correct code.

However, my complaint is that it’s now also easy to make a mistake. In the previous model, it wasn’t so easy (the previous model was simpler: an init would always be matched by a deinit).

On Tue, Jan 26, 2016 at 6:39 PM, Douglas Gregor <dgregor@apple.com <mailto:dgregor@apple.com>> wrote:

On Jan 26, 2016, at 9:15 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.

Let’s re-order the statements in your example:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int

    init?(capacity: Int) {
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        pointer = UnsafeMutablePointer.alloc(capacity)
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

If the initializer returns ‘nil’ and we still call deinit, we end up destroying a pointer that was never allocated.

If you come from an Objective-C background, you might expect implicit zeroing of the allocated block to help here. However, Swift doesn’t have that, because many Swift types don’t have a “zero” state that’s safe to destroy. For example, anything with a member of non-optional reference type, e.g.,

class ClassWrapper {
  var array: MyArray<String>

  init(array: MyArray<String>) {
    self.array = array
  }

  deinit {
    print(array) // array is a valid instance of MyArray<String>
  }
}

A valid ClassWrapper instance will always have an instance of MyArray<String>, even throughout its deinitializer.

The basic property here is that one cannot run a deinitializer on an instance that hasn’t been fully constructed (all the way up the class hierarchy).

  - Doug

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Jean-Daniel) #12

The discussed sample is explicitly about a UnsafeMutablePointer.alloc(capacity) call that must be explicitly freed. That why I said local variable wouldn’t improve much the issue.

You can’t restrict the operations that can be done before an early return. How would you write a failable init that return nil when a ressource allocation or access fail (which is far more common than having a @pure function failing) ?

···

Le 27 janv. 2016 à 18:15, Dave via swift-evolution <swift-evolution@swift.org> a écrit :

Wouldn’t local variables go away on their own when the function returns? Unless you’re thinking of, say, unhooking yourself from Core Location or something, in which case, yes, you are correct. What if only “@pure” (has that proposal gone through yet?) functions could be called before an early return? Alternately, what if functions that must be “paired” (startMonitoringForRegion, for example) were marked as such? Part of me doesn’t want functions with 20 @whatevers hanging off their declaration, but another (bigger) part of me really wants to see what the language can do when the compiler knows more about what’s going on.

- Dave Sweeris

On Jan 27, 2016, at 06:24, Jean-Daniel Dupas via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Forcing usage of local variable don’t solve the code duplication issue.
You will still have to replicate the deinit logic in the initializer (to release resources used by temporary local variable instead of member variables, but this is the same).

Le 27 janv. 2016 à 10:26, Kostiantyn Koval via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> a écrit :

In my opinion deinit should be called only for objets that we fully initialized successfully (as it works now).
How about allowing early return only when no properties were initialized.

The possible problem with this solution is that complex initializers would need to store intermediate results in local variables before assigning them to properties.
Example:

struct MyArray<T> {
  var pointer: UnsafeMutablePointer<T>
  var capacity: Int

  init?(capacity: Int) {
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    if capacity > 100 {
      return nil // Error. Can't return nil after partial initialization.
    }
    self.capacity = capacity
  }

  init?(capacity: Int) {
    if capacity > 100 {
      return nil // This is ok, early return before initializing any property
    }
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    self.capacity = capacity
  }
}

Kostiantyn

On 27 Jan 2016, at 02:40, Joseph Lord via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

I think that there is risk here but I don't think it is serious enough one to be with making changes for.

1. For most classes there is no problem because everything is managed by ARC anyway so will clean up without special consideration (few of my classes require a deinit).
2. It doesn't seem surprising that if init did not succeed that deinit will not run.

I suppose the risk is greatest where the nil is a result of a returned value from a call made being passed up as explicit `return nil` or `throw` are more visible.

If something were to be changed I wonder if in cases where a failable init exists in a class with a deinit the init could still be responsible for the clean up but must also call an special empty function to confirm to compiler that the situation has been handled. Not sure on name but maybe `deinited()`. Maybe it would just be a warning if not called. Treatment could maybe be similar to warnings where super calls are not made.

Joseph

On Jan 26, 2016, at 9:39 PM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Absolutely, I agree with you. Of course, this was a simplified example, you can easily construct an example where just reordering isn’t going to cut it.

It’s not hard to do things right in the current model. We can reorder statements, write wrappers, or think of different solutions altogether. Once you understand how it works, it’s very easy to write correct code.

However, my complaint is that it’s now also easy to make a mistake. In the previous model, it wasn’t so easy (the previous model was simpler: an init would always be matched by a deinit).

On Tue, Jan 26, 2016 at 6:39 PM, Douglas Gregor <dgregor@apple.com <mailto:dgregor@apple.com>> wrote:

On Jan 26, 2016, at 9:15 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Now that we can return nil from a failable initializer without having initialized all the properties, it’s easier to make a mistake. For example, consider the following (artificial) code:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        pointer = UnsafeMutablePointer.alloc(capacity)
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

In the `return nil` case, we should really free the memory allocated by the pointer. Or in other words, we need to duplicate the behavior from the deinit.

Before Swift 2.2, this mistake wasn’t possible, because we knew that we could count on deinit being called, *always*. With the current behavior, return `nil` is easier, but it does come at the cost of accidentally introducing bugs. As Joe Groff pointed out, a solution would be to have something like “deferOnError” (or in this case, “deferOnNil”), but that feels a bit heavy-weight to me (and you still have to duplicate code).

In any case, I think it’s nice that we can now return nil earlier. I don’t like that it goes at the cost of safety, but I realize it’s probably only making things less safe in a small amount of edge cases.

Let’s re-order the statements in your example:

class MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int

    init?(capacity: Int) {
        if capacity > 100 {
            // Here we should also free the memory. In other words, duplicate the code from deinit.
            return nil
        }
        self.capacity = capacity
        pointer = UnsafeMutablePointer.alloc(capacity)
    }
    
    deinit {
        pointer.destroy(capacity)
    }
}

If the initializer returns ‘nil’ and we still call deinit, we end up destroying a pointer that was never allocated.

If you come from an Objective-C background, you might expect implicit zeroing of the allocated block to help here. However, Swift doesn’t have that, because many Swift types don’t have a “zero” state that’s safe to destroy. For example, anything with a member of non-optional reference type, e.g.,

class ClassWrapper {
  var array: MyArray<String>

  init(array: MyArray<String>) {
    self.array = array
  }

  deinit {
    print(array) // array is a valid instance of MyArray<String>
  }
}

A valid ClassWrapper instance will always have an instance of MyArray<String>, even throughout its deinitializer.

The basic property here is that one cannot run a deinitializer on an instance that hasn’t been fully constructed (all the way up the class hierarchy).

  - Doug

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(David Sweeris) #13

I don’t think UnsafeMutablePointer.alloc(capacity) qualifies as “pure”, so you’d have to handle it before failing:
init?(capacity: Int) {
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    if capacity > 100 {
        self.pointer.dealloc(capacity)
        return nil // Ok, because everything that has side effects has been dealt with
    }
    self.capacity = capacity
}
or, as in Kostiantyn’s example, bail out before anything with side effects has been done:
init?(capacity: Int) {
    if capacity > 100 {
        return nil // This is ok, we haven’t actually done anything yet
    }
    self.pointer = UnsafeMutablePointer.alloc(capacity)
    self.capacity = capacity
}

Either way, in this case we don’t really lose anything because current system doesn’t prevent this kind of memory leak in the first place. This doesn’t bother the playground at all:
struct MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int) {
        self.pointer = UnsafeMutablePointer.alloc(capacity)
        self.capacity = capacity
        if capacity > 100 {
            return nil // Oops! Memory leak!
        }
    }
    // Will probably leak anyway, since structs don’t have deinits.
}

What if we allowed functions to have where clauses?
struct MyArray<T> {
    var pointer: UnsafeMutablePointer<T>
    var capacity: Int
    
    init?(capacity: Int where capacity <= 100) { // Immediately returns nil if the where clause evaluates to false
        self.pointer = UnsafeMutablePointer.alloc(capacity)
        self.capacity = capacity
    }
    /* OR (note that the init has become non-failable) */
    init(capacity: Int where capacity <= 100) throws { // Immediately throws an error if the where clause evaluates to false
        self.pointer = UnsafeMutablePointer.alloc(capacity)
        self.capacity = capacity
    }
}

I know guard offers similar functionality, but it can appear anywhere in the function body… where must to be up-front, and would get checked before the function body has a chance to do anything. Plus, since it’s part of the function’s signature, it’d appear in API headers. I can’t remember which thread it was in, but as I’ve said before… Which is more useful: a note buried somewhere in the API docs about preconditions, or being able to see the actual bit of code that’ll validate your data?

- Dave Sweeris

···

On Jan 27, 2016, at 10:24, Jean-Daniel Dupas <mailing@xenonium.com> wrote:

The discussed sample is explicitly about a UnsafeMutablePointer.alloc(capacity) call that must be explicitly freed. That why I said local variable wouldn’t improve much the issue.

You can’t restrict the operations that can be done before an early return. How would you write a failable init that return nil when a ressource allocation or access fail (which is far more common than having a @pure function failing) ?