Skip to content

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Feb 20, 2024

Editors other than Xcode don’t have a notion of editor placeholders or their expansion, so we can’t produce results with editor placeholders and expect the user to expand them while completing the function arguments.

Instead, expand all trailing closure placeholders when producing the code completion results.

The generated expansion is currently not formatted with respect to the file’s indentaiton. Since we don’t want to launch swift-format for every completion item, this formatting will need to be done using BasicFormat which needs to infer the file’s indentation. Doing so will be non-trivial work on its own and will be done in a follow-up PR (rdar://123287930).

rdar://121130170

@ahoppen
Copy link
Member Author

ahoppen commented Feb 20, 2024

swiftlang/swift-syntax#2503

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Feb 21, 2024

CI testing passed on macOS and Linux. This PR depends on two swift-syntax PRs (swiftlang/swift-syntax#2503 and swiftlang/swift-syntax#2506). Will re-trigger once they are merged.

Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

My main concern here is performance across many completions. Any idea how this performs on a relatively large file with thousands of completions?

@ahoppen ahoppen force-pushed the ahoppen/expand-trailing-closure branch from 408cd33 to 23ae42b Compare February 22, 2024 02:45
@ahoppen
Copy link
Member Author

ahoppen commented Feb 22, 2024

Changed it to only parse the completion item itself instead of doing an incremental parse of the entire file. Measured performance and expanding placeholders in a single item takes ~0.2ms. So in the worst case if all 200 possible completion items have closure placeholders, it would could down completion by 40ms. Realistically, for example completion on Array has ~40 completion items with closure placeholders, which makes filtering those results go from 2ms to 10ms, both of which are acceptable values to me.

@ahoppen
Copy link
Member Author

ahoppen commented Feb 22, 2024

swiftlang/swift-syntax#2503

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/expand-trailing-closure branch from 23ae42b to c3a273b Compare February 23, 2024 01:54
@ahoppen
Copy link
Member Author

ahoppen commented Feb 24, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Feb 24, 2024

@swift-ci Please test Windows

Editors other than Xcode don’t have a notion of editor placeholders or their expansion, so we can’t produce results with editor placeholders and expect the user to expand them while completing the function arguments.

Instead, expand all trailing closure placeholders when producing the code completion results.

The generated expansion is currently not formatted with respect to the file’s indentaiton. Since we don’t want to launch `swift-format` for every completion item, this formatting will need to be done using `BasicFormat` which needs to infer the file’s indentation. Doing so will be non-trivial work on its own and will be done in a follow-up PR.

rdar://121130170
@ahoppen ahoppen force-pushed the ahoppen/expand-trailing-closure branch from c3a273b to 90c124c Compare February 25, 2024 01:59
@ahoppen
Copy link
Member Author

ahoppen commented Feb 25, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Feb 25, 2024

@swift-ci Please test Windows

ahoppen added a commit to ahoppen/swift that referenced this pull request Feb 26, 2024
ahoppen added a commit to ahoppen/swift that referenced this pull request Feb 29, 2024
sourcekit-lsp will be using this in swiftlang/sourcekit-lsp#1072 and Windows uses the swift-syntax modules from the compiler build for sourcekit-lsp.
@ahoppen
Copy link
Member Author

ahoppen commented Feb 29, 2024

swiftlang/swift#71983

@swift-ci Please test Windows

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.

2 participants