Skip to content

Conversation

@AnthonyLatsis
Copy link
Collaborator

No description provided.

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis AnthonyLatsis force-pushed the assoc-inference-system branch from 614359a to 4c96b74 Compare December 2, 2021 19:26
@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis AnthonyLatsis force-pushed the assoc-inference-system branch from 4c96b74 to a18fe75 Compare December 3, 2021 14:08
@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please clean smoke test macOS

Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this PR fix the problem where associated types must be re-stated in inherited protocols?

Eg, this doesn't type check today:

protocol Base {
  associatedtype A
}

protocol Derived : Base {
  func foo() -> A
}

struct S : Derived {
  func foo() -> Int { return 0 }
}

You need to pointlessly re-declare the 'associatedtype A' inside 'protocol Derived'. It would be nice to make this work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about Self.X.Y == Self.X.Y.*, etc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything other than Self.X := Y is not considered at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is sensitive to the order of protocols in the inheritance clause, which might be weird. Perhaps you should canonicalize the list of inherited protocols when walking (or walk over conformance requirements on 'Self' in the requirement signature instead, which are in canonical order).

@slavapestov
Copy link
Contributor

@AnthonyLatsis can you run source compat testing on this once you resolve the merge conflict?

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Dec 9, 2021

Does this PR fix the problem where associated types must be re-stated in inherited protocols?

Not in general, I haven't touched inference via value witnesses yet.

@AnthonyLatsis AnthonyLatsis force-pushed the assoc-inference-system branch from a18fe75 to 530181c Compare January 27, 2022 08:50
@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please test source compatibility

2 similar comments
@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please test source compatibility

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please test source compatibility

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Feb 2, 2022

@slavapestov source compatibility failures are unrelated, but in isolation this is in fact a source breaking change. There is a group of conformances we are not yet prepared to deal with for which associated type inference now fails. A selected few of these compile today, e.g.

protocol P { associatedtype A = Int }
protocol Q { associatedtype B }
protocol R: P, Q where A == B {}

struct S: R {} // A := Int, B := Int (correct)

and while some infer the correct type witnesses by chance like the above, others do not:

protocol P { associatedtype B = Int }
protocol Q { associatedtype A }
protocol R: P, Q where A == B {}

struct S<A>: R {} // A := A (generic parameter), B := A (wrong)

They all have two things in common: you need more information than just the same-type constraints to decide on a type witness, and the type witnesses of one conformance cannot be resolved independently of another in general.
I reckon that having something fail before it starts working properly is better than a sudden change in behavior. Still, I would like to hear your thoughts on whether we should gate this behind a flag until the global issue is addressed.

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Feb 9, 2022

@swift-ci please test source compatibility

Let's make these green.

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Feb 15, 2022

The probability of someone relying on the entailing source compatibility nuance I mentioned is really low, given that these cases just started "working" in the last minor release or so, so I'm going to merge this and try do deal with the existing bigger issue in 5.7. Then again, we can always defer or revert this if something goes left.

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

Since we collect same-type constraints by scanning requirement signatures,
the type witness system must be prerared to face conflicting solutions
for a particular type witness
…esses in inferAbstractTypeWitnesses()

A regular type.subst() is not enough, because tentative type witnesses may contain type parameters,
and we have to substitute them as well, recursively, e.g A := G<B>, B := G<C>, C := Never. Avoiding
subst() here also eliminates a use case of the getSubstOptionsWithCurrentTypeWitnesses() hack.
@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please test source compatibility

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test macOS

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please test source compatibility

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci clean smoke test macOS

1 similar comment
@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci clean smoke test macOS

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please test source compatibility

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Feb 18, 2022

@shahmishal Did something happen with the archiving of build artefacts in source compat builds?

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please test source compatibility

@AnthonyLatsis AnthonyLatsis merged commit b004396 into swiftlang:main Feb 20, 2022
@AnthonyLatsis AnthonyLatsis deleted the assoc-inference-system branch February 20, 2022 12:52
hborla added a commit to hborla/swift that referenced this pull request Mar 17, 2022
…nference-system"

This reverts commit b004396, reversing
changes made to e30ba5f.
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.

2 participants