Improve diagnostics when trying to extend existential type #60680
Conversation
cbjeukendrup
force-pushed
the
diag_unsupported_existential_extension
branch
from
4e445a8 to
e35f8d1
Compare
cbjeukendrup
force-pushed
the
diag_unsupported_existential_extension
branch
2 times, most recently
from
0ca8393 to
c7d4878
Compare
| @@ -3042,21 +3042,31 @@ class DeclChecker : public DeclVisitor<DeclChecker> { | |||
| const bool wasAlreadyInvalid = ED->isInvalid(); | |||
| ED->setInvalid(); | |||
| if (!extType->hasError() && extType->getAnyNominal()) { | |||
| auto canExtType = extType->getCanonicalType(); | |||
| if (auto existential = canExtType->getAs<ExistentialType>()) { | |||
There was a problem hiding this comment.
You can call getAs() on a non-canonical type, it will automatically desugar it, so you don’t need to move the canonical type computation from below
There was a problem hiding this comment.
Using auto existential = extType->getAs<ExistentialType>() instead still gives a difference. The comment for the fixit in the test case becomes "did you mean to extend 'A4' (aka 'P4') instead?", while it will still replace the type with just P4. Wouldn't it be better if the comment just says "did you mean to extend 'P4' instead?"?
Or do we want that the fixit replaces the type with A4 in this case?
There was a problem hiding this comment.
I personally think it's better for the error message to mention 'A4' (aka 'P4') because A4 is what's written in the extension, so it helps the programmer make the connection between A4 and P4. However, I don't feel too strongly about this and I'm fine with merging this change as-is.
|
@swift-ci Please smoke test |
|
@slavapestov I'm sorry, it turns out I hadn't pushed my latest changes yet, awaiting your opinion on #60680 (comment). I'll push them in a few minutes, after running the tests locally once more! |
cbjeukendrup
force-pushed
the
diag_unsupported_existential_extension
branch
from
c7d4878 to
d47bba6
Compare
|
@swift-ci Please test |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test macOS |
|
Since we got approval ⛵ |
|
Thank you for the contribution @cbjeukendrup! |
@hborla Thanks for the review! By the way, I also have another open PR, namely #58545. Would be great if you could have a look at that one too! |
Resolves: #60595