Skip to content

[CodeComplete] A couple of contextual type handling tweaks #71211

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
Jan 30, 2024
Merged

[CodeComplete] A couple of contextual type handling tweaks #71211

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
3d06f0d
[CodeComplete] Improve `ExpectsNonVoid` computation
hamishknight Jan 29, 2024
d022cf8
[CodeComplete] Avoid using an unresolved contextual type
hamishknight Jan 29, 2024
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
32 changes: 18 additions & 14 deletions lib/IDE/PostfixCompletion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,23 +189,27 @@ void PostfixCompletionCallback::sawSolutionImpl(

bool BaseIsStaticMetaType = S.isStaticallyDerivedMetatype(ParsedExpr);

bool ExpectsNonVoid = false;
SmallVector<Type, 4> ExpectedTypes;
if (ExpectedTy) {
ExpectedTypes.push_back(ExpectedTy);
}

bool ExpectsNonVoid = false;
ExpectsNonVoid |= ExpectedTy && !ExpectedTy->isVoid();
ExpectsNonVoid |=
!ParentExpr && CS.getContextualTypePurpose(CompletionExpr) != CTP_Unused;

for (auto SAT : S.targets) {
if (ExpectsNonVoid) {
// ExpectsNonVoid is already set. No need to iterate further.
break;
}
if (SAT.second.getAsExpr() == CompletionExpr) {
ExpectsNonVoid |= SAT.second.getExprContextualTypePurpose() != CTP_Unused;
ExpectsNonVoid = !ExpectedTy->isVoid();
} else {
// If we don't know what the expected type is, assume it must be non-Void
// if we have a contextual type that is not unused. This prevents us from
// suggesting Void values for e.g bindings without explicit types.
ExpectsNonVoid |= !ParentExpr &&
CS.getContextualTypePurpose(CompletionExpr) != CTP_Unused;

for (auto SAT : S.targets) {
if (ExpectsNonVoid) {
// ExpectsNonVoid is already set. No need to iterate further.
break;
}
if (SAT.second.getAsExpr() == CompletionExpr) {
ExpectsNonVoid |=
SAT.second.getExprContextualTypePurpose() != CTP_Unused;
}
}
}

Expand Down
5 changes: 4 additions & 1 deletion lib/IDE/TypeCheckCompletionCallback.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,11 @@ void TypeCheckCompletionCallback::fallbackTypeCheck(DeclContext *DC) {

Type swift::ide::getTypeForCompletion(const constraints::Solution &S,
ASTNode Node) {
// Use the contextual type, unless it is still unresolved, in which case fall
// back to getting the type from the expression.
if (auto ContextualType = S.getContextualType(Node)) {
return ContextualType;
if (!ContextualType->hasUnresolvedType())
return ContextualType;
}

if (!S.hasType(Node)) {
Expand Down
8 changes: 5 additions & 3 deletions test/IDE/complete_issue-55711.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
// RUN: %swift-ide-test -code-completion -source-filename=%s -code-completion-token=A | %FileCheck %s --check-prefix=A
// RUN: %swift-ide-test -code-completion -source-filename=%s -code-completion-token=B | %FileCheck %s --check-prefix=B
// RUN: %swift-ide-test -code-completion -source-filename=%s -code-completion-token=D | %FileCheck %s --check-prefix=D
// RUN: %batch-code-completion

// https://github.com/apple/swift/issues/55711
// https://forums.swift.org/t/code-completion-enhancement-request/38677
Expand Down Expand Up @@ -37,6 +35,10 @@ func test() {
C(.a) {
.#^A^#
}
C(.a) {
()
return .#^A_MULTISTMT?check=A^#
}
// A: Begin completions, 2 items
// A-DAG: Decl[StaticMethod]/CurrNominal/TypeRelation[Convertible]: foo({#arg: Bool#})[#A<X>#];
// A-DAG: Decl[StaticMethod]/CurrNominal/TypeRelation[Convertible]: bar({#arg: Int#})[#A<Y>#];
Expand Down
64 changes: 64 additions & 0 deletions test/IDE/complete_single_expression_return.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,55 @@ struct TestSingleExprClosureGlobal {
// TestSingleExprClosureGlobal-DAG: Decl[InstanceMethod]/CurrNominal: void()[#Void#];
}

struct TestSingleExprClosureBinding {
func void() -> Void {}
func str() -> String { return "" }
func int() -> Int { return 0 }

func test() -> Int {
let fn = {
self.#^TestSingleExprClosureBinding^#
}
return fn()
}
// Void is always valid in an implicit single expr closure.
// TestSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: str()[#String#];
// TestSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: int()[#Int#];
// TestSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: void()[#Void#];
}

struct TestExplicitSingleExprClosureBinding {
func void() -> Void {}
func str() -> String { return "" }
func int() -> Int { return 0 }

func test() {
let fn = {
return self.#^TestExplicitSingleExprClosureBinding^#
}
}
// FIXME: Because we have an explicit return, and no expected type, we shouldn't suggest Void.
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think suggesting values that return Void after return makes sense if the closure has a Void return type. For example return callback() is a common pattern for early returns out of closures and as a shorthand of callback(); return

So, I would just remove the FIXME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but in this case there's no contextual type. If there was an explicit Void contextual type, we'd allow it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could carve out an exception for CTP_ReturnStmt if we really wanted this behavior though

Copy link
Member

Choose a reason for hiding this comment

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

I think what I’m trying to say is that I don’t really care about this case because you can argue either way and I would just remove the FIXME. But if you want to keep it, that’s good with me as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to keep it for now since this is going to change anyway with my constraint system patch. I should also note that not suggesting Void is also the behavior we currently have for multi-statement closure returns. I don't really feel too strongly about it either way (and am happy to carve out an exception for returns if we want that), but I think we should at least be consistent there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing in #71272

// TestExplicitSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: str()[#String#];
// TestExplicitSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: int()[#Int#];
// TestExplicitSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: void()[#Void#];
}

struct TestExplicitSingleExprClosureBindingWithContext {
func void() -> Void {}
func str() -> String { return "" }
func int() -> Int { return 0 }

func test() {
let fn: () -> Void = {
return self.#^TestExplicitSingleExprClosureBindingWithContext^#
}
}
// We know Void is valid.
// TestExplicitSingleExprClosureBindingWithContext-DAG: Decl[InstanceMethod]/CurrNominal: str()[#String#];
// TestExplicitSingleExprClosureBindingWithContext-DAG: Decl[InstanceMethod]/CurrNominal: int()[#Int#];
// TestExplicitSingleExprClosureBindingWithContext-DAG: Decl[InstanceMethod]/CurrNominal/TypeRelation[Convertible]: void()[#Void#];
}

struct TestNonSingleExprClosureGlobal {
func void() -> Void {}
func str() -> String { return "" }
Expand Down Expand Up @@ -190,6 +239,21 @@ struct TestSingleExprFunc {
// TestSingleExprFunc-DAG: Decl[InstanceMethod]/CurrNominal: void()[#Void#];
}

struct TestSingleExprFuncReturnVoid {
func void() -> Void {}
func str() -> String { return "" }
func int() -> Int { return 0 }

func test() {
return self.#^TestSingleExprFuncReturnVoid^#
}

// Void is the only possible type that can be used here.
// TestSingleExprFuncReturnVoid-DAG: Decl[InstanceMethod]/CurrNominal: str()[#String#];
// TestSingleExprFuncReturnVoid-DAG: Decl[InstanceMethod]/CurrNominal: int()[#Int#];
// TestSingleExprFuncReturnVoid-DAG: Decl[InstanceMethod]/CurrNominal/TypeRelation[Convertible]: void()[#Void#];
}

struct TestSingleExprFuncUnresolved {
enum MyEnum { case myEnum }
enum NotMine { case notMine }
Expand Down