Retain type sugar for extension declarations that name generic typealiases #15450

DougGregor · 2018-03-23T07:04:24Z

When extending a type via a generic typealias, where the type parameters of
the underlying nominal type line up precisely with those of the
generic typealias and its specialization of the underlying nominal
type (a so-called "pass-through" typealias in the new code), maintain
type sugar in the extension declaration.

This new type sugar enables inference of type requirements from the
generic typealias, which is both useful by itself (it lets the type
requirements on generic typealiases be meaningful for extensions like
they are elsewhere), and also addresses a source-compatability
regression where an extension of CountableRange will now infer the
requirement Bound: Comparable.

Fixes SR-6907 / rdar://problem/29066394.

…iases. When extending a type via a generic typealias, where the type parameters of the underlying nominal type line up precisely with those of the generic typealias and its specialization of the underlying nominal type (a so-called "pass-through" typealias in the new code), maintain type sugar in the extension declaration. This new type sugar enables inference of type requirements from the generic typealias, which is both useful by itself (it lets the type requirements on generic typealiases be meaningful for extensions like they are elsewhere), and also addresses a source-compatability regression where an extension of `CountableRange` will now infer the requirement `Bound: Comparable`. Fixes SR-6907 / rdar://problem/29066394.

DougGregor · 2018-03-23T07:04:31Z

@swift-ci please smoke test

DougGregor · 2018-03-23T07:04:40Z

@swift-ci please test source compatibility

slavapestov · 2018-03-23T07:06:48Z

test/Compatibility/CountableRange.swift

