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

Remove Fix-it suggestion for unsafeBitcast #40523

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RAJAGOPALAN-GANGADHARAN
Copy link
Contributor

Remove Fix-it suggestion for unsafeBitcast to change it into unsafeDowncast. The compiler suggest this but on applying the fix it fails at debugPrecondition(x is T). The options were either to remove fix it entirely or apply only when we know x is T for sure. Tried to find if x is T but not able to get this data at this step of the pipeline (the dynamic type casting/ mimicking is operator using swiftDynamicCast seems to happen during runtime). Hence opted for entirely removing the fix it

Resolves SR-14943.

Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

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

This probably needs updates on the existing tests for this diagnostic and maybe the addition of the ticket example(minimized if possible) to the tests :)

@@ -818,15 +818,6 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
return;
}
}

// Unchecked casting to a subclass is better done by unsafeDowncast.
if (fromTy->isBindableToSuperclassOf(toTy)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I read the description and the opting for removing the diagnostic all together, but I still wonder if it would be better to still emit this diagnostic when we and only when we know that we have both (from and to) concrete types. In that case we can know is x is T statically and safely emit the diagnostic and fix-it.

That is normally how those runtime diagnostics warnings are emitted, only when compiler can know that information at compile time, so in the example, generic parameters there is no way to know that at compile time.
To me it makes sense to just extend the check and still emit diagnostic when it can know for sure, but is just a personal opinion, since was stated in the ticket that removing diagnostic was also fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 to leaving it in place, but restricting it to when we have sufficient information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @LucianoPAlmeida and @compnerd for the suggestions, It makes sense to do that. I will try to find way to apply it for concrete types atleast.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like what we want here is to check that both sides are ClassDecl and that "from" class declaration is a subclass of "to" declaration via ClassDecl::sSuperclassOf(ClassDecl *)

@RAJAGOPALAN-GANGADHARAN
Copy link
Contributor Author

Guys pardon me for posting this here but would be really helpful if someone could help me resolve this issue - https://forums.swift.org/t/swift-ios-build-error-due-to-sdk-versions/54373 , Thanks

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

4 participants