-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[CodeCompletion] Migrate completions after return
and yield
to solver-based
#68079
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
Conversation
@swift-ci Please smoke test |
@swift-ci Please SourceKit stress test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
e15688a
to
343fcc0
Compare
Note to self: The stress tester passed |
We didn’t actually have any tests for this. Completions aren’t great here at the moment but since this is an underscored language feature it’s not that important at the moment.
343fcc0
to
ea9582e
Compare
// `return nil` is not actually represented as a `ReturnExpr` in the AST but | ||
// gets translated to a `FailStmt`. We thus can't produce the 'nil' completion | ||
// using the solver-based implementation. Add the result manually. | ||
if (auto ctor = dyn_cast_or_null<ConstructorDecl>(DC->getAsDecl())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand this statement. Since we are completing nil
, the I think AST doesn't have it yet. So the AST should be like (return_stmt (code_completion_expr))
. As far as I can see from the code, only return nil
is converted to FailExpr
but non-nil
isn't.
As long as CompletionLookup::addValueLiteralCompletions()
is called, nil
seems to be added unconditionally. Could you explain why it's not the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we don’t even get to type checking (and thus sawSolutionImpl
, which would trigger CompletionLookup
) because PreCheckReturnStmtRequest
fails. I thought about changing PreCheckStmtRequest
to allow return #^COMPLETE^#
but I thought that allowing return
statements inside initializers might cause issues further down the line if initializer type checking assumes that all return
s have been translated to FailStmt
.
ea9582e
to
4702f82
Compare
@swift-ci Please smoke test |
This was pretty straightforward. Completions after
return
needed special handling forreturn nil
in failable constructors becausereturn nil
is represented as aFailStmt
in the type checker and thus we didn’t get completions fornil
here by default.We didn’t have any tests for completion after
yield
. I added a couple. The results aren’t amazing (I didn’t look into whether we could include some results or add more type relations) but probably better than before and since this is an experimental language feature, I think it’s not too important.