Sema: Fix assertion failure when overriding a dynamic method with a final method [4.0]

[slavapestov]

• Description: Fixes an assertion failure when overriding an imported method (or property) with a final method (or property).

• Scope of the issue: Affects builds with assertions on, like downloadable snapshots.

• Origination: The underlying bug has been there forever I think, but became easier to hit when overrides of imported methods started to be marked 'dynamic' as part of the Swift 3/4 mix-and-match support.

• Risk: Low. The change has been in the master branch for a while now with no apparent fallout.

• Tested: New tests added.

• Reviewed by: @jrose-apple

[slavapestov]

@swift-ci Please test

[slavapestov]

@jrose-apple I thought I cherry-picked this into 4.0 already but it looks like I didn't. Mind reviewing?

[jrose-apple]

On second thought, I don't think this is correct. If a base method is dynamic, that could mean the author expects it to be replaced at run-time. That means the overriding method has to be dynamic as well.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - f2f9e2e

[slavapestov]

@jrose-apple However, the override in question is explicitly 'final'. I think we should probably disallow final overrides of explicitly-defined 'dynamic' methods altogether, but in this case, the method was implicitly 'dynamic' because it was imported. Since a customer is hitting this issue with a snapshot toolchain, I'd like to check this in as-is. If you feel that perfecting the semantics is more important than the other items on your plate right now, you're welcome to take over.

[slavapestov]

@swift-ci Please test Linux

[jrose-apple]

Given that there's a workaround—remove final—I'm inclined to say that it'd be better to only allow this if the dynamic comes from an imported declaration, and to actually produce the warning in other cases. Otherwise, we would have run-time behavior that doesn't match the documented behavior of dynamic. @ematejska or @tkremenek may have a different opinion on the relative priorities here, though.

[slavapestov]

Having a workaround is not a good enough reason not to fix a crash. Users don't always know what the workaround is.

[DougGregor]

Personally, I think avoiding the crash here is the right thing to do for 4.0. LGTM for 4.0

Outvoted by Doug and Slava.