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

[Mangling] Several fixes for de/re/-mangling of generic typealiases #17509

Merged
merged 7 commits into from Jun 27, 2018

Conversation

DougGregor
Copy link
Member

@DougGregor DougGregor commented Jun 26, 2018

Introduce several fixes for mangling and remangling of generic typealiases:

  • Mangle the generic arguments of a typealias type directly, rather than (incorrectly) mangling the generic arguments of the underlying nominal type, and also account for nested generic typealiases.
  • Remangle generic typealiases the same way we remangle generic nominal types
  • Aside: fix remangling of protocol references (we were missing substitutions)
  • Type reconstruction: guard against demangling the wrong # of generic arguments. This is a hack to avoid crashing; a more complete fix requires deeper changes to the type reconstruction logic.

Should fix rdar://problem/41444286

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@@ -11,3 +11,4 @@ $SSayypXpG ---> Array<Any.Type>
$S12EyeCandyCore11XPCListenerC14messageHandleryyAA13XPCConnectionV_AA10XPCMessageVxtcvpfiyAF_AHxtcfU_TA.4 ---> Can't resolve type of $S12EyeCandyCore11XPCListenerC14messageHandleryyAA13XPCConnectionV_AA10XPCMessageVxtcvpfiyAF_AHxtcfU_TA.4
$Ss10CollectionP7ElementQa ---> Can't resolve type of $Ss10CollectionP7ElementQa
$S12TypeReconstr8patatinoayAA5tinkyVGSgD ---> Optional<patatino>
$S12TypeReconstr5OuterV3Fooayx_SiGD ---> Can't resolve type of $S12TypeReconstr5OuterV3Fooayx_SiGD
Copy link
Member

Choose a reason for hiding this comment

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

Why is this invalid?

Copy link
Member Author

Choose a reason for hiding this comment

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

"This is a hack to avoid crashing; a more complete fix requires deeper changes to the type reconstruction logic."

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to do the thing where we pull out all of the generic argument lists from the "parent" type and put them together.

return;

auto *genericTypeAlias =
cast<TypeAliasDecl>(generic_type_result._decls.front());
GenericSignature *signature = genericTypeAlias->getGenericSignature();
if (template_types_result._types.size() != signature->getGenericParams().size())
Copy link
Member

Choose a reason for hiding this comment

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

This could (maybe) set an error instead of silently returning (result.error).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

@@ -254,6 +257,10 @@ int main(int argc, char **argv) {
Invocation.setModuleName("lldbtest");
Invocation.getClangImporterOptions().ModuleCachePath = ModuleCachePath;

if (!ResourceDir.empty()) {
Invocation.setRuntimeResourcePath(ResourceDir);
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this for testing? [If so, can you add a test for this].

Copy link
Member Author

Choose a reason for hiding this comment

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

It's for debugging: I often use a Debug compiler with a standard library that was build with a Release+Asserts compiler, because the latter builds the standard library so much faster. Testing doesn't need this.

Copy link
Member

Choose a reason for hiding this comment

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

OK, makes sense, thanks!

Copy link
Member

@dcci dcci left a comment

Choose a reason for hiding this comment

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

I don't know mangling well enough to comment but the type reconstruction changes look reasonable to me (modulo minors).

We need to look through a Type node before trying the standard
substitutions.
The mangling of generic typealiases was using the underlying type’s generic
arguments rather than the generic arguments for the typealias itself.
Directly encode the generic arguments from the substitution map instead.

Also address some related issues with remangling generic typealiases.

Fixes rdar://problem/41444286.
…ealias.

When we are attempting to reconstruct a type node for a generic typealias,
make sure that we get the right number of generic arguments. If not,
fail gracefully rather than crashing.

Eventually, we should be able to handle this nested case.
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@swift-ci please smoke test and merge

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 1e5155d into apple:master Jun 27, 2018
@DougGregor DougGregor deleted the mangle-generic-typealias branch June 27, 2018 01:04
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