-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[IDE] Resolve [.]Type
completion for (any P).
to produce singleton metatype instead of existential metatype.
#73163
base: main
Are you sure you want to change the base?
Conversation
@AnthonyLatsis |
@swift-ci please smoke test Linux |
Currently protocol ExistentialProto {
static func staticMethod()
func instanceMethod()
}
func testExistential() {
let _ = ExistentialProto.#^PROTOCOLTYPE_DOT_1^#
// PROTOCOLTYPE_DOT_1: Begin completions, 3 items
// PROTOCOLTYPE_DOT_1-DAG: Keyword[self]/CurrNominal: self[#(any ExistentialProto).Type#]; name=self
// PROTOCOLTYPE_DOT_1-DAG: Keyword/CurrNominal: Protocol[#(any ExistentialProto).Type#]; name=Protocol
// PROTOCOLTYPE_DOT_1-DAG: Keyword/CurrNominal: Type[#any ExistentialProto.Type#]; name=Type |
What are your thoughts on these three completions: Does either look incorrect to you? If so, why? |
I think according to my changes since we are checking the canonical type to be |
According to this logic, the result of appending
|
At the moment, the expected output as per my logic is |
In terms of the implementation, I feel like the syntactic sugar for parenthesis, i.e |
What do you mean by outcome, and what type are you referring to? |
0fd78cb
to
69e67bb
Compare
With the latest changes, the test passes. |
Please check that other IDE tests pass too. |
All tests pass: Testing Time: 40.10s
Excluded : 9854
Unsupported : 6
Passed : 480
Expectedly Failed: 1
2 warning(s) in tests |
@swift-ci please smoke test Linux |
@AnthonyLatsis |
test/IDE/issue-65843.swift
Outdated
// RUN: %empty-directory(%t) | ||
// RUN: %target-swift-ide-test -batch-code-completion -source-filename %s -filecheck %raw-FileCheck -completion-output-dir %t | ||
|
||
protocol P {} | ||
|
||
(any P).#^COMPLETE?check=META^# | ||
// META: Keyword/CurrNominal: Type[#(any P).Type#]; name=Type |
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.
Please add a test for (P).
. P.Type
== (P).Type
, so the completion should be an existential metatype. I will try to find a better place for these tests in the meantime. Issue-specific test files are more of a last resort in organizing tests.
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.
Sorry, I accidentally truncated my reply. I meant the test case to be (P)
, not P.Type
, etc.
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.
Ok.
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 test currently fails as expected, since the paren sugar check would work for (any P).
and not for (P).
.
if (instanceTy->hasParenSugar()) { | ||
addKeyword("Type", MetatypeType::get(instanceTy), | ||
SemanticContextKind::CurrentNominal); | ||
} else { | ||
addKeyword("Type", ExistentialMetatypeType::get(instanceTy), | ||
SemanticContextKind::CurrentNominal); | ||
} |
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.
Yeah, this is the right direction. But there really is no guarantee that the semantic type will retain the parentheses. A better option is to check the parsed type representation, which is meant to preserve the structure of the written type. You can get it from the type expression.
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.
Wouldn't this check ensure that?
if (isa<TypeExpr>(ParsedExpr)) {
// ...
}
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 you misunderstand. I am suggesting to check for parentheses in the type representation as opposed to the semantic type. The type representation is stored in the type expression.
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 type representation doesn't actually have a function for checking this, rather has a function to retrieve the expression without the parenthesis, do we want to introduce 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.
I think the existing function is optimal here.
…n metatype instead of existential metatype. Resolves swiftlang#65843
69e67bb
to
d27484a
Compare
Resolves #65843
The type completion for
(any P).
is a singleton meta type which currently falsely producesany P.
, i.e, an existential meta type.