Kill validateDeclForNameLookup Harder #27172

CodaFi · 2019-09-13T18:19:18Z

Using the techniques from @xymus' patch, try to kill validateDeclForNameLookup harder. There are some changes from the exact code in #24957 that, unfortunately, I cannot get to pass the tests to land independently. Namely, this patch

Defines a new UnderlyingTypeRequest for computing the interface type of the underlying type if we have it.
Changes the structural type request to resugar with a TypeAliasType. I suspect this is what is causing the changes in requirement signatures in the few tests that are touched here.
Separate computing the interface type of a TypeAliasDecl from computing its underlying type. Not doing this makes reasoning about the laziness in UnderlyingTypeRequest interacting with the interface type of the typealias extremely difficult.

All of this should finally let us remove validateDeclForNameLookup. I have great fears that this will fail the compatibility suite. But there's only one way to find out!

CodaFi · 2019-09-13T18:19:48Z

The canary in the coal mine:

@swift-ci please smoke test

CodaFi · 2019-09-13T18:26:40Z

I have high hopes for this patch. If it actually works then we can merge the lazy generic signatures patch minus a ton of the hacks.

CodaFi · 2019-09-13T18:28:18Z

Oh joy, I need LLDB patches.

slavapestov · 2019-09-13T18:28:39Z

lib/Sema/TypeCheckGeneric.cpp

+  // to compute the structural type.  This also prevents us from writing
+  // ErrorTypes into otherwise valid ASTs with generated typealiases.
+  if (typeAlias->hasInterfaceType()) {
+    return typeAlias->getInterfaceType()->getMetatypeInstanceType();


This should use getDeclaredInterfaceType()

slavapestov · 2019-09-13T18:29:56Z

lib/AST/GenericSignatureBuilder.cpp

-      return resolved;
-
+    if (typealias->hasInterfaceType())
+      return typealias->getDeclaredInterfaceType();
    return typealias->getStructuralType();


Doesn't getStructuralType() subsume the above 'if' now?

slavapestov · 2019-09-13T18:30:31Z

lib/AST/NameLookup.cpp

-        if (auto type = typealias->getUnderlyingTypeLoc().getType()) {
-          if (type->isAnyObject())
-            anyObject = true;
+        if (typealias->hasInterfaceType()) {


The hasInterfaceType() check should not be necessary?

Requesting the underlying type potentially calls back into validation. This is also justified because the interface type separation changes mean that AnyObject declarations will always have their interface type set by the time we see them.

slavapestov · 2019-09-13T18:31:48Z

lib/Sema/TypeCheckDecl.cpp

-               : extendedNominal->getDeclaredType();
+      // Nested Hack to break cycles if this is called before validation has
+      // finished.
+      if (aliasDecl->hasInterfaceType()) {


Instead of checking for hasInterfaceType(), validate the alias to make this deterministic

This is here because we're validating the alias when this is false...

slavapestov · 2019-09-13T18:32:13Z

lib/Sema/TypeCheckType.cpp

+                    dyn_cast<TypeAliasDecl>(unboundGeneric->getAnyGeneric())) {
+              if (ugAliasDecl == aliasDecl) {
+                if (resolution.getStage() == TypeResolutionStage::Structural &&
+                    !aliasDecl->hasInterfaceType()) {


Shouldn't need this check

slavapestov · 2019-09-13T18:32:19Z

lib/Sema/TypeCheckType.cpp

+                  dyn_cast<TypeAliasType>(extendedType.getPointer())) {
+            if (aliasType->getDecl() == aliasDecl) {
+              if (resolution.getStage() == TypeResolutionStage::Structural &&
+                  !aliasDecl->hasInterfaceType()) {


slavapestov · 2019-09-13T18:32:26Z

lib/Sema/TypeCheckType.cpp

-      // Otherwise, simply return the underlying type.
+      // Otherwise, return the appropriate type.
+      if (resolution.getStage() == TypeResolutionStage::Structural &&
+          !aliasDecl->hasInterfaceType()) {


slavapestov · 2019-09-13T18:32:48Z

lib/Sema/TypeCheckType.cpp

-    selfType = resolution.mapTypeIntoContext(
-      foundDC->getDeclaredInterfaceType());
+    selfType =
+        resolution.mapTypeIntoContext(foundDC->getDeclaredInterfaceType());


There's a lot of re-formatting here which makes the patch harder to review. Is it really necessary?

slavapestov · 2019-09-13T18:33:39Z

lib/Sema/TypeCheckType.cpp

-      !isa<NominalTypeDecl>(typeDecl)) {
+  if ((!options.is(TypeResolverContext::ExtensionBinding) ||
+       !isa<NominalTypeDecl>(typeDecl)) &&
+      !prevalidatingAlias(typeDecl, resolution)) {
    // Validate the declaration.
    if (lazyResolver)
      lazyResolver->resolveDeclSignature(typeDecl);


You could try removing this altogether, and validating the decl only once you know you need the interface type.

There's some deep cycles at work that break the standard library if this is removed.

slavapestov · 2019-09-13T18:36:27Z

Once getGenericSignature() is a request, we can request-ify getDeclaredInterfaceType() on a type alias decl.

Supports swiftlang/swift#27172

CodaFi · 2019-09-13T18:44:26Z

apple/swift-lldb#1986
@swift-ci Please smoke test

CodaFi · 2019-09-13T20:34:29Z

~~Looks like changing the requirement signature has changed the way the exportability checker behaves. Looking into this.~~

Apparently, validateType is needed for UnderlyingTypeRequest to emit the right diagnostics.

CodaFi · 2019-09-13T23:07:03Z

apple/swift-lldb#1986
@swift-ci Please smoke test

CodaFi · 2019-09-14T01:03:23Z

apple/swift-lldb#1986
@swift-ci Please smoke test

CodaFi · 2019-09-14T01:03:36Z

apple/swift-lldb#1986
@swift-ci Please smoke test

slavapestov · 2019-09-14T01:39:12Z

@CodaFi I see there's a few diagnostic checks in validateType(). They should be pushed down to resolveType() so that we emit the same diagnostics everywhere; validateType() should just be a transitional utility method that writes the Type back into a TypeLoc. But you don't have to fix that now.

Let's look at those cycles involving validateDecl() next -- we'll have to resolve them once getInterfaceType() becomes a request, because now cycles will produce diagnostics instead of silently bailing out of validation.

CodaFi · 2019-09-14T02:07:19Z

@swift-ci please test

swift-ci · 2019-09-14T02:10:00Z

Build failed
Swift Test Linux Platform
Git Sha - 3f9738bbf13afb06aaf18d3137dfe511f45867dd

CodaFi · 2019-09-14T02:10:21Z

apple/swift-lldb#1986
@swift-ci Please test

CodaFi · 2019-09-14T02:10:47Z

apple/swift-lldb#1986
@swift-ci Please test source compatibility

CodaFi · 2019-09-14T03:12:16Z

Sent the wrong CI command here. Gonna try again when macOS fails.

Supports swiftlang/swift#27172

CodaFi · 2019-09-14T05:39:10Z

apple/swift-lldb#1986
@swift-ci Please test

CodaFi · 2019-09-14T08:30:48Z

apple/swift-lldb#1986
@swift-ci Please test source compatibility

Define a request for computing the interface type of the underlying type of a typealias. This can be used in place of the declared interface type of the alias itself when it is nested to preserve the fragile validation ordering issues that can cause.

Structural type computation ought to actually return a typealias type here using the same mechanism we compute the interface type with (minus the metatype).

Dump the underlying type by desugaring ourselves a little.

Computing the interface type of a typealias used to push validation forward and recompute the interface type on the fly. This was fragile and inconsistent with the way interface types are computed in the rest of the decls. Separate these two notions, and plumb through explicit interface type computations with the same "computeType" idiom. This will better allow us to identify the places where we have to force an interface type computation. Also remove access to the underlying type loc. It's now just a cache location the underlying type request will use. Push a type repr accessor to the places that need it, and push the underlying type accessor for everywhere else. Getting the structural type is still preferred for pre-validated computations. This required the resetting of a number of places where we were - in many cases tacitly - asking the question "does the interface type exist". This enables the removal of validateDeclForNameLookup

Flush it and the early validation hack now that we can delay computing the underlying interface type on demand and have taught type resolution to honor the structural type of a typealias. This changes the way requirement signatures are spelled as a side effect.

This avoids making the structural type dependent on whether the interface type has been computed and neatly avoids special-casing non-parsed declarations which set their underlying type up front.

Use the interface type of the underlying type in the GSB in as many cases as possible except for one important one: requirement signature computation. There, use the structural type so we don't wind up recursively validating the requirements of an incomplete protocol.

CodaFi · 2019-09-18T01:43:38Z

Canary

apple/swift-lldb#1986
@swift-ci Please smoke test

CodaFi · 2019-09-18T02:40:44Z

apple/swift-lldb#1986
@swift-ci Please test

CodaFi · 2019-09-18T02:40:54Z

apple/swift-lldb#1986
@swift-ci Please test source compatibility

swift-ci · 2019-09-18T02:45:34Z

Build failed
Swift Test OS X Platform
Git Sha - f87c9781ac1edc1933ca877e06c19b5ede99fb70

swift-ci · 2019-09-18T02:45:44Z

Build failed
Swift Test Linux Platform
Git Sha - f87c9781ac1edc1933ca877e06c19b5ede99fb70

CodaFi · 2019-09-18T05:08:57Z

Ugh, the same set of offenders as last time...

Not having the generic signature is the real culprit here.

CodaFi · 2019-09-18T18:24:55Z

apple/swift-lldb#1986
@swift-ci Please test

CodaFi · 2019-09-18T18:25:04Z

apple/swift-lldb#1986
@swift-ci Please test source compatibility

Supports swiftlang/swift#27172

CodaFi · 2019-09-18T19:17:22Z

apple/swift-lldb#1986
@swift-ci Please test Linux platform

CodaFi · 2019-09-18T19:19:30Z

apple/swift-lldb#1986
@swift-ci Please test macOS platform

CodaFi · 2019-09-18T22:00:39Z

⛵️

slavapestov

Nice work @xymus and @CodaFi! Really happy to see this land.

slavapestov · 2019-09-18T22:57:39Z

lib/AST/ASTDumper.cpp

@@ -616,9 +616,9 @@ namespace {
    void visitTypeAliasDecl(TypeAliasDecl *TAD) {
      printCommon(TAD, "typealias");
      PrintWithColorRAII(OS, TypeColor) << " type='";
-      if (TAD->getUnderlyingTypeLoc().getType()) {
+      if (auto underlying = TAD->getUnderlyingType()) {


This kicks off a request :(

I'm going to add an RAII type to catch all of these.

slavapestov · 2019-09-18T22:58:20Z

lib/AST/GenericSignatureBuilder.cpp

-      return resolved;
-
-    return typealias->getStructuralType();
+	// When we're computing requirement signatures, the structural type


It looks like the indentation here might be a bit funky

slavapestov · 2019-09-18T22:58:26Z

lib/AST/GenericSignatureBuilder.cpp

+	// requirements.
+	auto *proto = dyn_cast_or_null<ProtocolDecl>(typealias->getDeclContext()->getAsDecl());
+	if (proto && proto->isComputingRequirementSignature())
+		return typealias->getStructuralType();


slavapestov · 2019-09-18T23:00:01Z

lib/Sema/TypeCheckDecl.cpp

-    validateTypealiasType(*this, typeAlias);
+
+    // Finally, set the interface type.
+    if (!typeAlias->hasInterfaceType())


Is it ever the case that hasInterfaceType() returns true here? If you copy and pasted this from the old code it's just an artifact of validateDecl() being called after validateDeclForNameLookup()

It was true when the code was written - something something cycles with witness matching.

slavapestov · 2019-09-18T23:01:10Z

lib/Sema/TypeCheckGeneric.cpp

@@ -556,7 +556,7 @@ static Type formExtensionInterfaceType(
  }

  // If we have a typealias, try to form type sugar.
-  if (typealias && TypeChecker::isPassThroughTypealias(typealias)) {
+  if (typealias && TypeChecker::isPassThroughTypealias(typealias, typealias->getUnderlyingType(), nominal)) {


This line is too long

slavapestov · 2019-09-18T23:02:17Z

lib/Serialization/Deserialization.cpp


+    auto underlying = MF.getType(underlyingTypeID);
+    alias->setUnderlyingType(underlying);
+    SubstitutionMap subs;


Is this not alias->computeType()?

- Clean up some errant formatting mistakes. - Collapse some code that was duplicating computing the interface type.

CodaFi requested review from slavapestov and xymus September 13, 2019 18:19

slavapestov reviewed Sep 13, 2019

View reviewed changes

CodaFi added a commit to CodaFi/swift-lldb that referenced this pull request Sep 13, 2019

Switch to the new underlying type API

3fa9039

Supports swiftlang/swift#27172

CodaFi mentioned this pull request Sep 13, 2019

Switch to the new underlying type API apple/swift-lldb#1986

Merged

CodaFi force-pushed the aliasing-artifacts-and-noise-reduction-techniques branch from 3a52ab0 to 82b6101 Compare September 13, 2019 23:06

CodaFi force-pushed the aliasing-artifacts-and-noise-reduction-techniques branch from 82b6101 to f87c978 Compare September 14, 2019 01:03

swiftlang deleted a comment from swift-ci Sep 14, 2019

CodaFi added a commit to CodaFi/swift-lldb that referenced this pull request Sep 14, 2019

Switch to the new underlying type API

0cdf9bc

Supports swiftlang/swift#27172

swiftlang deleted a comment from swift-ci Sep 14, 2019

CodaFi force-pushed the aliasing-artifacts-and-noise-reduction-techniques branch from c2e4bbb to f87c978 Compare September 16, 2019 18:38

CodaFi added 6 commits September 17, 2019 08:19

Define UnderlyingTypeRequest

2ab5ea8

Define a request for computing the interface type of the underlying type of a typealias. This can be used in place of the declared interface type of the alias itself when it is nested to preserve the fragile validation ordering issues that can cause.

Resugar the structural type

278ead3

Structural type computation ought to actually return a typealias type here using the same mechanism we compute the interface type with (minus the metatype).

Improve the TypeAlias{Type|Decl} dump

dceb357

Dump the underlying type by desugaring ourselves a little.

Setting the underlying type should set the structural type too

c254f4d

This avoids making the structural type dependent on whether the interface type has been computed and neatly avoids special-casing non-parsed declarations which set their underlying type up front.

CodaFi force-pushed the aliasing-artifacts-and-noise-reduction-techniques branch from f87c978 to c254f4d Compare September 17, 2019 15:49

Break some cycles

6b7fbc9

Use the interface type of the underlying type in the GSB in as many cases as possible except for one important one: requirement signature computation. There, use the structural type so we don't wind up recursively validating the requirements of an incomplete protocol.

Change a cycle condition

4f6951c

Not having the generic signature is the real culprit here.

swiftlang deleted a comment from swift-ci Sep 18, 2019

CodaFi added a commit to CodaFi/swift-lldb that referenced this pull request Sep 18, 2019

Switch to the new underlying type API

bb7576c

Supports swiftlang/swift#27172

CodaFi merged commit 38bde33 into swiftlang:master Sep 18, 2019

CodaFi deleted the aliasing-artifacts-and-noise-reduction-techniques branch September 18, 2019 22:00

slavapestov reviewed Sep 18, 2019

View reviewed changes

CodaFi added a commit to CodaFi/swift that referenced this pull request Sep 18, 2019

[NFC] Address review feedback from swiftlang#27172

bddb2c7

- Clean up some errant formatting mistakes. - Collapse some code that was duplicating computing the interface type.

Kill validateDeclForNameLookup Harder #27172

Kill validateDeclForNameLookup Harder #27172

Uh oh!

Conversation

CodaFi commented Sep 13, 2019

Uh oh!

CodaFi commented Sep 13, 2019

Uh oh!

CodaFi commented Sep 13, 2019

Uh oh!

CodaFi commented Sep 13, 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!

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!

slavapestov commented Sep 13, 2019

Uh oh!

CodaFi commented Sep 13, 2019

Uh oh!

CodaFi commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Sep 13, 2019

Uh oh!

CodaFi commented Sep 14, 2019

Uh oh!

CodaFi commented Sep 14, 2019

Uh oh!

slavapestov commented Sep 14, 2019

Uh oh!

CodaFi commented Sep 14, 2019

Uh oh!

swift-ci commented Sep 14, 2019

Uh oh!

CodaFi commented Sep 14, 2019

Uh oh!

CodaFi commented Sep 14, 2019

Uh oh!

CodaFi commented Sep 14, 2019

Uh oh!

CodaFi commented Sep 14, 2019

Uh oh!

CodaFi commented Sep 14, 2019

Uh oh!

CodaFi commented Sep 18, 2019

Uh oh!

CodaFi commented Sep 18, 2019

Uh oh!

CodaFi commented Sep 18, 2019

Uh oh!

swift-ci commented Sep 18, 2019

Uh oh!

swift-ci commented Sep 18, 2019

Uh oh!

CodaFi commented Sep 18, 2019

Uh oh!

CodaFi commented Sep 18, 2019

CodaFi commented Sep 13, 2019 •

edited

Loading