Fix name lookup in nested protocol inheritance clauses

[karwa]

Currently, this fails to compile with a circular reference error:

protocol SomeBaseProtocol {}

struct ConformanceOnDecl: ConformanceOnDecl.P {
    protocol P: SomeBaseProtocol {}
}

Basically what happens is that InheritedDeclsReferencedRequest is executed for the nested protocol P, and tries to look up what SomeBaseProtocol is.

This goes through to ASTScope lookup, which walks up to the struct ConformanceOnDecl context and calls IterableTypeBodyPortion::lookupMembersOf, which performs a qualified lookup request within the struct ConformanceOnDecl DeclContext.

Qualified lookup then finds the protocol P again and calls getSuperclassDecl on it, which prompts another InheritedDeclsReferencedRequest, and we have a request cycle.

This doesn't happen for non-nested protocols because they live at the top level, so this whole thing where ASTScope walks up to their parent and then back down in to them again won't happen.

---

This patch turns off AllowProtocolMembers in InheritedDeclsReferencedRequest if the decl is a protocol, which causes qualified lookup to skip processing protocols it finds. There are no protocol members that are even valid in an inheritance list, so this should be fine.

The generics book says:

{UnqualifiedLookupFlags::AllowProtocolMembers}: if set, lookup finds members of protocols and protocol extensions. Generally should always be set, except to avoid request cycles in cases where it is known the result of the lookup cannot appear in a protocol or protocol extensions.

So I think un-setting this to avoid a cycle seems reasonable. Other flags such as rhsOfSelfRequirement seem like they exist to avoid similar cycles (although that particular flag does support protocol members such as associated types, which we don't want here).

[slavapestov]

It’s probably time to replace these boolean flags with an OptionSet.

[karwa]

@slavapestov There are a couple of reasons I wanted to avoid doing that with this PR:

1. In my opinion, this kind of cycle should be avoided in a more structured way by ASTScope lookup. It is obvious that if it walks up to a parent scope and enumerates the children, it will find the scope it just visited, and depending on which methods happen to be called, it is incredibly easy for it to get in to a request cycle. If it knew the child it had just visited, it would be able to avoid this.

   I tried to implement this, but there's quite a bit of plumbing to do, and of course I don't know if there are any longer-term plans to rework this lookup so I wasn't sure if it was worth doing right now. I'd like to keep this change low-risk so it can be cherry-picked to 6.0, and major refactoring of name lookup seems to me higher risk than is absolutely necessary to fix this issue.

2. I haven't audited all of the places which pass allowProtocolMembers as true. Right now, everywhere passes it as true in order to maintain the behaviour before this patch, but I suspect some places could afford to turn it off for a slight performance improvement.

Also, could you run the smoke test again? There was a failure on Mac, but the data isn't available on CI any more (I think it was something to do with LLDB? Possibly unrelated to this change. I tried the LLDB tests locally and don't see anything related)

[karwa]

Ping @slavapestov

[DougGregor]

@swift-ci please smoke test

[slavapestov]

@swift-ci Please smoke test

[slavapestov]

@karwa Replacing the boolean flags here with an OptionSet is not a high-risk refactoring. Do you mind landing it on the main branch as a separate PR after this one?

[karwa]

@slavapestov sure 👍

[karwa]

OK nice - the macOS tests passed, so it seems the previous LLDB-related failure was indeed unrelated. (GitHub still shows the test as failing because it's the same commit as last time)

I've dropped the refactoring commit and will open a separate PR for it. So are we good for this one, @slavapestov?

[slavapestov]

LGTM.

As for folding this into ASTScope, it's certainly possible, but the behaviors here are annoying because of backward compatibility. In particular, the LHS and RHS of a conformance requirement are resolved slightly differently, so we would need to create two new scopes for each requirement. I would prefer to phase out some of these quirks in a future -swift-version instead of baking them into the ASTScope design.

[slavapestov]

@swift-ci Please smoke test macOS

[karwa]

Is this okay to merge, then? I'd like to tick it off my todo list 😅

[slavapestov]

@karwa Thank you for the fix!