Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

Alternative attempt to support rangeLength field in LSP #283

Merged
merged 1 commit into from
May 2, 2017

Conversation

albel727
Copy link

@albel727 albel727 commented Apr 28, 2017

This is another attempt to fix the issue #280.

Since the pull request #281 ran into problems due to the fact that rls-span and rls-data crates are actually used as API for interaction with Rust compiler, and have an embedded version of themselves inside it, changing them doesn't seem a good idea anymore.

Hence this alternative less invasive approach, which touches only rls and rls-vfs crates. Since it keeps using rls-span 0.1 and rls-data 0.1, it works and passes all tests locally, including fresh compilers, and so I expect Travis to confirm.

I've reverted all changes in rls-analysis and rls-vfs that were made in preparation of #281, as they're conflicting with this attempt, and further edited rls-vfs to support this new way, which basically consists of adding len field to ReplaceText event of rls <-> rls-vfs interface.

I will send PRs with those changes to the nrc repos, once Travis checks this build. Meanwhile, I've pointed Cargo.toml to my fixed repos, and you can see what's in them here.

rust-dev-tools/rls-vfs@master...albel727:master
rust-dev-tools/rls-analysis@master...albel727:master

Luckily, changes to rls-span and rls-data need not be reverted immediately, as they're depended upon not as git repos, but through crates.io. Let's leave the decision of what to do with them to nrc.

@albel727
Copy link
Author

cc @jonathandturner @skeleten

@nrc
Copy link
Member

nrc commented May 2, 2017

I think this approach looks better. The rls-vfs change looks fine, there is no change to rls-analysis that I can see from the diffs, and the change here looks good. I guess we need the rls-vfs change to land, then I'll publish to crates.io. Then this PR will need a rebase and we can land it. For rls-span and rls-data, we should revert the len field changes and issue new versions (0.3 and 0.4) which are identical to 0.1 and 0.2.

@nrc
Copy link
Member

nrc commented May 2, 2017

And thanks for digging in to this problem!

@nrc
Copy link
Member

nrc commented May 2, 2017

rls-vfs v0.2.0 is now on crates.io

@albel727
Copy link
Author

albel727 commented May 2, 2017

For rls-span and rls-data, we should revert the len field changes and issue new versions (0.3 and 0.4) which are identical to 0.1 and 0.2.

Note, that rls-data has another breaking change besides len field changes, namely addition of repr(C) on Analysis.

rust-dev-tools/rls-data@2f18261

If this is to remain in 0.4, dependencies in the Rust compiler will have to be updated too, and this will break compat with older compilers, though admittedly it will probably mean less subtle breakage in the future if Rust changes field ordering and such in repr(Rust).

If it's not to remain, I'm not quite sure why new versions of rls-span and rls-data are even needed. Everything should just keep depending on 0.1 versions, IMO. At least there seems to be no need to publish new rls-span/rls-data to crates.io, until some actual change warranting a version bump appears.

@nrc
Copy link
Member

nrc commented May 2, 2017

So, I plan to update the compiler + RLS with that change soon. I'd rather remove the len field from those repos so it is not hanging around to trip people up. Also it feels wrong for the 'right' version of the crate not to be the most recent.

In the short term, we should be on 0.1 of rls-data and 0.3 of rls-span, and I will upgrade use to 0.4 of rls-data later.

@albel727 albel727 force-pushed the rangelength-take-two branch from 97c4c90 to e61a3d0 Compare May 2, 2017 10:48
@albel727
Copy link
Author

albel727 commented May 2, 2017

Rebased on top of master and eliminated dependencies on my repositories. rls-vfs 0.2 and rls-analysis 0.1 from crates.io are used instead. rls-data and rls-span remain on 0.1.

@nrc nrc merged commit 53fa8ca into rust-lang:master May 2, 2017
@nrc
Copy link
Member

nrc commented May 2, 2017

Thank you!

@albel727 albel727 deleted the rangelength-take-two branch May 2, 2017 21:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants