[Sema] Diagnose missing 'nil' in failable initializer return

[HassanElDesouky]

If you create a failable initializer and accidentally add a simple return rather than return nil, will show an error and provide a fix to add nil to the return statement.

Resolves SR-12421.

[HassanElDesouky]

@xedin @LucianoPAlmeida Please, review.

[LucianoPAlmeida]

@HassanElDesouky As I mentioned in the last comment on the issue, I think this is already correct...
Because when a ReturnStmt has no result !RS->hasResult() it is a return or an implicit return, so this would be also valid inside a failable init.
Here an example:

init?(label: String?) { 
         guard let actualLabel = label else {
             return nil
         }

         self.label = actualLabel
         return // You could add an explicit return
     }

init?(label: String?) { 
         guard let actualLabel = label else {
             return nil
         }

         self.label = actualLabel
     } // There is an implicit return statement here I think

In both examples the code is valid :)
So in my opinion, if the incorrect message from the SR is fixed on master and this is already fixed. And you could just add the test case here
cc @xedin

[HassanElDesouky]

I'm sorry I don't quite understand. So you are suggesting to just remove all of this and a test case?
If so... which diagnostic this should be diagnosed to?

[HassanElDesouky]

@LucianoPAlmeida I don't have a master branch, the master branch isn't compiling on Ubuntu. You can check the error message here Therefore, I'm on an older branch.

[LucianoPAlmeida]

Sorry, just edited the previous comment, but what I mean is that I think this code is already correct if it compiles and it shouldn't be diagnosed because is valid code like the examples. So it shouldn't be diagnosed.
And if this code compiles on master, I think the issue is already fixed :)

[HassanElDesouky]

I understand now, thank you! However, what do you mean by "you could just add the test case here"?

[theblixguy]

Yeah, I agree with @LucianoPAlmeida. This is something that should be handled in SIL diagnostics and not the typechecker. Anyway, this case is already handled by DefiniteInitialization:

// swiftc -emit-sil
/Users/suyashsrijan/Desktop/test.swift:3:5: error: constant 'self.label' used before being initialized
    init?(label: String?) {
    ^

as described in the JIRA ticket, but I don't think we can assume that every return must be a return nil (as shown in the examples above), so it might be a bit tricky to detect the right "exits" where a return nil is the right answer.

[pilky]

I would argue that in a failable initialiser any return before all properties are initialised should be an error (at the return, not at the init, as that is where the error actually is). I would also argue that the compiler should provide a fix-it to convert it to a return nil as the user forgetting the nil part is the most likely cause of the error (and is something the current error message and location don't actually explain in any way)

[LucianoPAlmeida]

so it might be a bit tricky to detect the right "exits" where a return nil is the right answer

Agree @theblixguy, I was thinking in something like tweak the wording of diagnostic message for cases like this to something like 'self.label' should be initialized on all paths/branches..., when there is no explicit reference to the member in at least one of the paths ... maybe this could make it more clear.

[LucianoPAlmeida]

I would also argue that the compiler should provide a fix-it to convert it to a return nil as the user forgetting the nil part is the most likely cause of the error

Humm, since in failable inits that's the most commom way people use return it makes sense

[xedin]

I agree with @pilky. It seems like the problem here is that return without nil should be diagnosed in situations when it's used before all of the properties are initialized. Which seems like it belongs in SIL diagnostics.

[shahmishal]

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

• How to change the base branch: (Link)
• More detail about the branch update: (Link)