Skip to content

RequirementMachine: Protocol reduction order refactoring #39864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

slavapestov
Copy link
Contributor

@slavapestov slavapestov commented Oct 21, 2021

The previous reduction order did not satisfy the property that a merged associated type pair is smaller than or equal to both original associated types. This would trigger an assertion failure in completion.

That is, if P3 : P2 and P3 : P1, we want

[P3:T] < [P1&P2:T] < [P1:T]

Because the merge of [P3:T] with [P1&P2:T] is in fact [P3:T].

In the previous order, a merged symbol was always smaller than a single associated type symbol, so we had [P1&P2:T] < [P3:T].

Also, storing the reduction order in a per-rewrite system protocol graph didn’t really make sense. Make it global on RewriteContext and fold what remains of the ProtocolGraph into the RewriteSystemBuilder, where it is used to compute the transitive closure of the set of protocol dependencies.

Fixes rdar://problem/83768458.

Previously we said that if P1 inherits from P2, then [P1:T] < [P2:T].

However, this didn't generalize to merged associated type symbols;
we always had [P1&P2:T] < [P3:T] even if P3 inherited from both P1
and P2.

This meant that the 'merge' operation did not preserve order, which
fired an assertion in the completion logic.

Fix this by generalizing the linear order to compare the 'support'
of protocols rather than the 'depth', where the 'support' is
defined as the size of the transitive closure of the set under
protocol inheritance.

Then, if you have something like

  protocol P1 {}
  protocol P2 {}
  protocol P3 : P1, P2 {}

The support of 'P1 & P2' is 2, and the support of 'P3' is 3,
therefore [P3:T] < [P1&P2:T] in this example.

Fixes <rdar://problem/83768458>.
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov slavapestov merged commit 11d2837 into swiftlang:main Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant