-
Couldn't load subscription status.
- Fork 10.6k
[CodeCompletion] Wrap base expression with CodeCompletionExpr #32184
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 |
c510db9 to
f67c1b5
Compare
|
@swift-ci Please smoke test |
lib/Sema/CSGen.cpp
Outdated
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.
Not sure this is desirable. But this is needed to preserve the current behavior in several test cases.
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.
Am I reading this correctly that we are saying that in
foo(something.<complete>)
the type of something.completed_expr must be defaultable to the type of something? What kinds of tests changes do you see without this?
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.
ConstraintKind::Defaultable means "the first type can be defaulted to the second", not "must be defaultable", IIUC. It's something like "fallback" type.
The failing test case was:
let val = Something.<HERE>
In this case val is type checked to the type of Something so each result has type relation (e.g. Invalid for Void result). Without Defaultable, since val cannot be resolved, there's no expected type here. (I'm not saying that val as Something is the right thing)
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 see. That seems like an unfortunate way to say "non-Void". What happens if there is an expected type and it really is Void?
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.
It seems like what we really want here is for code completion expression to be marked as a hole by default, just like we do in some other cases, that would solve a case where there is absolutely no contextual information available (let _ = foo.<#complete#> just like Rintaro mentioned).
I'm still thinking about this but it might be possible to generate constraints here in a way which would match what we do for DefineMemberBasedOnUse fix if member couldn't be found, that would guarantee that there is either a type or a hole inferred for a member reference.
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 think the least disruptive change would be to default code completion to UnresolvedType regardless of whether it's a member lookup or not, we also currently use UnresolvedType to indicate holes so it's fitting.
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.
Conceptually seems reasonable to me, but the CSGen change doesn't make sense to me.
lib/Sema/CSGen.cpp
Outdated
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.
Am I reading this correctly that we are saying that in
foo(something.<complete>)
the type of something.completed_expr must be defaultable to the type of something? What kinds of tests changes do you see without this?
f67c1b5 to
0ed063d
Compare
|
@swift-ci Please smoke test |
test/IDE/complete_at_top_level.swift
Outdated
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.
The test case is
var topLevelVar1 = #^TOP_LEVEL_VAR_INIT_1^#
// Check that the variable itself does not show up.
// TOP_LEVEL_VAR_INIT_1_NEGATIVE-NOT: topLevelVar1This used to work only because typechecking this decl was failed. Now that it's typechecked to UnresolvedType so it revealed the existing issue(rdar://problem/56755598).
0ed063d to
12493c6
Compare
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.
This is a regression. For example:
let _ = [<HERE>: 1]Previously this used to be type checked to [AnyHashable : Int], but now it's [<<unresolved type>> : Int]. It's unfortunate, but in practice, I don't think this causes a big difference.
|
@swift-ci Please smoke test |
For example for: funcName(base.<HERE>) Wrap 'base' with 'CodeCompletionExpr' so that type checker can check 'base' independently without preventing the overload choice of 'funcName'. This increases the chance of successful type checking. rdar://problem/63965160
12493c6 to
3ec250f
Compare
|
@swift-ci Please smoke test |
|
Decided not to default it to |
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.
LGTM! We'll iterate on how to make code completion expressions proper "holes".
For example, for:
Wrap
basewithCodeCompletionExprso that type checker can checkbaseindependently without affecting the overload choice offuncName.This increases the chance of successful type checking.
rdar://problem/63965160