[IRGen] Canonicalize symbolic ref types in the right generic context #25955

jrose-apple · 2019-07-03T01:38:05Z

When referencing a superclass type from a subclass, for example, the type uses the subclass's generic parameters, not the superclass's. This can be important if a nested type constrains away some of its parent type's generic parameters.

This doesn't solve all the problems around mis-referenced generic parameters when some are constrained away, though. That might require a runtime change. See the FIXME comments in the test cases (and rdar://problem/52364601).

rdar://problem/51627403

jrose-apple · 2019-07-03T01:38:37Z

lib/IRGen/GenMeta.cpp

@@ -470,6 +470,7 @@ namespace {
    void addExtendedContext() {
      auto string = IGM.getTypeRef(
          E->getSelfInterfaceType()->getCanonicalType(),
+          E->getGenericSignature(),


I wasn't sure about this one, but it might matter with @Azoy's plan for extensions to have their own generic parameters.

I haven't poked around IRGen enough yet, but could you explain why this might affect extensions with there own generic parameters?

Mostly because I'm relatively unfamiliar with it too. :-) The point I was trying to make was that this is probably already in the right form today since the extension can't have introduced any new parameters, but I'm not sure if that'll be true if it can. I think it still is, though.

(The latest change to the runtime, though, #25984, almost certainly is not correct in that world.)

lib/IRGen/GenMeta.cpp

jrose-apple · 2019-07-03T01:39:28Z

lib/IRGen/GenReflection.cpp

@@ -466,7 +470,7 @@ class AssociatedTypeMetadataBuilder : public ReflectionMetadataBuilder {
    for (auto AssocTy : AssociatedTypes) {
      auto NameGlobal = IGM.getAddrOfFieldName(AssocTy.first);
      B.addRelativeAddress(NameGlobal);
-      addTypeRef(AssocTy.second);
+      addTypeRef(AssocTy.second, /*generic signature*/nullptr);


Should this be using the conformance DC's signature?

Good question. If the conformance is a normal conformance, I would think so, yes.

Oof, even more to check.

jrose-apple · 2019-07-03T01:39:57Z

lib/IRGen/GenReflection.cpp

@@ -901,7 +908,7 @@ class CaptureDescriptorBuilder : public ReflectionMetadataBuilder {

    // Now add typerefs of all of the captures.
    for (auto CaptureType : CaptureTypes) {
-      addTypeRef(CaptureType);
+      addTypeRef(CaptureType, /*genericSignature*/nullptr);


Should captures and captured parameters be using a particular signature?

It looks like the capture type was computed with mapTypeOutOfContext(). The result of that is always canonical wrt. the generic signature that the archetype was built from.

jrose-apple · 2019-07-03T01:40:23Z

@swift-ci Please test source compatibility

slavapestov · 2019-07-03T17:21:04Z

lib/IRGen/GenReflection.cpp

@@ -466,7 +470,7 @@ class AssociatedTypeMetadataBuilder : public ReflectionMetadataBuilder {
    for (auto AssocTy : AssociatedTypes) {
      auto NameGlobal = IGM.getAddrOfFieldName(AssocTy.first);
      B.addRelativeAddress(NameGlobal);
-      addTypeRef(AssocTy.second);
+      addTypeRef(AssocTy.second, /*generic signature*/nullptr);


Good question. If the conformance is a normal conformance, I would think so, yes.

slavapestov · 2019-07-03T17:21:29Z

lib/IRGen/GenReflection.cpp

@@ -168,25 +168,29 @@ class PrintMetadataSource
  }
 };

-llvm::Constant *IRGenModule::getTypeRef(CanType type, MangledTypeRefRole role) {
+llvm::Constant *IRGenModule::getTypeRef(CanType type,


May as well relax this to just Type so that callers don't have to call getCanonicalType()

Yeah, I guess so.

The whole point of taking a CanType here, and throughout IRGen, is that the type should already be correctly canonicalized by this point. Are we messing up earlier in the pipeline?

slavapestov · 2019-07-03T17:22:06Z

lib/IRGen/GenReflection.cpp

@@ -901,7 +908,7 @@ class CaptureDescriptorBuilder : public ReflectionMetadataBuilder {

    // Now add typerefs of all of the captures.
    for (auto CaptureType : CaptureTypes) {
-      addTypeRef(CaptureType);
+      addTypeRef(CaptureType, /*genericSignature*/nullptr);


It looks like the capture type was computed with mapTypeOutOfContext(). The result of that is always canonical wrt. the generic signature that the archetype was built from.

jrose-apple · 2019-07-03T17:48:55Z

@swift-ci Please test

jrose-apple · 2019-07-05T21:50:06Z

@jckarter How's this version look?

@swift-ci Please test

swift-ci · 2019-07-05T21:52:14Z

Build failed
Swift Test Linux Platform
Git Sha - 834dc483b7bd53e733aa6517aa0cdfe8987da1d2

swift-ci · 2019-07-05T21:54:05Z

Build failed
Swift Test OS X Platform
Git Sha - 834dc483b7bd53e733aa6517aa0cdfe8987da1d2

jckarter

LGTM, thanks!

jrose-apple · 2019-07-08T16:42:27Z

@swift-ci Please smoke test

When referencing a superclass type from a subclass, for example, the type uses the subclass's generic parameters, not the superclass's. This can be important if a nested type constrains away some of its parent type's generic parameters. This doesn't solve all the problems around mis-referenced generic parameters when some are constrained away, though. That might require a runtime change. See the FIXME comments in the test cases. rdar://problem/51627403

jrose-apple · 2019-07-08T17:30:27Z

@swift-ci Please smoke test

jrose-apple · 2019-07-08T20:41:48Z

@swift-ci Please smoke test macOS

…wiftlang#25955) When referencing a superclass type from a subclass, for example, the type uses the subclass's generic parameters, not the superclass's. This can be important if a nested type constrains away some of its parent type's generic parameters. This doesn't solve all the problems around mis-referenced generic parameters when some are constrained away, though. That might require a runtime change. See the FIXME comments in the test cases. rdar://problem/51627403 (cherry picked from commit 4abefdb)

jrose-apple requested review from slavapestov, jckarter and DougGregor July 3, 2019 01:38

jrose-apple commented Jul 3, 2019

View reviewed changes

slavapestov approved these changes Jul 3, 2019

View reviewed changes

jrose-apple force-pushed the headcanon branch from 834dc48 to 60cd6cb Compare July 5, 2019 21:49

jrose-apple mentioned this pull request Jul 5, 2019

[runtime] Handle same-type constraints when resolving generic params #25984

Merged

jckarter approved these changes Jul 8, 2019

View reviewed changes

jrose-apple force-pushed the headcanon branch from 60cd6cb to 589ea11 Compare July 8, 2019 16:41

jrose-apple force-pushed the headcanon branch from 589ea11 to 8757da3 Compare July 8, 2019 17:30

jrose-apple merged commit 4abefdb into swiftlang:master Jul 8, 2019

jrose-apple deleted the headcanon branch July 8, 2019 21:40

[IRGen] Canonicalize symbolic ref types in the right generic context #25955

[IRGen] Canonicalize symbolic ref types in the right generic context #25955

Uh oh!

Conversation

jrose-apple commented Jul 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jul 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jul 3, 2019

Uh oh!

jrose-apple commented Jul 5, 2019

Uh oh!

swift-ci commented Jul 5, 2019

Uh oh!

swift-ci commented Jul 5, 2019

Uh oh!

jckarter left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jul 8, 2019

Uh oh!

jrose-apple commented Jul 8, 2019

Uh oh!

jrose-apple commented Jul 8, 2019

Uh oh!

Uh oh!