Warn if a non dynamic class declaration is overridden in an extension #6346
Conversation
|
There are some test failures in CursorInfoTest that I was hoping were a build issue, but still happen. Any thoughts on how to run them without re-running the whole test suite? I scanned through the tests and I'm not sure how I could of broken them, they aren't even operating on classes 🤔 |
|
@KingOfBrian Run |
|
Thanks @slavapestov -- I couldn't find |
| // warn, since it is not added to the class vtable. | ||
| // FIXME: Only warn if the extension is in another module, and if | ||
| // it is in the same module, update the vtable. | ||
| if (auto baseDecl = dyn_cast<ClassDecl>(base->getDeclContext())) { |
There was a problem hiding this comment.
Can you please put the * after the auto. This follows LLVM style:
http://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto
There was a problem hiding this comment.
Great, thanks for the link
| // If the overridden method is declared in a Swift Class Declaration, | ||
| // dispatch will use table dispatch. If the override is in an extension | ||
| // warn, since it is not added to the class vtable. | ||
| // FIXME: Only warn if the extension is in another module, and if |
There was a problem hiding this comment.
Can you put a new comment in between the FIXME and the rest of the comment.
So this:
// Foo
// Bar
// FIXME:
Becomes
// Foo
// Bar
//
// FIXME:
This optimizes the FIXME for the reader who is quickly scanning the file by making it more obvious.
| if (auto *baseDecl = dyn_cast<ClassDecl>(base->getDeclContext())) { | ||
| if (baseDecl->hasKnownSwiftImplementation() && | ||
| !base->isDynamic() && | ||
| override->getDeclContext()->isExtensionContext()) { |
There was a problem hiding this comment.
Can you make this conditional on !TC.Context.isSwiftVersion3(), or turn it into a warning?
To test both the Swift 3 and 4 behavior, have test/decl/inherit/override.swift pass -swift-version 4 in its RUN line, and add a new test/Compatibility/override.swift which tests the Swift 3 behavior.
There was a problem hiding this comment.
Ahh thanks, I did not consider compatibility. I should be able to get a fix up soon.
|
The other fix we'd like to make in this area is to just take extensions in the same module into account when building the vtable. I guess this doesn't make things worse, though; it just means we stop lying about what's working and what's not. |
|
@jrose-apple Just to clarify, implementing the new vtable behavior is not a requirement for this patch going in, is it? I think even after we implement the new SILGen behavior, we'll still want the warning for the case where you're extending a type from another module. |
|
Yes, I agree. It's not a warning but an error anyway. (I suppose we could warn in Swift 3 mode and error in Swift 4 mode.) |
|
Thanks again for the feedback. I went with your suggestion @jrose-apple of generating a warning in swift 3 in the hope that it helps someone. I couldn't figure out how to generate a diagnostic as a warning or an error without adding another entry in |
|
Two duplicate diagnostics are fine in this case. Can you rebase though to kill that merge commit? |
KingOfBrian
force-pushed
the
bugfix/SR-584
branch
from
7b56e03 to
2e1264f
Compare
|
Hey @jrose-apple - I made a naive stab at this and was curious what you thought. This is my first dip into SILGen, and my cpp is still pretty rusty, but I was able to get extensions to override. I still don't understand how this could work for extensions in multiple files though. If this is a tree that you think is worth me barking up, I could make a PR for comments, or bring discussion to swift-dev? |
There was a problem hiding this comment.
The new version looks great. I only have two minor nitpicks.
I had a look at the SILGen vtable change. It looks like a good start. I saw you completely removed the diagnostic, though -- I think we still want it to handle the case where an extension attempts to override a method from another source file or another module.
| @@ -1703,6 +1703,12 @@ ERROR(override_ownership_mismatch,none, | |||
| "cannot override %select{strong|weak|unowned|unowned(unsafe)}0 property " | |||
| "with %select{strong|weak|unowned|unowned(unsafe)}1 property", | |||
| (/*Ownership*/unsigned, /*Ownership*/unsigned)) | |||
| ERROR(override_class_declaration_in_extension,none, | |||
| "cannot override a non-dynamic class declaration from an extension.", | |||
| "cannot override a non-dynamic class declaration from an extension.", | ||
| ()) | ||
| WARNING(override_class_declaration_in_extension_warning,none, | ||
| "cannot override a non-dynamic class declaration from an extension.", |
|
I agree with @slavapestov: this version looks good to go (with the diagnostic nitpicks). We can do the SILGen part later (and probably not for Swift 3.1). |
|
@swift-ci Please smoke test |
|
I can fix up the diagnostic errors on that commit and make another PR. I've been trying to understand when a module refers to a compilation context and when it refers to the Swift module. This made the warning checks a bit more interesting, so I just disabled them while seeing if I could get the SIL to change correctly. Allowing VTable expansion inside one file doesn't seem like much of an improvement. It would be great if the VTable could be expanded inside a swift module, but I think that would force compilation to use whole module optimizations. I'm not sure if forcing WMO to build with swift is in the cards or not though. And without forcing WMO, we'd need to restrict VTable assembly to account for the file when building with WMO so the VTable is the same with and without WMO. Anyway, I'm just thinking these things through, I imagine you guys have a plan here, but it's been fun to tag along. |
|
The plan is to go crawl through all extensions in other files when in single-file mode. That's pretty much it. :-) |
|
Interesting, thanks @jrose-apple! |
|
@KingOfBrian I filed an issue related to this diagnostic. It looks like it is emitted multiple times in some cases: https://bugs.swift.org/browse/SR-4024 |
This will generate a warning if a non-dynamic class declaration is overridden in an extension. When this happens, swift does not update the vtable entry for the class, it just adds the dynamic method. This causes a silent failure when invoking methods via the base class, since the overridden method is not in the vtable.
I updated some Sema unit tests to include this error, since the scenario was tested, and expected to work.
This makes progress on SR-584. I'm investigating the full fix of correctly updating the VTable, if the extension is in the declaring module, but I believe that is in the SILGen code which I haven't touched yet. Any pointers of where to look would be great. I'm going to try to trace into how a
dynamicoverride is able to correctly update the vtable, and see where that goes.I still think that this fix as it stands would be an improvement, as this can still warn the developer at compile time that the override will not work.