Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rajveer100
Copy link
Contributor

Resolves #65843

The type completion for (any P). is a singleton meta type which currently falsely produces any P., i.e, an existential meta type.

@Rajveer100
Copy link
Contributor Author

@AnthonyLatsis
Could you trigger the CI to see if it breaks other tests?

@AnthonyLatsis
Copy link
Collaborator

@swift-ci please smoke test Linux

@Rajveer100
Copy link
Contributor Author

Rajveer100 commented Apr 20, 2024

Currently IDE/complete_value_expr.swift test fails. Would the following also be considered a singleton meta type:

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

@AnthonyLatsis
Copy link
Collaborator

What are your thoughts on these three completions: Does either look incorrect to you? If so, why?

@Rajveer100
Copy link
Contributor Author

Rajveer100 commented Apr 20, 2024

I think according to my changes since we are checking the canonical type to be ExistentialMetatypeType, the completion should be Type[#(any ExistentialProto).Type#]

@AnthonyLatsis
Copy link
Collaborator

P.Type is equivalent to any P.Type.

I think accordingly my changes since we are checking the canonical type to be ExistentialMetatypeType

According to this logic, the result of appending .Type is:

  • An existential metatype if the type we are completing is also an existential metatype. This is not true: appending .Type to (any P.Type), which is an existential metatype, produces the singleton metatype (any P.Type).Type.
  • A singleton metatype otherwise. This is also not true: appending .Type to any P, which is not an existential metatype, produces the existential metatype any P.Type.

@Rajveer100
Copy link
Contributor Author

At the moment, the expected output as per my logic is Type[#(any ExistentialProto).Type#] which obviously fails. What I meant to say is, if the test is right, i.e, it's an existential meta type, the canonical type parameter which I added to the conditional check should succeed yielding the result.

@AnthonyLatsis
Copy link
Collaborator

#65843 (comment)

@Rajveer100
Copy link
Contributor Author

In terms of the implementation, I feel like the syntactic sugar for parenthesis, i.e (...) affects the outcome although the canonical type is correctly parsed for the AST?

@AnthonyLatsis
Copy link
Collaborator

What do you mean by outcome, and what type are you referring to?

@Rajveer100
Copy link
Contributor Author

With the latest changes, the test passes.

@AnthonyLatsis
Copy link
Collaborator

Please check that other IDE tests pass too.

@Rajveer100
Copy link
Contributor Author

All tests pass:

Testing Time: 40.10s
  Excluded         : 9854
  Unsupported      :    6
  Passed           :  480
  Expectedly Failed:    1

2 warning(s) in tests

@AnthonyLatsis
Copy link
Collaborator

@swift-ci please smoke test Linux

@Rajveer100
Copy link
Contributor Author

@AnthonyLatsis
Are there any other changes needed?

Comment on lines 1 to 7
// 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
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Apr 24, 2024

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Contributor Author

@Rajveer100 Rajveer100 Apr 25, 2024

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)..

Comment on lines +2443 to +2449
if (instanceTy->hasParenSugar()) {
addKeyword("Type", MetatypeType::get(instanceTy),
SemanticContextKind::CurrentNominal);
} else {
addKeyword("Type", ExistentialMetatypeType::get(instanceTy),
SemanticContextKind::CurrentNominal);
}
Copy link
Collaborator

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.

Copy link
Contributor Author

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)) {
  // ...
}

Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Apr 24, 2024

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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
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.

[.]Type completion for (any P). shows it will produce an existential metatype
2 participants