[GSB] Eliminate concrete potential archetypes by early substitution #14965

xedin · 2018-03-04T12:20:33Z

To make generic signature builder more robust it's imperative to
eliminate possibility of out-of-order typealias substitution.

These changes try to make it so potential archetypes could only be
constructed with associated types by doing early concrete type (typealias)
subsitution while trying to resolve equivalence class.

xedin · 2018-03-04T12:25:19Z

/cc @DougGregor could you please take a look? Spent some time working on this over the weekend to remove the hack for typealias we have in place now...

It seems like it can detect more situations where typealias makes generic parameters 'non-generic' e.g.

protocol P {
  typealias A = Int
}

func foo<T: P, U>(_ a: T, _ b: U) where T.A == U {}

is going to produce error: same-type requirement makes generic parameter 'U' non-generic as this code does without my changes:

func foo<U>(_ a: U) where U == Int {}

DougGregor

This is looking really promising! I'd like us to think about that loop over the "other" declarations found with the same name (in maybeResolvedEquivalenceClass), and will comment on the new error separately.

DougGregor · 2018-03-05T06:24:27Z

lib/AST/GenericSignatureBuilder.cpp

+  if (!concreteDecl->hasInterfaceType())
+    builder.getLazyResolver()->resolveDeclSignature(concreteDecl);
+
+  if (concreteDecl->hasInterfaceType()) {


Nit: I suggest doing an early return if !concreteDecl->hasInterfaceType() to de-nest the main code path.

Sure! I confess that I have a habit of moving the code around without really refactoring it all the way...

DougGregor · 2018-03-05T06:25:07Z

lib/AST/GenericSignatureBuilder.cpp

+static Type substituteConcreteType(GenericSignatureBuilder &builder,
+                                   PotentialArchetype *basePA,
+                                   TypeDecl *concreteDecl) {
+  if (!concreteDecl)


Will we ever call it with a null concreteDecl? It seems like this should be an assert.

Yeah, makes sense!

DougGregor · 2018-03-05T06:28:12Z

lib/AST/GenericSignatureBuilder.cpp

-      // introduce various same-type constraints.
-      for (auto concreteDecl : concreteDecls) {
-        (void)basePA->updateNestedTypeForConformance(*this, concreteDecl,
-                                                     resolutionKind);


Huh. I'm surprised we can get rid of this. It's supposed to establish equivalences between same-named nested types.

To be honest I'm just not sure what it's going to do now because we substitute early which means that we'd end up with new potential archetype that has associated type so we don't really need to add new same-type requirement like before where we had potential archetype with concrete decl and substituted typealias type. But I might be totally missing something here, could you please provide an example?

Huh. I had convinced myself that this mattered due to #14974, but my change there didn't fix the problem at all. Maybe this is indeed dead code!

I suspect the deletion of this loop is the cause of the regression in ./validation-test/compiler_crashers_2_fixed/0075-rdar30248571.swift.

DougGregor · 2018-03-05T06:30:00Z

lib/AST/GenericSignatureBuilder.cpp

-         target->getParent() &&
-         sourceDepMemTy->getAssocType() &&
-         sourceDepMemTy->getAssocType() == target->getResolvedAssociatedType()){
+         target->getParent() && sourceDepMemTy->getAssocType() &&


Oh, I don't think you need to check for a null sourceDepMemTy->getAssocType(), because the "subject" of any constraint should (now) always be a well-formed type. I bet there are other places where we have such null checks that could go away with your PR!

Right, good catch! Thanks!

DougGregor · 2018-03-05T06:35:17Z

That same-type requirement makes generic parameter 'U' non-generic error might cause us some source-compatibility trouble (particularly with IndexDistance recently changing to Int), but we can assess the impact before we make any changes.

DougGregor · 2018-03-05T06:36:17Z

Here's an assertion I have lying around that I think we can introduce into your PR, which would ensure that unresolved dependent types never slip into a generic function type. It's worth a shot!

diff --git a/lib/Sema/TypeCheckGeneric.cpp b/lib/Sema/TypeCheckGeneric.cpp
index 5763586508..7c93d90f0c 100644
--- a/lib/Sema/TypeCheckGeneric.cpp
+++ b/lib/Sema/TypeCheckGeneric.cpp
@@ -866,7 +866,15 @@ TypeChecker::validateGenericFuncSignature(AbstractFunctionDecl *func) {
   }
 
   configureInterfaceType(func, sig);
-  return sig;
+
+  assert(func->getInterfaceType()->hasError() ||
+         !func->getInterfaceType().findIf([](Type type) {
+    if (auto depMemTy = type->getAs<DependentMemberType>()) {
+      return depMemTy->getAssocType() == nullptr;
+    }
+    return false;
+  }));
+return sig;
 }
 
 void TypeChecker::configureInterfaceType(AbstractFunctionDecl *func,

xedin · 2018-03-05T08:49:45Z

@DougGregor You've already added findUnresolvedDependentMemberType as a method on TypeBase so I'm just going to use that in the assert :)

xedin · 2018-03-06T06:06:23Z

@swift-ci please smoke test

xedin · 2018-03-06T07:01:10Z

@DougGregor Could you please take a look at the failures - especially new error message in ./validation-test/compiler_crashers_2_fixed/0075-rdar30248571.swift and see that makes sense?

xedin · 2018-03-06T21:49:55Z

@DougGregor Thanks! I'll take a look, maybe I should get it back and substitute all of them the same way as typealiases, will try that later today.

xedin · 2018-03-07T04:50:29Z

@swift-ci please smoke test macOS platform

xedin · 2018-03-07T05:18:46Z

@DougGregor looks like your changes for nested typealiases have fixed validation test, I am going to fix up code completion one and run source compatibility suite.

To make generic signature builder more robust it's imperative to eliminate possibility of out-of-order typealias substitution. These changes try to make it so potential archetypes could only be constructed with associated types by doing early concrete type (typealias) subsitution while trying to resolve equivalence class.

xedin · 2018-03-07T07:35:28Z

@swift-ci please smoke test

xedin · 2018-03-07T08:05:58Z

@swift-ci please test source compatibility

xedin · 2018-03-07T08:30:26Z

@swift-ci please test

xedin · 2018-03-07T10:51:10Z

@DougGregor Looks like everything is green, going to merge in the morning.

DougGregor · 2018-03-07T16:47:05Z

Fantastic!

DougGregor · 2018-03-07T16:47:48Z

lib/Sema/TypeCheckGeneric.cpp

+
+  // Make sure that there are no unresolved
+  // dependent types in the generic signature.
+  assert(func->getInterfaceType()->hasError() ||


Very excited that this assertion is here now!

xedin requested a review from DougGregor March 4, 2018 12:20

xedin force-pushed the potential-archetype-only-assoc branch from bfa046a to a1fb120 Compare March 4, 2018 12:42

DougGregor reviewed Mar 5, 2018

View reviewed changes

xedin force-pushed the potential-archetype-only-assoc branch from a1fb120 to 2be67ef Compare March 5, 2018 08:43

xedin force-pushed the potential-archetype-only-assoc branch from 2be67ef to d0c533f Compare March 5, 2018 08:57

xedin force-pushed the potential-archetype-only-assoc branch from d0c533f to 76fc9f7 Compare March 7, 2018 07:08

xedin force-pushed the potential-archetype-only-assoc branch from 76fc9f7 to 4b05449 Compare March 7, 2018 07:34

DougGregor approved these changes Mar 7, 2018

View reviewed changes

xedin merged commit e226c20 into swiftlang:master Mar 7, 2018

[GSB] Eliminate concrete potential archetypes by early substitution #14965

[GSB] Eliminate concrete potential archetypes by early substitution #14965

Uh oh!

Conversation

xedin commented Mar 4, 2018

Uh oh!

xedin commented Mar 4, 2018

Uh oh!

DougGregor left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Mar 5, 2018

Uh oh!

DougGregor commented Mar 5, 2018

Uh oh!

xedin commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Mar 6, 2018

Uh oh!

xedin commented Mar 6, 2018

Uh oh!

xedin commented Mar 6, 2018

Uh oh!

xedin commented Mar 7, 2018

Uh oh!

xedin commented Mar 7, 2018

Uh oh!

xedin commented Mar 7, 2018

Uh oh!

xedin commented Mar 7, 2018

Uh oh!

xedin commented Mar 7, 2018

Uh oh!

xedin commented Mar 7, 2018

Uh oh!

DougGregor commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xedin commented Mar 5, 2018 •

edited

Loading