[Deserialization] Output a diagnostic when deserializing an invalid decl

[bnbarham]

The previous change 47b068d was
partially reverted (when not allowing errors) as it output diagnostics
for valid code.

Decl::isInvalid had a special case for AccessorDecl where if its
interface type hadn't already been computed, it would fallback to
checking whether its storage (ie. its VarDecl) was valid. Since the
VarDecl isn't fully deserialized when deserializing the
AccessorDecl, this sometimes causes isInvalid to be true, even
though it isn't actually invalid.

The fallback introduced to isInvalid was to prevent a cycle during
typechecking caused by SimpleDidSetRequest - during didSet
typechecking, the body would be typechecked, which would then check
whether the declaration is valid, calling the typechecking for the
declaration again (and so on).

This change removes the special case from isInvalid. The result of
isInvalid is then correct for AccessorDecl, preventing spurious
diagnostics.

The isInvalid check in performAbstractFuncDeclDiagnostics has been
removed to prevent the cycle there - it works perfectly fine without it.
The unused variable diagnostics are skipped on seeing an error in the
body anyway. The opaque type matching diagnostics make sense to keep
when the return type is valid.

Removing that check exposes another cycle when computing captures in
body typechecking caused by shouldMarkAsObjC checking isInvalid as
well. It already checks for didSet, so have moved that check above the
isInvalid check.

Resolves rdar://74541834

[bnbarham]

I don't really consider this PR fully complete yet - I'm yet to reduce a test case for when the AccessorDecl is invalid when it is actually valid (after actually deserializing everything). It definitely does happen (reproduces in a whole project build) and I've confirmed it works properly after this PR. It seems to involve some combination of generics, opaque types, and @_implementationOnly that I haven't been able to reduce :(.

I've mostly put this up because I'd like some opinions. This is the second hacky-ish branch I've added because of SimpleDidSetRequest typechecking the body (see rdar://70739449 and #34569 for the other). What are your thoughts on not typechecking in the SimpleDidSetRequest, ie. just look for references to oldValue after parsing?

This would be incorrect if eg. there was a local oldValue instead, but this seems like a rare edge case to me that we could introduce a warning for. Unfortunately this would slightly change the semantics - ie. a didSet that previously skipped running the getter, may now run it (if it contained an oldValue that wasn't the didSet parameter). That was the behaviour before the didSet refinement anyway though.

The other solution would be to explicitly require specifying oldValue, but that's a source compatibility breaking change.

[bnbarham]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - e8ada4205342646af6a79c0d6d309e109519f472

[hamishknight]

Out of curiosity, why does performAbstractFuncDeclDiagnostics need to check for AFD->isInvalid() in the first place? I feel like MiscDiagnostics should still be able to run on the function body even if the function's interface type has an error.

[bnbarham]

Out of curiosity, why does performAbstractFuncDeclDiagnostics need to check for AFD->isInvalid() in the first place? I feel like MiscDiagnostics should still be able to run on the function body even if the function's interface type has an error.

I was assuming it was to avoid outputting diagnostics that are because the decl is invalid/a general optimization.

Digging into it, the diagnostics produced by this function are:

• various unused diagnostics (VarDeclUsageChecker), this skips outputting diagnostics on finding errors anyway
• opaque return type checker, which will only run when the return type is an opaque type. This one seems like it may be useful to run even if the decl is invalid (as it only depends on the return type)

So after looking at that... I'd be happy to just remove isInvalid in this case.

[bnbarham]

Test failure is a different cycle caused by didSet type checking -> body type checking -> compute captures -> isObjC -> shouldMarkAsObjC -> isInvalid. Easy enough fix by moving the accessor kind check above the isInvalid, but... I'd still prefer removing the body typechecking.

I've done that fix + removed isInvalid from performAbstractFuncDeclDiagnostics

[bnbarham]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 2d9cdb6

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 2d9cdb6

[bnbarham]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 2d9cdb6

[bnbarham]

Going to close this in favour of another fix (will be up soon).

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 2d9cdb6