Remove `validateDeclForNameLookup` and related logic #24957

xymus · 2019-05-21T19:53:24Z

validateDeclForNameLookup was a phase executed before validateDecl on some decls to avoid cycles. We already moved its work to be lazier, so we don't need this phase anymore. Calling validateDecl is enough as it relies on the new lazy services to avoid cycles.

The next goal is to make lazy more of the logic in validateDecl so it is evaluated only when it is actually needed.

xymus · 2019-05-21T19:53:56Z

@swift-ci Please smoke test

xymus · 2019-05-21T19:54:03Z

@swift-ci Please test source compatibility

slavapestov · 2019-05-23T05:52:09Z

I believe you can remove {LazyResolver,TypeChecker}::resolveProtocolEnvironment() now, too.

xymus · 2019-06-17T21:06:37Z

@swift-ci Please smoke test

xymus · 2019-06-17T21:06:41Z

@swift-ci Please test source compatibility

xymus · 2019-06-18T22:35:07Z

@swift-ci Please test source compatibility

xymus · 2019-06-18T22:39:49Z

@swift-ci Please smoke test

xymus · 2019-06-21T17:28:58Z

@swift-ci Please test source compatibility

xymus · 2019-07-10T16:46:49Z

@swift-ci Please test source compatibility

xymus · 2019-07-10T16:46:55Z

@swift-ci Please smoke test

xymus · 2019-07-24T20:37:19Z

@swift-ci Please smoke test

xymus · 2019-07-24T20:37:25Z

@swift-ci Please test source compatibility

slavapestov

A few suggestions you might want to look at. The only important one is the removal of the requestNominalLayout() call.

lib/Sema/TypeCheckDecl.cpp

slavapestov · 2019-07-24T20:48:30Z

lib/Sema/TypeCheckType.cpp

+  auto aliasDecl = dyn_cast<TypeAliasDecl>(typeDecl);
+  if (resolution.getStage() == TypeResolutionStage::Structural &&
+      aliasDecl && !aliasDecl->getGenericParams() &&
+      !aliasDecl->getUnderlyingTypeLoc().wasValidated()) {


There's a similar check in getStructuralType() in GenericSignatureBuilder.cpp. Maybe TypeAliasDecl::getStructuralType() should automatically return the checked underlying type if its available.

I've applied the suggested change to TypeAliasDecl::getStructuralType. I'll leave this check here because we still want to execute TypeChecker::resolveTypeInContext if the type was previously validated, it provides better diagnostics.

slavapestov · 2019-07-24T20:50:07Z

lib/Sema/TypeChecker.h

  static Type substMemberTypeWithBase(ModuleDecl *module, TypeDecl *member,
-                                      Type baseTy, bool useArchetypes = true);
+                                      Type baseTy, bool useArchetypes = true,
+                                      TypeResolutionStage stage = TypeResolutionStage::Interface);


Maybe if you pass in a TypeResolution instead of a TypeResolutionStage, you can combine the useArchetypes and state parameters into one; useArchetypes then just becomes resolution.usesArchetypes().

lib/Sema/TypeCheckType.cpp

slavapestov · 2019-07-24T20:52:26Z

lib/Sema/TypeCheckType.cpp

