Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SR-11679: Incorrect FixIt suggestion for CFTypes #28015

Merged
merged 1 commit into from Nov 6, 2019
Merged

SR-11679: Incorrect FixIt suggestion for CFTypes #28015

merged 1 commit into from Nov 6, 2019

Conversation

nexon
Copy link
Contributor

@nexon nexon commented Nov 1, 2019

Summary

Convenience inits are only allowed on classes and in extensions thereof. And in CFTypes we are not allowed to put convenince inits since swift does not support this feature yet.

This PR solve the bad fix suggestion when a user declare a init method in an extension of CF Type. The simple solution was removing the fix suggestion when the ClassDecl type is CF.

Another bug (similar to this one) that i found and became visible when i was doing the test cases for SR-11679. Was the fact that if you declare a convenience init that delegates the init function in self, a fix suggestion will appear. The same suggestion that is causing troubles in SR-11679. The fix for this was removing the fix suggestion when the class kind type was CFType since if we leave it as is, it will be the same problem as SR-11679.

Copy link
Collaborator

@theblixguy theblixguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you run clang-format and format your changes?

lib/Sema/TypeCheckStmt.cpp Outdated Show resolved Hide resolved
lib/Sema/TypeCheckDecl.cpp Outdated Show resolved Hide resolved
lib/Sema/TypeCheckDecl.cpp Outdated Show resolved Hide resolved
lib/Sema/TypeCheckDecl.cpp Outdated Show resolved Hide resolved
test/decl/init/cf-types.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@theblixguy theblixguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! One last thing - can you squash your commits to a single commit?

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved
lib/Sema/TypeCheckDecl.cpp Outdated Show resolved Hide resolved
Convenience inits are only allowed on classes and in extensions thereof. And in CFTypes we are not allowed to put convenince inits since swift does not support this feature yet.

This PR solve the bad fix suggestion when a user declare a init method in an extension of CF Type.

Another bug (similar to this one) that i found and became visible when i was doing the test cases for SR-116779 was the fact that if you declare a convenience init that delegates the init function in self, a fix suggestion will appear. The same suggestion that is causing troubles in SR-11679. The fix for this was removing the fix suggestion when the class kind type was CFType since if we leave it as is, it will be the same problem as SR-11679.

SR-11679: PR Feedback

Formatting the new code with clang-format and using `isa` and `cast` instead of `dyn_cast`

SR-11679: PR Feedback

Inlining `cast

SR-11679: PR Feedback

Remove semi-colon

SR-11679: PR Feedback

Remove empty line & add mark to avoid fixit.

SR-11679: PR Feedback

Remove empty line.

SR-11679: PR Feedback

Move Diagnostics Info & Remove empty lines
@theblixguy
Copy link
Collaborator

@swift-ci please smoke test

@nexon
Copy link
Contributor Author

nexon commented Nov 2, 2019

Anyone knows why it failed on Linux? Is CI environment related?

@theblixguy
Copy link
Collaborator

@swift-ci please smoke test Linux

@theblixguy theblixguy merged commit 3ecc13b into apple:master Nov 6, 2019
@theblixguy
Copy link
Collaborator

@nexon Thank you for your contribution! Please don't forget to mark the JIRA ticket as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants