Fix a Hole in Extension Checking For Non-Nominal Types and Indirectly Nominal Types #26970
Conversation
CodaFi
changed the title
CodaFi
force-pushed
the
extension-winding
branch
from
f4e70a5 to
0f9d085
Compare
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
| .highlight(ED->getExtendedTypeRepr()->getSourceRange()); | ||
| ED->diagnose(diag::invalid_nominal_extension_rewrite, canExtType) | ||
| .fixItReplace(ED->getExtendedTypeRepr()->getSourceRange(), | ||
| canExtType->getString()); |
There was a problem hiding this comment.
We don't usually allow a bound generic type here. Can you try a few cases and see if the fixit makes sense?
test/decl/ext/extensions.swift
Outdated
| @@ -153,3 +153,13 @@ extension DoesNotImposeClassReq_2 where Self : AnyObject { | |||
| set { property = newValue } // Okay | |||
| } | |||
| } | |||
|
|
|||
| // Reject extension of nominal type via indirection through a typealias | |||
There was a problem hiding this comment.
Type aliases are fine -- the problematic case here is a typealias whose underlying type is a type parameter. We can't resolve it at extension binding time without a conformance check.
lib/Sema/TypeCheckDecl.cpp
Outdated
| if (!ED->getGenericParams() && | ||
| !ED->isInvalid()) { | ||
| ED->diagnose(diag::extension_nongeneric_trailing_where, | ||
| nominal->getFullName()) |
There was a problem hiding this comment.
Nitpick: Indentation is off by one here
There was a problem hiding this comment.
One day I will fix Xcode's C++ support...
|
Looks great other than a few small suggestions, thanks! |
If getExtendedNominal() fails but getExtendedType() succeeds, we need to diagnose otherwise we end up with a bogus extension not bound to anything.
If the canonical type is a nominal type then we can offer a diagnostic that rewrites to it. Resolves rdar://54799560
CodaFi
force-pushed
the
extension-winding
branch
from
0f9d085 to
5968751
Compare
|
@swift-ci please smoke test |
| @@ -50,14 +50,6 @@ extension K { | |||
| func replacement_finalFunction() {} | |||
| } | |||
|
|
|||
| extension undeclared { // expected-error{{use of undeclared type 'undeclared'}} | |||
There was a problem hiding this comment.
Instead of deleting the test case, just update the expected-errors
There was a problem hiding this comment.
There aren't any meaningful errors in either file. We just diagnose the extension now.
There was a problem hiding this comment.
My point still stands. We want to make sure we don’t regress and start crashing here. It’s testing a specific case where the extension has a missing type and a nested type is inside it.
| @@ -8,12 +8,12 @@ extension G { | |||
| } | |||
| } | |||
|
|
|||
| extension { // expected-error {{expected type name in extension declaration}} | |||
| struct S<T> { | |||
| extension G { | |||
There was a problem hiding this comment.
Instead of changing the test case, just update the expected-errors
|
⛵️ |
Address review feedback on swiftlang#26970
Relates to #26967. We were allowing a narrow class of extensions of trickily-constructed nominal types that involved some kind of indirection through a typealias to slip through our semantic checks. Diagnose these, and offer an error in the cases where we know we can rewrite the underlying type to a valid nominal type.
See also rdar://54799560, whose general pattern is reproduced in the added test.