Sema: Improve redeclaration error for synthesized inits #24090
Conversation
| @@ -667,6 +667,9 @@ ERROR(reserved_member_name,none, | |||
| " 'foo.%1' expression", (DeclName, StringRef)) | |||
|
|
|||
| ERROR(invalid_redecl,none,"invalid redeclaration of %0", (DeclName)) | |||
| ERROR(invalid_redecl_synth_init,none, | |||
| "invalid redeclaration of synthesized " | |||
| "%select{default|memberwise}1 %0", (DeclName, bool)) | |||
There was a problem hiding this comment.
Do we actually use the term "default initializer" for a no-argument initializer? Unlike in C++, it's not the "default" if you don't initialize something. It might be fine just to leave out the qualifier in that case.
There was a problem hiding this comment.
I tend to call "default" a synthesized no-argument initializer, but I guess we really don't formally have that term in Swift.
There was a problem hiding this comment.
I stand corrected, and it makes sense if it only refers to the synthesized one.
AnthonyLatsis
force-pushed
the
synth-init-conflict-diag
branch
from
64f15f6 to
d383624
Compare
|
So I've gone a bit further with the second commit disabling redeclarations for inherited inits when they conflict with the implicit one. The reason for this is that the redeclaration error here always intersects with a more productive |
lib/Sema/TypeCheckDecl.cpp
Outdated
| // have already taken care of emitting a more productive diagnostic. | ||
| if (isa<ConstructorDecl>(other) && other->isImplicit() && | ||
| other->getOverriddenDecl()) | ||
| continue; |
There was a problem hiding this comment.
I'm concerned by this not requiring any changes to existing tests, and I don't think your new test covers it. What if the inherited initializer is exposed to Objective-C?
There was a problem hiding this comment.
Also, I'm not sure exiting early is a good idea, because we still need to mark one of them invalid.
There was a problem hiding this comment.
Sema, Constraints, decl and Generics pass just fine, but it might affect something elsewhere.
and I don't think your new test covers it. What if the inherited initializer is exposed to Objective-C?
You're right, I'll add some more tests with @objc and dynamic. Right now though it boils down to 'init(arg:)' has already been overridden with and without the change if I plug all the errors.
AnthonyLatsis
force-pushed
the
synth-init-conflict-diag
branch
from
d383624 to
a452995
Compare
This can only happen when the conflicting init is within an extension. It is the override checker's responsibility to emit a sensible diagnostic here.
AnthonyLatsis
force-pushed
the
synth-init-conflict-diag
branch
from
a452995 to
78f4b9d
Compare
|
Concerning the |
|
Unfortunately I think that's legal, but not if the initializers have already been inherited. In this case, though, source compat is probably the deciding factor. What does Swift 5.0 do? |
When I try to override one? P.S. I don't have a fresh build though; this is a February master and Xcode 10.2 beta 2. |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test source compatibility |
|
Thank you for guiding me through this! Just in case there was a misunderstanding: class A {
@objc dynamic init(arg: Int) {}
}
class B: A {}
extension B {
convenience override init(arg: Int) {} // 'init(arg:)' has already been overridden
}I was pondering solutions to the "already overridden" diagnostic (which isn't great in this case), and then realized it might be legal to simply allow the override to follow, since the same can be done to a method. Hence the question earlier /. |
|
Oops. Can you add |
|
@swift-ci Please smoke test |
Resolves SR-8021.
@jrose-apple