Skip to content

[code-completion] Add type context for single-expression closures #23411

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 4 commits into from
Mar 20, 2019

Conversation

benlangmuir
Copy link
Contributor

When completing in the only expression of closure, use the return type of the closure as the type context for the code-completion. However, since code-completion may be on an incomplete input, we only use the return type to improve the quality of the result, not to mark it invalid, since (a) we may add another statement afterwards, or (b) if the context type is Void it doesn't need to match the value.

When completing in the only expression of closure, use the return type
of the closure as the type context for the code-completion. However,
since code-completion may be on an incomplete input, we only use the
return type to improve the quality of the result, not to mark it
invalid, since (a) we may add another statement afterwards, or (b) if
the context type is Void it doesn't need to match the value.
@benlangmuir benlangmuir requested a review from rintaro March 19, 2019 00:05
@benlangmuir
Copy link
Contributor Author

@swift-ci please test

@benlangmuir
Copy link
Contributor Author

@rintaro this is a bit different than we discussed. Instead of handling Void specially, I keep track of whether the type context is from a single-expression body and in that case do not mark anything as type-relation "invalid".

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - b817cbb

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - b817cbb

There usually won't be a lot of expected types, so just use a
SmallVector and copy it instead of trying to get tricky with the
lifetime.
@benlangmuir
Copy link
Contributor Author

@swift-ci please test

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

Seen in validation-test/IDE/crashers_fixed/019-swift-vardecl-emitlettovarnoteifsimple.swift
@benlangmuir
Copy link
Contributor Author

@swift-ci please test

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

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

Implementation makes sense to me :)

case 1:
return body->getElements()[0].is<Expr *>();
default:
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Do we already have test cases for this path? like:

var floatVal: Float = 2
let _: () -> Float = {
  #^COMPLETE^# // Should be normal completion.
  return 42
}
let _: () -> SomeEnum = {
  .#^COMPLETE^# // Should not provide anything.
  return 42
}

If not, please add them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this code for handling getNumElements() == 0 is from before I changed the parser to add the CodeCompletionExpr in completeStmtOrExpr. I'll remove the 0 case and add the additional test.

@@ -1976,7 +1976,8 @@ bool MissingArgumentsFailure::diagnoseAsError() {
auto path = locator->getPath();

// TODO: Currently this is only intended to diagnose contextual failures.
if (!(path.back().getKind() == ConstraintLocator::ApplyArgToParam ||
if (path.empty() ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xedin FYI

Avoids assertion failure:
https://ci.swift.org/job/swift-PR-osx/11875/consoleFull#-1251633450ef3c3c00-f496-40b9-85a7-0eb69d0f491b

(lldb) p locator->anchor->dump()
(assign_expr type='<null>'
  (unresolved_dot_expr type='<null>' field 'a' function_ref=unapplied
    (type_expr implicit type='<null>' typerepr='l'))
  (closure_expr type='<null>' discriminator=0
    (parameter_list range=[/Users/blangmuir/src/swift/validation-test/IDE/crashers_fixed/019-swift-vardecl-emitlettovarnoteifsimple.swift:2:38 - line:2:38])
    (brace_stmt range=[/Users/blangmuir/src/swift/validation-test/IDE/crashers_fixed/019-swift-vardecl-emitlettovarnoteifsimple.swift:2:38 - line:2:39]
      (code_completion_expr implicit type='<null>'))))
abort + 127
basename_r + 0
swift::constraints::MissingArgumentsFailure::diagnoseAsError() + 150
swift::constraints::AddMissingArguments::diagnose(swift::Expr*, bool) const + 122
swift::constraints::ConstraintSystem::applySolutionFixes(swift::Expr*, :$_12::operator()(swift::Expr*) const + 203
swift::constraints::ConstraintSystem::applySolutionFixes(swift::Expr*, + 548
swift::constraints::ConstraintSystem::applySolution(swift::constraints::Solution&, ) + 73
swift::TypeChecker::typeCheckExpressionImpl(swift::Expr*&, swift::DeclContext*, Purpose, swift::OptionSet<rFlags, unsigned int>, swift::ExprTypeCheckListener&, ) + 1047
swift::TypeChecker::typeCheckBinding(swift::Pattern*&, swift::Expr*&, swift::DeclContext*) 
swift::TypeChecker::typeCheckPatternBinding(swift::PatternBindingDecl*, unsigned int) + 247
swift::typeCheckPatternBinding(swift::PatternBindingDecl*, unsigned int) + 144
(anonymous namespace)::typeCheckContextImpl(swift::DeclContext*, swift::SourceLoc) + 226
(anonymous namespace)::CodeCompletionCallbacksImpl::doneParsing() + 3452
swift::performDelayedParsing(swift::DeclContext*, swift::PersistentParserState&, *) + 458
swift::CompilerInstance::parseAndCheckTypesUpTo(swift::CompilerInstance::ImplicitImports ) + 710
swift::CompilerInstance::performSemaUpTo(swift::SourceFile::ASTStage_t) + 615

Ensure that we get the correct behaviour when we are not in the
single-expression case, and remove code that handled 0-element bodies,
which is no longer needed after the parser change.
@benlangmuir
Copy link
Contributor Author

@swift-ci please test

@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

@benlangmuir
Copy link
Contributor Author

@swift-ci please test macOS

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.

3 participants