Skip to content

[CodeCompletion] Call sawSolution when type checking a result builder #41846

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
merged 1 commit into from
Mar 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions include/swift/Sema/CompletionContextFinder.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#ifndef SWIFT_SEMA_COMPLETIONCONTEXTFINDER_H
#define SWIFT_SEMA_COMPLETIONCONTEXTFINDER_H

#include "swift/AST/ASTNode.h"
#include "swift/AST/ASTWalker.h"
#include "swift/AST/Expr.h"
#include "swift/Sema/CodeCompletionTypeChecking.h"
Expand Down Expand Up @@ -49,10 +50,10 @@ class CompletionContextFinder : public ASTWalker {

public:
/// Finder for completion contexts within the provided initial expression.
CompletionContextFinder(Expr *initialExpr, DeclContext *DC)
: InitialExpr(initialExpr), InitialDC(DC) {
CompletionContextFinder(ASTNode initialNode, DeclContext *DC)
: InitialExpr(initialNode.dyn_cast<Expr *>()), InitialDC(DC) {
assert(DC);
initialExpr->walk(*this);
initialNode.walk(*this);
};

/// Finder for completion contexts within the outermost non-closure context of
Expand Down
12 changes: 11 additions & 1 deletion lib/Sema/BuilderTransform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1736,7 +1736,17 @@ Optional<BraceStmt *> TypeChecker::applyResultBuilderBodyTransform(

// Solve the constraint system.
SmallVector<Solution, 4> solutions;
if (cs.solve(solutions) || solutions.size() != 1) {
bool solvingFailed = cs.solve(solutions);

if (cs.getASTContext().CompletionCallback) {
CompletionContextFinder analyzer(func, func->getDeclContext());
filterSolutionsForCodeCompletion(solutions, analyzer);
for (const auto &solution : solutions) {
cs.getASTContext().CompletionCallback->sawSolution(solution);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to just stop here instead of going farther and emitting diagnostics?

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ll try that in a follow-up PR because I’m curious to see if stopping early improves performance and if yes, by how much.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense regardless of performance impact because these diagnostics would just get emitted to be discarded.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mostly want to know how big the performance win is. And I wanted to get this one merged to unblock the argument completion PR ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, no worries, I'm not saying it should have been added in this PR, just want to make a point about it being logically correct thing to do :)

}

if (solvingFailed || solutions.size() != 1) {
// Try to fix the system or provide a decent diagnostic.
auto salvagedResult = cs.salvage();
switch (salvagedResult.getKind()) {
Expand Down
59 changes: 32 additions & 27 deletions lib/Sema/TypeCheckCodeCompletion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -513,11 +513,32 @@ TypeChecker::getTypeOfCompletionOperator(DeclContext *DC, Expr *LHS,
}
}

/// Remove any solutions from the provided vector that both require fixes and
/// have a score worse than the best.
static void filterSolutions(SolutionApplicationTarget &target,
SmallVectorImpl<Solution> &solutions,
CodeCompletionExpr *completionExpr) {
static bool hasTypeForCompletion(Solution &solution,
CompletionContextFinder &contextAnalyzer) {
if (contextAnalyzer.hasCompletionExpr()) {
return solution.hasType(contextAnalyzer.getCompletionExpr());
} else {
assert(contextAnalyzer.hasCompletionKeyPathComponent());
return solution.hasType(
contextAnalyzer.getKeyPathContainingCompletionComponent(),
contextAnalyzer.getKeyPathCompletionComponentIndex());
}
}

void TypeChecker::filterSolutionsForCodeCompletion(
SmallVectorImpl<Solution> &solutions,
CompletionContextFinder &contextAnalyzer) {
// Ignore solutions that didn't end up involving the completion (e.g. due to
// a fix to skip over/ignore it).
llvm::erase_if(solutions, [&](Solution &S) {
if (hasTypeForCompletion(S, contextAnalyzer))
return false;
// FIXME: Technically this should never happen, but it currently does in
// result builder contexts. Re-evaluate if we can assert here when we have
// multi-statement closure checking for result builders.
return true;
});

if (solutions.size() <= 1)
return;

Expand Down Expand Up @@ -608,15 +629,6 @@ bool TypeChecker::typeCheckForCodeCompletion(
if (!cs.solveForCodeCompletion(target, solutions))
return CompletionResult::Fallback;

// FIXME: instead of filtering, expose the score and viability to clients.
// Remove any solutions that both require fixes and have a score that is
// worse than the best.
CodeCompletionExpr *completionExpr = nullptr;
if (contextAnalyzer.hasCompletionExpr()) {
completionExpr = contextAnalyzer.getCompletionExpr();
}
filterSolutions(target, solutions, completionExpr);

// Similarly, if the type-check didn't produce any solutions, fall back
// to type-checking a sub-expression in isolation.
if (solutions.empty())
Expand All @@ -626,19 +638,7 @@ bool TypeChecker::typeCheckForCodeCompletion(
// closure body it could either be type-checked together with the context
// or not, it's impossible to say without checking.
if (contextAnalyzer.locatedInMultiStmtClosure()) {
auto &solution = solutions.front();

bool HasTypeForCompletionNode = false;
if (completionExpr) {
HasTypeForCompletionNode = solution.hasType(completionExpr);
} else {
assert(contextAnalyzer.hasCompletionKeyPathComponent());
HasTypeForCompletionNode = solution.hasType(
contextAnalyzer.getKeyPathContainingCompletionComponent(),
contextAnalyzer.getKeyPathCompletionComponentIndex());
}

if (!HasTypeForCompletionNode) {
if (!hasTypeForCompletion(solutions.front(), contextAnalyzer)) {
// At this point we know the code completion node wasn't checked with
// the closure's surrounding context, so can defer to regular
// type-checking for the current call to typeCheckExpression. If that
Expand All @@ -651,6 +651,11 @@ bool TypeChecker::typeCheckForCodeCompletion(
}
}

// FIXME: instead of filtering, expose the score and viability to clients.
// Remove solutions that skipped over/ignored the code completion point
// or that require fixes and have a score that is worse than the best.
filterSolutionsForCodeCompletion(solutions, contextAnalyzer);

llvm::for_each(solutions, callback);
return CompletionResult::Ok;
};
Expand Down
11 changes: 9 additions & 2 deletions lib/Sema/TypeChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@
#include "swift/AST/NameLookup.h"
#include "swift/AST/PropertyWrappers.h"
#include "swift/AST/TypeRefinementContext.h"
#include "swift/Parse/Lexer.h"
#include "swift/Basic/OptionSet.h"
#include "swift/Sema/ConstraintSystem.h"
#include "swift/Config.h"
#include "swift/Parse/Lexer.h"
#include "swift/Sema/CompletionContextFinder.h"
#include "swift/Sema/ConstraintSystem.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/TinyPtrVector.h"
#include <functional>
Expand Down Expand Up @@ -571,6 +572,12 @@ FunctionType *getTypeOfCompletionOperator(DeclContext *DC, Expr *LHS,
DeclRefKind refKind,
ConcreteDeclRef &refdDecl);

/// Remove any solutions from the provided vector that require more fixes than
/// the best score or don't contain a type for the code completion token.
void filterSolutionsForCodeCompletion(
SmallVectorImpl<constraints::Solution> &solutions,
CompletionContextFinder &contextAnalyzer);

/// Type check the given expression and provide results back to code completion
/// via specified callback.
///
Expand Down