[AST][SILOptimizer]: unify missing return diagnostics in some cases

[jamieQ]

Previously, missing return diagnostics for unreachable subscripts differed from the treatment unreachable functions received, leading to inconsistent diagnostic behavior. This change removes the responsibility for handling the relevant diagnostics from the AST code, in favor of the diagnostics implemented via the SIL optimizer. Additionally, where the AST-generation code would previously have diagnosed a missing return for an implicit empty getter, it will now admit as valid, deferring the missing return diagnostics to the later SIL passes.

Resolves #74210.

[jamieQ]

@hamishknight i took a stab at addressing the issue outlined in #74210. would appreciate feedback if this approach looks like a feasible option if/when you have some time. thanks in advance!

[hamishknight]

Thanks!

[hamishknight]

I suspect we can also remove this branch too since not having a result type means we don't have a TypedPattern, which will be diagnosed in parseDeclVarGetSet (and it looks like parseDeclSubscript always diagnoses a null type and sets an ErrorTypeRepr). This means we ought to be able to inline consumeEmptyBraces into the parsingLimitedSyntax case too.

[jamieQ]

okay, so to clarify what i think is happening & what you're proposing – this branch seems like it was added via #40705 to address this sort of thing:

var x = true {} // error: unexpected '{' in declaration

if i remove the logic, the diagnostics change, but the scenario still appears to be handled in a reasonable manner (to me, anyway). e.g. if the unexpected_curly_braces_in_decl diagnostic path is removed (aside: it looks like this is its only use, so it's another diagnostic that can perhaps be removed entirely), then parsing produces:

var x = true {} // error: computed property must have an explicit type

if we add a type annotation, given the changes proposed in this PR, we successfully parse the code, but typechecking fails with:

var x: Bool = true {} // error: variable with getter/setter cannot have an initial value

so given the other suggested branch removal, the final logic for this conditional would be something like:

1. if we're parsing in the limited syntax mode, consume the empty braces
2. otherwise, parse an implicit getter
3. in both cases, return a parser success

locally this appeared to pass tests other than the affected error message change mentioned above. if this sounds accurate to you, i'll try to clean up this code and update the tests.

[hamishknight]

Yeah that seems reasonable to me. If we wanted to improve things further, we could add a special-cased diagnostic for when you both have an initializer and an empty accessor body (regardless of whether you have a type); since in that case it's more likely than not the user is intending to write a didSet/willSet accessor.

[hamishknight]

Nit: There's no missing return expectation for this case :)

[jamieQ]

sorry, i don't think i follow... i added similar code to the missing_returns tests but this formulation is now valid if the result builder protocol is satisfied. there's no expectation b/c this is now 'fixed'. seemed like it should be moved there though since this test doesn't do the SIL pass. should this be worded differently, or am i misunderstanding?

[hamishknight]

Sorry yeah I just meant this should be worded differently since the issue isn't a lack of return

[hamishknight]

These tests were only added to check the diagnostic, so I think we can remove them here in favor of the ones in the SILOptimizer test

[jamieQ]

noticed this wording change when moving to the SIL-based diagnostics. i have yet to determine how to special case the relevant logic to improve this. let me know if this seems like a blocker.

[hamishknight]

I don't think it's a blocker, it's the same diagnostic we give today for e.g:

struct S {
  subscript(_ x: Int) -> Never {
    let x = 0
  }
}

[hamishknight]

@swift-ci please test