Skip to content

[Mangling] Add a new mangling for opaque return type to use when mangling an ObjC runtime name #33035

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

Merged
merged 3 commits into from
Aug 5, 2020

Conversation

theblixguy
Copy link
Collaborator

@theblixguy theblixguy commented Jul 21, 2020

The old remangler does not handle opaque result types and can crash when mangling an ObjC runtime name for a class that is in a local context that returns an opaque type, for example:

func opaqueTypeFunc() -> some MyProtocol {
  class Foo: MyProtocol { ... }
  return Foo()
}

var opaqueTypeVar: some MyProtocol {
  class Bar: MyProtocol { ... }
  return Bar()
}

Introduce a new Qu mangling to represent the opaque return type in old mangling.

Resolves SR-13203

@theblixguy theblixguy requested a review from jckarter July 21, 2020 21:43
@jckarter
Copy link
Contributor

I think we need to implement a real mangling here. The ObjC runtime name can become load-bearing ABI, if the class name ends up being used in a keyed archiver, nib, or other serialized format, and it needs to be unique relative to any other class names that might be instantiated in the process.

@theblixguy theblixguy changed the title [ASTMangler] Swap opaque result types with error types when mangling an ObjC runtime name [Mangling] Add a new mangling for opaque return type to use when mangling an ObjC runtime name Jul 29, 2020
@theblixguy
Copy link
Collaborator Author

theblixguy commented Jul 29, 2020

@jckarter, I have used a new Qu mangling to represent the return type in the old remangler. Does that seem in line with what you had in mind? (Additionally, do we need a new one if we can reuse Qr here)?

Given we don't allow overloading with opaque return types, I think the new mangled name should be completely unique. Is my assumption correct?

@jckarter
Copy link
Contributor

I think that should work.

@theblixguy
Copy link
Collaborator Author

@jckarter Great! Thank you! I'll add this to Mangling.rst as well.

@theblixguy
Copy link
Collaborator Author

@jckarter I have added it to Mangling.rst. Does everything look good to you?

@jckarter
Copy link
Contributor

jckarter commented Aug 3, 2020

Looks good to me. Thanks!

@theblixguy
Copy link
Collaborator Author

@swift-ci please test

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

@theblixguy
Copy link
Collaborator Author

@swift-ci please test

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

@theblixguy
Copy link
Collaborator Author

@swift-ci please test Linux

@theblixguy theblixguy merged commit 3502e07 into swiftlang:master Aug 5, 2020
@theblixguy theblixguy deleted the fix/SR-13203 branch August 5, 2020 04:03
@slavapestov
Copy link
Contributor

This needs an executable test in test/Interpreter/ which prints the runtime name. Otherwise we don't know if the mangling works as specified or not.

@theblixguy
Copy link
Collaborator Author

@slavapestov Sure, I have added some new tests here: #33340

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.

4 participants