-                                      generic);
+  if (!type)
+    type =
+      TypeChecker::resolveTypeInContext(typeDecl, foundDC, resolution,


Should getting the structural type be pushed down into resolveTypeInContext(), since it has access to the resolution object?

We probably could, but we also need to avoid the call to the previous resolveDeclSignature because it triggers validateDecl.

xymus · 2019-07-24T23:32:46Z

@swift-ci Please smoke test

xymus · 2019-07-24T23:32:51Z

@swift-ci Please test source compatibility

xymus · 2019-07-25T18:39:33Z

@swift-ci Please test source compatibility

xymus · 2019-08-01T17:22:42Z

@swift-ci Please smoke test

xymus · 2019-08-01T17:23:10Z

@swift-ci Please test source compatibility

xymus · 2019-08-01T18:47:31Z

@swift-ci Please test source compatibility

xymus · 2019-08-05T17:05:18Z

@swift-ci Please test source compatibility

xymus · 2019-08-05T17:05:26Z

@swift-ci Please smoke test

lib/Sema/TypeCheckGeneric.cpp

slavapestov · 2019-08-09T21:39:28Z

lib/Sema/TypeChecker.h

@@ -1024,7 +1021,12 @@ class TypeChecker final : public LazyResolver {
  void checkDefaultArguments(ParameterList *params, ValueDecl *VD);

  virtual void resolveDeclSignature(ValueDecl *VD) override {
-    validateDeclForNameLookup(VD);
+    auto dc = VD->getDeclContext();
+    if (auto nominal = dyn_cast<NominalTypeDecl>(dc)) {


This should not be necessary?

xymus · 2019-08-09T22:30:35Z

@swift-ci Please smoke test

xymus · 2019-08-09T22:30:43Z

@swift-ci Please test source compatibility

xymus · 2019-08-12T17:03:07Z

@swift-ci Please test source compatibility

It passed the source compat suite, now trying again with one less hack.

xymus · 2019-08-12T17:03:27Z

@swift-ci Please smoke test

xymus · 2019-08-12T17:04:03Z

@swift-ci Please test source compatibility

xymus · 2019-08-12T22:59:38Z

@swift-ci Please smoke test

xymus · 2019-08-12T22:59:46Z

@swift-ci Please test source compatibility

…arly Fixes failures on circular references.

This fixes an issue with protocol compositions using a typealias of anoter protocol composition causing a difference between the flat structural type and the nested underlying TypeRepr.

With the recent change to StructuralTypeRequest where all returned types are wrapped in a TypeAliasType, printing the requirement signature could display requirements such as `Self.P3Assoc == Self.P3Assoc`. This change will make sure that when in debug mode, the underlying type will be displayed instead on the RHS.

xymus · 2019-08-20T23:12:16Z

@swift-ci Please test source compatibility

xymus · 2019-08-20T23:12:19Z

@swift-ci Please smoke test

xymus force-pushed the remove-validateDeclForNameLookup branch from 7a49ec1 to b3f375c Compare June 17, 2019 21:05

xymus force-pushed the remove-validateDeclForNameLookup branch from b3f375c to 6f0b915 Compare June 18, 2019 22:34

xymus force-pushed the remove-validateDeclForNameLookup branch from 6f0b915 to 9d76ff7 Compare June 21, 2019 17:28

xymus force-pushed the remove-validateDeclForNameLookup branch from 9d76ff7 to 761cd67 Compare July 10, 2019 16:46

xymus force-pushed the remove-validateDeclForNameLookup branch 2 times, most recently from 6b4d88f to c715c51 Compare July 24, 2019 20:37

slavapestov approved these changes Jul 24, 2019

View reviewed changes

xymus force-pushed the remove-validateDeclForNameLookup branch from c715c51 to 12598ba Compare July 24, 2019 23:32

xymus force-pushed the remove-validateDeclForNameLookup branch 2 times, most recently from 68dcec4 to 96e4e97 Compare August 1, 2019 17:22

xymus force-pushed the remove-validateDeclForNameLookup branch from 96e4e97 to 3daaeaf Compare August 5, 2019 17:03

slavapestov reviewed Aug 9, 2019

View reviewed changes

lib/Sema/TypeCheckGeneric.cpp Outdated Show resolved Hide resolved

slavapestov reviewed Aug 9, 2019

View reviewed changes

lib/Sema/TypeCheckGeneric.cpp Outdated Show resolved Hide resolved

slavapestov reviewed Aug 9, 2019

View reviewed changes

xymus force-pushed the remove-validateDeclForNameLookup branch from 3daaeaf to a9de890 Compare August 9, 2019 22:30

xymus force-pushed the remove-validateDeclForNameLookup branch from a9de890 to 367ce21 Compare August 12, 2019 17:02

xymus force-pushed the remove-validateDeclForNameLookup branch from 367ce21 to 49e9609 Compare August 12, 2019 22:57

xymus mentioned this pull request Aug 12, 2019

Cleanup to prepare to remove validateDeclForNameLookup #26627

Closed

xymus added 9 commits August 20, 2019 14:58

Move generic signature debugging code that triggered extra work too e…

a268e0c

…arly Fixes failures on circular references.

TypeAliasDecl::getStructuralType returns a validated type if available

da6cf5e

TypeChecker: substMemberTypeWithBase respects structural type validation

d2e3df4

TypeChecker: resolveTypeDecl use the structural type of alias decls

92bec03

Sema: StructuralTypeRequest wraps the resolved type in a TypeAliasType

da4a38c

This fixes an issue with protocol compositions using a typealias of anoter protocol composition causing a difference between the flat structural type and the nested underlying TypeRepr.

Remove validateDeclForNameLookup and related logic

7476430

Remove resolveProtocolEnvironment, it should be no more needed

d81a5d6

Sema: validateDecl validates the context of GenericTypeParamDecls

87be387

xymus force-pushed the remove-validateDeclForNameLookup branch from 49e9609 to 87be387 Compare August 20, 2019 23:11

CodaFi mentioned this pull request Sep 13, 2019

Kill validateDeclForNameLookup Harder #27172

Merged

xymus closed this Sep 23, 2019

Remove validateDeclForNameLookup and related logic #24957

Remove validateDeclForNameLookup and related logic #24957

Uh oh!

Conversation

xymus commented May 21, 2019

Uh oh!

xymus commented May 21, 2019

Uh oh!

xymus commented May 21, 2019

Uh oh!

slavapestov commented May 23, 2019

Uh oh!

xymus commented Jun 17, 2019

Uh oh!

xymus commented Jun 17, 2019

Uh oh!

xymus commented Jun 18, 2019

Uh oh!

xymus commented Jun 18, 2019

Uh oh!

xymus commented Jun 21, 2019

Uh oh!

xymus commented Jul 10, 2019

Uh oh!

xymus commented Jul 10, 2019

Uh oh!

xymus commented Jul 24, 2019

Uh oh!

xymus commented Jul 24, 2019

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slavapestov Jul 24, 2019

Choose a reason for hiding this comment

Uh oh!

xymus Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

slavapestov Jul 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slavapestov Jul 24, 2019

Choose a reason for hiding this comment

Uh oh!

xymus Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

xymus commented Jul 24, 2019

Uh oh!

xymus commented Jul 24, 2019

Uh oh!

xymus commented Jul 25, 2019

Uh oh!

xymus commented Aug 1, 2019

Uh oh!

xymus commented Aug 1, 2019

Uh oh!

xymus commented Aug 1, 2019

Uh oh!

xymus commented Aug 5, 2019

Uh oh!

xymus commented Aug 5, 2019

Uh oh!

Uh oh!

Uh oh!

slavapestov Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

xymus commented Aug 9, 2019

Uh oh!

xymus commented Aug 9, 2019

Uh oh!

xymus commented Aug 12, 2019

Uh oh!

xymus commented Aug 12, 2019

Uh oh!

xymus commented Aug 12, 2019

Remove `validateDeclForNameLookup` and related logic #24957

Remove `validateDeclForNameLookup` and related logic #24957