Add an extra percent encoding layer when encoding DocumentURIs to LSP requests #1636
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
@swift-ci Please test |
lokesh-tr
reviewed
Aug 20, 2024
lokesh-tr
reviewed
Aug 20, 2024
Contributor
|
@ahoppen Thanks for this fix, I will test this out and give you an update. |
Contributor
… requests
The URI standard RFC 3986 is ambiguous about whether percent encoding and their represented characters are considered equivalent. VS Code considers them equivalent and treats them the same:
```js
vscode.Uri.parse("x://a?b=xxxx%3Dyyyy").toString() -> 'x://a?b%3Dxxxx%3Dyyyy'
vscode.Uri.parse("x://a?b=xxxx%3Dyyyy").toString(/*skipEncoding=*/true) -> 'x://a?b=xxxx=yyyy'
```
This causes issues because SourceKit-LSP's macro expansion URLs encoded by URLComponents use `=` do denote the separation of a key and a value in the outer query. The value of the `parent` key may itself contain query items, which use the escaped form '%3D'. Simplified, such a URL may look like `scheme://host?parent=scheme://host?line%3D2`.
But after running this through VS Code's URI type `=` and `%3D` get canonicalized and are indistinguishable.
To avoid this ambiguity, always percent escape the characters we use to distinguish URL query parameters, producing the following URL: `scheme://host?parent%3Dscheme://host%3Fline%253D2`.
ahoppen
force-pushed
the
more-percent-encoding
branch
from
b96f8e9 to
b2c17c7
Compare
Member
Author
|
@swift-ci Please test |
Member
Author
|
@swift-ci Please test Windows |
hamishknight
approved these changes
Aug 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The URI standard RFC 3986 is ambiguous about whether percent encoding and their represented characters are considered equivalent. VS Code considers them equivalent and treats them the same:
This causes issues because SourceKit-LSP's macro expansion URLs encoded by URLComponents use
=do denote the separation of a key and a value in the outer query. The value of theparentkey may itself contain query items, which use the escaped form '%3D'. Simplified, such a URL may look likescheme://host?parent=scheme://host?line%3D2.But after running this through VS Code's URI type
=and%3Dget canonicalized and are indistinguishable.To avoid this ambiguity, always percent escape the characters we use to distinguish URL query parameters, producing the following URL:
scheme://host?parent%3Dscheme://host%3Fline%253D2.EDIT: This should be a temporary fix. After some URI spec reading, I’ve concluded that the URI decoding in VS Code is incorrect (swiftlang/vscode-swift#1017 (review) and swiftlang/vscode-swift#1017 (comment)).
I would like to merge this PR for now to enable semantic functionality in nested macro expansions for @lokesh-tr’s GSoC project and the investigate a proper fix for URI de/encoding on the VS Code side after which we can revert this workaround.