Skip to content

[PreCheckExpr] Allow pack references when resolving types during TypeExpr folding. #64105

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.

Sign up for GitHub

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
merged 2 commits into from
Mar 5, 2023
Merged

[PreCheckExpr] Allow pack references when resolving types during TypeExpr folding. #64105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8ca989
[PreCheckExpr] Allow pack references when resolving types during Type…
hborla Mar 4, 2023
34b720c
[CSGen] Constraint generation for PackExpansionExpr should return a n…
hborla Mar 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/Sema/CSGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3078,6 +3078,12 @@ namespace {
if (auto *elementExpr = getAsExpr<PackElementExpr>(pack)) {
packType = CS.getType(elementExpr->getPackRefExpr());
} else if (auto *elementType = getAsTypeRepr<PackElementTypeRepr>(pack)) {
// OpenPackElementType sets types for 'each T' type reprs in
// expressions. Some invalid code won't make it there, and
// the constraint system won't have recorded a type.
if (!CS.hasType(elementType->getPackType()))
return Type();
Copy link
Contributor

Choose a reason for hiding this comment

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

Github doesn’t like when comments appear directly on commit apparently…

Aborting here is not ideal for closure contexts because it fails the closure without diagnostic, we should record invalid reference or similar fix and produce a placeholder type just like we do for ErrorExprs…

Copy link
Member Author

@hborla hborla Mar 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This happens when type resolution fails earlier than getting to each T type reprs when they're nested, so a diagnostic has already been produced. I've fixed the other cases where pre-check was failing.

Copy link
Member Author

@hborla hborla Mar 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I can address your suggestion later, but it'll need a little more work than allowing the type variable to bind to a hole, because this type needs to bind to a pack. This means we'll also need to teach ShapeOf constraints to gracefully handle hole types without propagating errors, etc. But for right now, this change fixes a crash-on-invalid, and the type resolution error + unable to infer closure type is a big improvement already.

How about we file a GitHub issue to track that change so we don't forget about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

a diagnostic has already been produced

This is a big problem in closure contexts currently because the diagnostic is produced mid-solving and unbeknownst to the solver so it’s really hard to reason about, we have this invalid reference fix which effectively lies that it produced a diagnostic (another point is that code completion depends on solver not failing early more and more now). We should try and avoid adding more cases like that if possible, fixing it separately sounds good to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #64109!


packType = CS.getType(elementType->getPackType());
} else {
llvm_unreachable("unsupported pack reference ASTNode");
Expand Down
7 changes: 6 additions & 1 deletion lib/Sema/PreCheckExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1435,8 +1435,13 @@ TypeExpr *PreCheckExpression::simplifyNestedTypeExpr(UnresolvedDotExpr *UDE) {
// Fold 'T.U' into a nested type.

// Resolve the TypeRepr to get the base type for the lookup.
TypeResolutionOptions options(TypeResolverContext::InExpression);
// Pre-check always allows pack references during TypeExpr folding.
// CSGen will diagnose cases that appear outside of pack expansion
// expressions.
options |= TypeResolutionFlags::AllowPackReferences;
const auto BaseTy = TypeResolution::resolveContextualType(
InnerTypeRepr, DC, TypeResolverContext::InExpression,
InnerTypeRepr, DC, options,
[](auto unboundTy) {
// FIXME: Don't let unbound generic types escape type resolution.
// For now, just return the unbound generic type.
Expand Down
12 changes: 12 additions & 0 deletions test/Constraints/pack-expansion-expressions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ func typeReprPacks<each T>(_ t: repeat each T) where each T: ExpressibleByIntege

_ = Array<each T>() // expected-error {{pack reference 'T' can only appear in pack expansion or generic requirement}}
_ = 1 as each T // expected-error {{pack reference 'T' can only appear in pack expansion or generic requirement}}
repeat Invalid<String, each T>("") // expected-error {{cannot find 'Invalid' in scope}}
}

func sameShapeDiagnostics<each T, each U>(t: repeat each T, u: repeat each U) {
Expand Down Expand Up @@ -105,3 +106,14 @@ func tupleExpansion<each T, each U>(
_ = zip(repeat each tuple1.element, with: repeat each tuple2.element)
// expected-error@-1 {{global function 'zip(_:with:)' requires the type packs 'U' and 'T' have the same shape}}
}

protocol Generatable {
static func generate() -> Self
}

func generateTuple<each T : Generatable>() -> (repeat each T) {
(each T).generate()
// expected-error@-1 {{pack reference 'T' can only appear in pack expansion or generic requirement}}

return (repeat (each T).generate())
}