+
+struct RequiresComparable<T: Comparable> { }
+
+extension CountableRange { // expected-warning{{'CountableRange' is deprecated: renamed to 'Range'}}


Can you test 'extension DictionaryIndex' too?

This function was unable to "look through" an unbound generic typealias to find the underlying nominal type that was getting extended, which can occur if we end up performing a name lookup into this context before the extension itself has been fully-checked. Also, test that extending DictionaryIndex works.

DougGregor · 2018-03-23T17:28:09Z

@swift-ci please smoke test

DougGregor · 2018-03-23T17:28:16Z

@swift-ci please test source compatibility

The optimization that avoided forming new generic signatures for extensions of nominal types was kicking in too eagerly, preventing us from inferring requirements from the typealias.

DougGregor · 2018-03-23T18:57:56Z

@swift-ci please smoke test

DougGregor · 2018-03-23T18:58:10Z

@swift-ci please smoke test

DougGregor · 2018-03-23T18:58:21Z

@swift-ci please smoke test

DougGregor · 2018-03-23T18:59:43Z

@swift-ci please test source compatibility

DougGregor · 2018-03-23T18:59:51Z

@swift-ci please test source compatibility

DougGregor · 2018-03-23T20:02:39Z

@swift-ci please smoke test

DougGregor · 2018-03-23T20:02:49Z

@swift-ci please smoke test

DougGregor · 2018-03-23T20:05:01Z

@swift-ci please smoke test

DougGregor · 2018-03-23T20:27:11Z

Linux failure is weird:

15:17:41 CMake Error at CMakeLists.txt:90 (message):
15:17:41   LLDB test compilers not specified.  Tests will not run
15:17:41 
15:17:41 
15:17:41 -- Configuring incomplete, errors occurred!

lattner

LGTM, but I don't have this stuff paged into my head anymore.

lattner · 2018-03-25T04:09:13Z

lib/AST/DeclContext.cpp

+  auto type = ext->getExtendedType();
+  if (!type) return nullptr;
+
+  do {


Trivial style point, but a while(true) is arguably better than do/while(true) because you can know that it is an infinite loop reading the code top to bottom.

Yeah, sure.

lattner · 2018-03-25T04:10:45Z

lib/Sema/TypeCheckDecl.cpp

+  // Check that the type parameters are the same the whole way through.
+  auto nominalGenericParams = nominalSig->getGenericParams();
+  auto typealiasGenericParams = typealiasSig->getGenericParams();
+  if (nominalGenericParams.size() != typealiasGenericParams.size())


Checking the sizes match is redundant with the subsequent std::equal call.

Unfortunately, it isn't, because std::equal is terrible API that only takes the "first" iterator to the second sequence, assuming that one has already performed the size check ahead of time!

Oh man. 🙄🙄

Well, there's the variant that takes the second end-iterator too…

lattner · 2018-03-25T04:11:10Z

lib/Sema/TypeCheckDecl.cpp

+  // a passthrough typealias.
+  auto innermostGenericParams = typealiasSig->getInnermostGenericParams();
+  auto boundArgs = boundGenericType->getGenericArgs();
+  if (boundArgs.size() != innermostGenericParams.size())


lattner · 2018-03-25T04:12:08Z

lib/Sema/TypeCheckDecl.cpp

+  auto nominal = dyn_cast<NominalTypeDecl>(genericDecl);
+  auto typealias = dyn_cast<TypeAliasDecl>(genericDecl);
+  if (!nominal) {
+    Type underlyingType = typealias->getUnderlyingTypeLoc().getType();


Are you sure that genericDecl is either a typealias or a nominaltype decl? If it is possible for it to be neither of those, then this code will crash with a null dereference.

I am, because the only forms of type declaration we have in the language are "nominal" and "alias". I suspect that distinction will last "forever", because any new kind of type we invent will either have a name and a runtime presence (nominal) or be some form of alias.

Ok, sounds good. please add an assertion :)

DougGregor · 2018-03-25T06:56:59Z

Okay, this one is actually good to go. The GRDB source-compatibility failure can't be addressed in a meaningful way. @lattner , I have another commit coming to fix the while(true) issue, but it's in a follow-on PR.

groue · 2018-03-26T07:04:29Z

The GRDB source-compatibility failure can't be addressed in a meaningful way.

I feel sorry (GRDB author here). Is there anything I can do to help?

DougGregor · 2018-03-26T17:02:16Z

@groue actually, my comment was incorrect and I was confused! GRDB has provided a fantastic example here, and I'd missed that I needed to update the standard library to take advance of the compiler work in this pull request. The standard library change at #15513 gets GRDB passing again!

groue · 2018-03-26T17:07:26Z

That's a relief, thank you Doug! Because the work that had to be done to make it pass was everything but trivial :-)

DougGregor · 2018-03-26T17:16:33Z

You gave me an excuse to refactor something that I've felt is wrong/broken for ~2 years, so thanks!

DougGregor requested review from lattner and slavapestov March 23, 2018 07:04

slavapestov reviewed Mar 23, 2018

View reviewed changes

[Type checker] When we sugar an extension type, infer requirements.

6707214

The optimization that avoided forming new generic signatures for extensions of nominal types was kicking in too eagerly, preventing us from inferring requirements from the typealias.

Update one last test for extensions of generic typealiases.

0c9fb62

lattner reviewed Mar 25, 2018

View reviewed changes

DougGregor merged commit 82ea1dc into swiftlang:master Mar 25, 2018

DougGregor deleted the infer-ext-generic-typealias branch March 25, 2018 06:57

huonw mannequin mentioned this pull request Mar 28, 2018

[SR-7209] Source compatibility regression: error: ambiguous reference to member 'min(by:)' #49757

Closed


		struct RequiresComparable<T: Comparable> { }

		extension CountableRange { // expected-warning{{'CountableRange' is deprecated: renamed to 'Range'}}

Retain type sugar for extension declarations that name generic typealiases #15450

Retain type sugar for extension declarations that name generic typealiases #15450

Uh oh!

Conversation

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

DougGregor commented Mar 23, 2018

Uh oh!

lattner 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 25, 2018

Uh oh!

groue commented Mar 26, 2018

Uh oh!

DougGregor commented Mar 26, 2018

Uh oh!

groue commented Mar 26, 2018

Uh oh!

DougGregor commented Mar 26, 2018

Uh oh!

Uh oh!