[SR-12421] Incorrect/unhelpful error when incorrectly calling return in failable initializer

[swift-ci]

Previous ID	SR-12421
Radar	None
Original Reporter	pilky (JIRA User)
Type	Bug
Status	In Progress
Resolution

Environment

Xcode 11.4 (11E146)

Additional Detail from JIRA

Votes	0
Component/s
Labels	Bug, StarterBug
Assignee	hassaneldesouky (JIRA)
Priority	Medium

md5: 50d6e39b9624d565f57eea856086a4de

Issue Description:

If you create a failable initialiser and accidentally add a simple return rather than return nil when you want to fail, the Compiler gives an error at the start of the init about a property not being initialised.

Ideally this should detect that you have done a simple return rather than a return nil and show an error at that line, ideally with a fixit to add the nil back in

class TestClass {
    let label: String
    init?(label: String?) { //Gives "Constant 'self.label' used before being initialized"
        guard let actualLabel = label else {
            return //Actual error is a missing nil here
        }
        self.label = actualLabel
    }
}

[swift-ci]

Comment by Hassan ElDesouky (JIRA)

pilky (JIRA User), @xedin

1. Is this TypeCheckStmt.cpp#L500 the right place in which I should diagnose this? I tried adding an if statement but it didn't work, so I guess there's more to it than just checking if `ctor->isFailable()`
2. Will I need to remove the old incorrect error message and if so how?

[LucianoPAlmeida]

hassaneldesouky (JIRA User) As far as I could see the incorrect error is already removed on master, and this is compiling correctly, which is not the expected behavior I think ...
So what you could try to do is before TypeCheckStmt.cpp#L454 and a check like if is a constructor decl and failable diagnose it. Maybe provide a fix to add `nil` too 🙂

[swift-ci]

Comment by Hassan ElDesouky (JIRA)

Thank you @LucianoPAlmeida I realized after posting the comment that I have to do it before TypeCheckStmt.cpp#L454.

[LucianoPAlmeida]

No problem! But one thing I just noticed is that this code should compile because when a result stmt has no result it means that it is a `return;` so is valid.
So if you can confirm that this example compiles on master, it is fixed I think 🙂

[swift-ci]

Comment by Hassan ElDesouky (JIRA)

PR: #30707

[LucianoPAlmeida]

pilky (JIRA User) Although this message may seem confusing is it the expected behavior because you can return; early on failable initializers but all members should be initialized in all paths, and that just doesn't happen in the guard early return of your example. So what the error is trying to say is that you should initialize label member before return; on the guard statement.

guard let actualLabel = label else {
   self.label = ''" // Label should be initialized before return
   return // It's allowed, but don't fail the init just return early 
}
self.label = actualLabel

Maybe it should say something like `self.label should be initialized in all branches` ... but I'm not sure
cc @slavapestov

[swift-ci]

Comment by Martin Pilkington (JIRA)

@LucianoPAlmeida The case I gave is just the simplest case to reproduce the issue. Often you will have multiple guard statements making it easier to mistype, and make having to initialise all the other properties more complex. There is also the problem that the reason for having a failable initialiser is you don't want the properties to be set and a value to be returned as it would be in an invalid state.

While the error is technically correct, ideally all compiler errors should point to and explain the actual problem. Firstly, for any initialiser I would expect the error to be on the line with the return statement of the branch where one or more properties aren't set rather than the method definition. And in the case of a failable initialiser I would expect the primary error/fix-it to be that that the user probably wanted to type return nil rather than a plain return, as that is the more likely error. It's worth noting that once all properties are set in a failable initialiser then a plain return would not be an error (it could still be a bug in the user's code if they actually wanted to return nil, but not one the compiler should be expected to detect).