Fix typealias used in associatedtype constraint #27987

zoecarver · 2019-10-31T03:46:16Z

This patch fixes the compiler crash when type typealias are used in associatedtype constraints. For example:

protocol ProtocolA {
    associatedtype T1
}

struct S : ProtocolA {
  typealias T1 = Int
}

protocol ProtocolB: ProtocolA {
    associatedtype T2: ProtocolB where T2.T1 == T3.T1
    associatedtype X
    typealias T3 = S
}

Resolves SR-11639.

Let me know if I put my tests in the right place.

CodaFi · 2019-10-31T16:35:45Z

lib/AST/GenericSignatureBuilderImpl.h

@@ -100,6 +100,9 @@ class GenericSignatureBuilder::ResolvedType {
  UnresolvedType getUnresolvedType() const {
    return type;
  }
+
+  /// Retrieve the stored pointer union
+  llvm::PointerUnion<PotentialArchetype *, Type> getType() { return type; }


Nit: You don't need this. You can use getUnresolvedType.

I don't think that will work because I need a way to check if the union is currently holding a Type or a PotentialArchetype *.

UnresolvedType is a PointerUnion of those types.

using UnresolvedType = llvm::PointerUnion<PotentialArchetype *, Type>;

Ah, you're right. I was looking at the wrong method.

CodaFi · 2019-10-31T16:37:59Z

lib/AST/GenericSignatureBuilder.cpp

@@ -3652,6 +3652,9 @@ ResolvedType GenericSignatureBuilder::maybeResolveEquivalenceClass(
                                   resolutionKind,
                                   wantExactPotentialArchetype);
    if (!resolvedBase) return resolvedBase;
+    if (resolvedBase.getType().is<Type>())


I'm kind of surprised this works. It would stomp the condition below, right?

It's evaluated before the below condition so, yes.

I mean, I’m surprised that this doesn’t break code because of that.

I'm also a little surprised this didn't break anything. It should be noted that it's checking the base type, not type. Then, when the condition succeeds, it will fail out to handleUnresolvedRequirement.

Is the Type here Null or have an ErrorType in it?

As the bots showed, this does cause issues. I'll look into it in more depth in the morning.

test/decl/nested/protocol.swift

CodaFi · 2019-11-04T01:03:49Z

Let's see what happens

@swift-ci please smoke test

CodaFi · 2019-11-04T01:04:00Z

@swift-ci please test source compatibility

zoecarver · 2019-11-04T01:06:48Z

Thanks @CodaFi!

zoecarver · 2019-11-06T03:10:37Z

lib/AST/GenericSignatureBuilder.cpp

@@ -3652,8 +3652,11 @@ ResolvedType GenericSignatureBuilder::maybeResolveEquivalenceClass(
                                   wantExactPotentialArchetype);
    if (!resolvedBase) return resolvedBase;
    // If the base is concrete, so is this member.
-    if (resolvedBase.getAsConcreteType())
+    if (resolvedBase.getAsConcreteType()) {
+      if (type->isTypeParameter())


This still seems like a bit of a hack to me. Generic parameter types are handled above but, it feels like this type parameter type should never make it here (I have no basis for thinking that, though). Could someone with more knowledge of this bit of code chime in, maybe?

cc @jckarter based on git blame

I don't recall, sorry. If you can remove it and it still works, then by all means remove it.

No worries. It's not removing anything as much as conditionally returning an unresolved nullptr. It seems to work, though, so it's probably OK.

zoecarver · 2019-11-06T04:08:57Z

@CodaFi I've run some of the relevant tests and verified that it can compile the standard library. Could you trigger smoke test again?

CodaFi · 2019-11-06T23:54:15Z

Aha, I think I see the problem here. We're better about resugaring Types with TypeAliases. The isTypeParameter() predicate doesn't check for that, which would explain how we managed to get a type parameter to slip in as a concrete type. Ignoring this failure will make the crash go away, but the right answer is to patch up all the places that are asking that question to account for the type alias case. Ultimately, I think this code should compile - it seems to in 4.2.

I think @AnthonyLatsis has the more principled fix (or at least the start of one) in #27425

zoecarver · 2019-11-07T00:39:39Z

@CodaFi great! That would explain it. Do you think I should update this patch to fix getAsConcreteType, submit another patch for it, or let it be done in #27425?

CodaFi · 2019-11-07T00:53:21Z

If you can make use of some of the primitives in Anthony's patch to fix this up, that'd be great.

zoecarver · 2019-11-07T00:57:01Z

Sounds good. I'll do that.

AnthonyLatsis · 2019-11-07T03:46:02Z

@CodaFi Is equivClass->concreteType always expected to return a fully concrete type – would Concrete<Assoc> be valid output?

zoecarver · 2019-11-08T17:42:35Z

@CodaFi I spent a bit of time trying to implement parts of Anthony's type substitution in this patch but I couldn't get anything working well. Would you mind pointing me in the right direction?

AnthonyLatsis · 2019-11-08T18:39:55Z

Just had a closer look and realized that I recently fixed a similar untracked issue alongside #23489. A cherry-pick confirms it serendipitously fixes this too. The crasher I unpatched is described here, and the fix is 3c87252

The problem was that a non-generic typealias inside a protocol was always resolved as a DependentMemberType at the structural stage over here, so the type-checker had no clue it was actually dealing with a TypeAliasType.

AnthonyLatsis · 2019-11-08T20:58:42Z

@zoecarver Could you cherry-pick your compiler_crashers test on top if I isolate the fix in a new PR?

zoecarver · 2019-11-09T01:28:44Z

@AnthonyLatsis sure. Thanks for all the links and explanation. I'll see if I can use that to fix the issue.

AnthonyLatsis · 2019-11-09T01:42:25Z

Was there a misunderstanding? 3c87252 fixes this issue alongside another one I had, so I proposed to isolate the fix together with your test case now that we actually have an independent one (my own test case depends on the feature implemented in the linked PR).

zoecarver · 2019-11-09T01:50:31Z

Ah, I see — my misunderstanding. I thought your commit fixed a similar issue for type aliases (and I was going to apply that to associated types). Anyway, I will update this PR with your fix.

AnthonyLatsis

Not to pollute the history on master, can you squash this branch to only contain a single commit with the fix?

zoecarver · 2019-11-09T01:59:43Z

Yep, just did.

Uses Anthony's fix (3c87252) to resolve collisions

CodaFi · 2019-11-09T02:06:00Z

@slavapestov Is more familiar with this particular code path than I am. Could you take a look?

CodaFi · 2019-11-09T02:23:43Z

@swift-ci please smoke test

CodaFi · 2019-11-09T02:24:03Z

@swift-ci please test source compatibility

AnthonyLatsis · 2019-11-09T03:29:36Z

@zoecarver The command for the test should not have the not prefix, because it should typecheck successfully. Otherwise the test fails, as you see. But the second test that failed – 0173-circular-generic-type-alias-deserialization – is unfortunately a regression.

zoecarver · 2019-11-09T21:43:23Z

@AnthonyLatsis I spent all morning trying to debug this. I can get one or the other to pass but not both. Any more ideas?

AnthonyLatsis · 2019-11-09T22:57:15Z

I am looking into it as well, as far as my bandwidth permits me. This is almost certainly an annoying circularity issue. I came up with numerous new failing test cases, here's one:

protocol P { associatedtype Assoc }

struct G<T> {}

protocol Q {
    associatedtype A
    typealias Alias<R> = G<R>
    associatedtype C: P where C.Assoc == Alias<Int>
}

zoecarver · 2019-11-09T23:05:10Z

Here's an idea (not sure if it's correct yet): In GenericSignatureBuilder::maybeResolveEquivalenceClass when a parameter type is received, it tries to look it up in Impl::PotentialArchetypes. I think that maybe the issue is that the type should never be added to PotentialArchetypes. Then, in maybeResolveEquivalenceClass it will not be able to find it and will return nullptr (like my original solution). I think the update to resolveTypeInContext might have caused PotentialArchetypes not to be updated.

futurejones · 2020-02-10T05:54:31Z

Can one of the admins verify this patch?

zoecarver · 2020-02-10T06:48:05Z

Closing this patch for now (may re-open at a later date). It may be best to let #27425 take care of this.

CodaFi reviewed Oct 31, 2019

View reviewed changes

test/decl/nested/protocol.swift Outdated Show resolved Hide resolved

zoecarver commented Nov 6, 2019

View reviewed changes

zoecarver requested a review from CodaFi November 6, 2019 04:07

AnthonyLatsis mentioned this pull request Nov 8, 2019

[SE] Allow where clauses on non-generic declarations in generic contexts #23489

Merged

9 tasks

AnthonyLatsis reviewed Nov 9, 2019

View reviewed changes

zoecarver force-pushed the fix-associatedtype branch from 18c645a to 36e4b6c Compare November 9, 2019 01:59

Resolve collisions when typealias used in associatedtype constraint

5248550

Uses Anthony's fix (3c87252) to resolve collisions

zoecarver force-pushed the fix-associatedtype branch from 36e4b6c to 5248550 Compare November 9, 2019 02:01

CodaFi requested a review from slavapestov November 9, 2019 02:06

stash

ca5e247

zoecarver closed this Feb 10, 2020

Fix typealias used in associatedtype constraint #27987

Fix typealias used in associatedtype constraint #27987

Conversation

zoecarver commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi commented Nov 4, 2019

CodaFi commented Nov 4, 2019

zoecarver commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoecarver commented Nov 6, 2019

CodaFi commented Nov 6, 2019 • edited Loading

zoecarver commented Nov 7, 2019

CodaFi commented Nov 7, 2019

zoecarver commented Nov 7, 2019

AnthonyLatsis commented Nov 7, 2019

zoecarver commented Nov 8, 2019

AnthonyLatsis commented Nov 8, 2019 • edited Loading

AnthonyLatsis commented Nov 8, 2019 • edited Loading

zoecarver commented Nov 9, 2019

AnthonyLatsis commented Nov 9, 2019

zoecarver commented Nov 9, 2019 • edited Loading

AnthonyLatsis left a comment

Choose a reason for hiding this comment

zoecarver commented Nov 9, 2019

CodaFi commented Nov 9, 2019

CodaFi commented Nov 9, 2019

CodaFi commented Nov 9, 2019

AnthonyLatsis commented Nov 9, 2019 • edited Loading

zoecarver commented Nov 9, 2019

AnthonyLatsis commented Nov 9, 2019

zoecarver commented Nov 9, 2019

futurejones commented Feb 10, 2020

zoecarver commented Feb 10, 2020

CodaFi Oct 31, 2019 •

edited

Loading

CodaFi commented Nov 6, 2019 •

edited

Loading

AnthonyLatsis commented Nov 8, 2019 •

edited

Loading

AnthonyLatsis commented Nov 8, 2019 •

edited

Loading

zoecarver commented Nov 9, 2019 •

edited

Loading

AnthonyLatsis commented Nov 9, 2019 •

edited

Loading