Deprecate ByteSourceRange in favor of Range<AbsolutePosition>
#2588
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
|
@swift-ci Please test |
ahoppen
added a commit
to ahoppen/sourcekit-lsp
that referenced
this pull request
Apr 4, 2024
…ent:)` This initializer was only introduced very recently since `IncrementalEdit` stores the replacement bytes instead of just the replacement length. We are now changing `IncrementalEdit` store the range it’s replacing as `Range<AbsolutePosition>` instead of `ByteSourceRange`. Companion of swiftlang/swift-syntax#2588
|
@swift-ci Please test |
|
@swift-ci Please test macOS |
bnbarham
approved these changes
Apr 5, 2024
| /// at the given offset is unclassified. | ||
| @available(*, deprecated, message: "Use classification(at: AbsolutePosition) instead") |
There was a problem hiding this comment.
This deprecation feels a little odd since it actually has different behavior. Should we make that clear in the deprecation message?
There was a problem hiding this comment.
Good point. I changed it to maintain the same behavior it had before the PR and kept the deprecation message the same.
ahoppen
force-pushed
the
ahoppen/deprecate-bytesourcerange
branch
from
22435e4 to
0dfe3a4
Compare
hamishknight
approved these changes
Apr 12, 2024
ahoppen
force-pushed
the
ahoppen/deprecate-bytesourcerange
branch
from
0dfe3a4 to
e51cc5a
Compare
|
@swift-ci Please test |
rintaro
reviewed
Apr 16, 2024
| /// Returns the range for the overlapping region between two ranges. | ||
| /// | ||
| /// If the intersection is empty, this returns `nil`. | ||
| public func intersecting(_ other: Range<AbsolutePosition>) -> Range<AbsolutePosition>? { |
There was a problem hiding this comment.
I feel like we should use Range methods like clamped(to:) / overlaps(_:) . Is there a reason we can't?
There was a problem hiding this comment.
Very good suggestion. Added a commit that deprecates intersection-style methods.
While being a little more verbose, this has a few advantages: - We only have a single type to represent ranges in SwiftSyntax instead of `Range<AbsolutePosition>` and `ByteSourceRange`, both of which we needed to use in sourcekit-lsp - Unifying the convenience functions on the two results in a single type that has all the convenience functions, instead of spreading them to two distinct sets - The use of `AbsolutePosition` and `SourceLength` makes type-system guarantees that these are UTF-8 byte positions / length, making it harder to accidentally add eg. UTF-16 lengths with UTF-8 lengths. rdar://125624626
ahoppen
force-pushed
the
ahoppen/deprecate-bytesourcerange
branch
from
e51cc5a to
dca214e
Compare
|
@swift-ci Please test |
|
@swift-ci Please test |
rintaro
reviewed
Apr 18, 2024
Release Notes/600.md
Outdated
| - Description: `SyntaxIdentifier.indexInTree` allows the retrieval of a `SyntaxIdentifier` that identifies the syntax node independent of the syntax tree. `SyntaxIdentifier.fromIndexInTree` allows the creation for a `SyntaxIdentifier` from a tree-agnostic `SyntaxIdentifier.IndexInTree` and the tree's root node. | ||
| - Pull request: https://github.com/apple/swift-syntax/pull/2594 | ||
|
|
||
| - `Range<AbsolutePosition>` | ||
| - Description: `Range<AbsolutePosition>` gained a few convenience functions inspired from `ByteSourceRange`: `init(position:length:)`, `length`, `intersectsOrTouches`, `intersects`, `intersecting` |
There was a problem hiding this comment.
Could you update this for overlaps()/clamped(to:) style APIs?
There was a problem hiding this comment.
Thanks for noticing. I forgot about the release notes
…utePosition>` We should use the native Range methods `clamped(to:)` and `overlaps` here. To match that naming scheme `intersectsOrTouches` has been renamed to `overlapsOrTouches`.
ahoppen
force-pushed
the
ahoppen/deprecate-bytesourcerange
branch
from
dca214e to
b66915f
Compare
|
@swift-ci Please test |
rintaro
approved these changes
Apr 18, 2024
|
@swift-ci Please test Windows |
|
@swift-ci Please test Windows |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
While being a little more verbose, this has a few advantages:
Range<AbsolutePosition>andByteSourceRange, both of which we needed to use in sourcekit-lspAbsolutePositionandSourceLengthmakes type-system guarantees that these are UTF-8 byte positions / length, making it harder to accidentally add eg. UTF-16 lengths with UTF-8 lengths.rdar://125624626