Skip to content

Conversation

slavapestov
Copy link
Contributor

@slavapestov slavapestov commented Sep 17, 2025

This PR is another intermediate step on my quest to simplify getTypeOfReference()/getTypeOfMemberReference().

These functions are used in exactly two places: when applying a disjunction choice inside the solver, and in protocol witness matching. I cleaned up the latter code path a bit to make it look more like what happens in the solver, and this allowed me to simplify the parameter lists of both functions a fair bit; both take an OverloadChoice now.

The Pre()/Post() split is transitional and I'm going to re-organize this a little bit more, but I'd like to land this separately and test source compatibility first.

@slavapestov slavapestov force-pushed the better-prepared-overloads branch from 3a0ab19 to 1ea7382 Compare September 17, 2025 14:22
@slavapestov slavapestov marked this pull request as ready for review September 17, 2025 14:55
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

2 similar comments
@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

@slavapestov
Copy link
Contributor Author

swiftlang/swift-package-manager#9150
@swift-ci Please smoke test Linux

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test macOS

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Windows

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

ASSERT(type && "Expected non-null type");

if (preparedOverload) {
preparedOverload->recordedNodeType(node, type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because property wrapper application happens as part of getTypeOfMemberReference() and it calls setType(), so we have to record it and play it back.

cs.addConstraint(ConstraintKind::Bind, typeVar, subst(gp), locator);
for (auto [gp, typeVar] : replacements) {
cs.addConstraint(ConstraintKind::Bind, typeVar, subst(gp), locator,
/*isFavored=*/false, preparedOverload);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of the fact that we have to add prepared overloads to all these various APIs, including addConstraint. I think prepared overloads belong in the solver not in the constraint system.

Copy link
Contributor Author

@slavapestov slavapestov Sep 18, 2025

Choose a reason for hiding this comment

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

Once getTypeOf{Member,}Reference() are always called with preparedOverload != nullptr, I'm planning on changing a bunch of APIs that currently take an optional preparedOverload to be methods on the PreparedOverloadBuilder itself, since in those places we'll always be able to call it on a builder that is provided. I agree it is annoying to have to plumb it through to intercept various places that would otherwise record trail changes, but I hope to separate the two code paths completely (edit: well, probably not 100%) at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, let's see how it turns out.

@slavapestov slavapestov force-pushed the better-prepared-overloads branch from 98d0ea5 to e9070cf Compare September 18, 2025 18:57
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test macOS

@slavapestov slavapestov merged commit cf3dab1 into swiftlang:main Sep 19, 2025
3 of 5 checks passed
ktoso pushed a commit to ktoso/swift that referenced this pull request Sep 19, 2025
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