Don't use deprecated rangeLength property in handleChanged

[mfussenegger]

The rangeLength property is deprecated in the specification.

[eclipse-ls-bot]

Can one of the admins verify this patch?

[fbricon]

removing deprecated properties means we might break some clients that have not yet updated to a recent LSP spec (this happened before). I'd rather get feedback from our adopters before moving forward.

[mfussenegger]

removing deprecated properties means we might break some clients that have not yet updated to a recent LSP spec (this happened before). I'd rather get feedback from our adopters before moving forward.

This is not removing the property itself. It just means that is infers the length from the (required) range property instead.

Part of the motivation for the PR was that in neovim we considered dropping sending the property, because in the current version of the specification it is also marked as optional and partly because the computation is a bit painful in neovim due to the offset conversation we have to do due to utf8/utf-16 missmatch. Many servers we checked already don't use the property anymore.

What do you think of only using rangeLength if present and otherwise fallback to the length calculation as proposed in the PR as a compromise? Currently if the client provides a range but no rangeLength the sync breaks completely

export type TextDocumentContentChangeEvent = {
	/**
	 * The range of the document that changed.
	 */
	range: Range;

	/**
	 * The optional length of the range that got replaced.
	 *
	 * @deprecated use range instead.
	 */
	rangeLength?: uinteger;

	/**
	 * The new text for the provided range.
	 */
	text: string;
} | {
	/**
	 * The new text of the whole document.
	 */
	text: string;
};

[fbricon]

add to whitelist

[fbricon]

I'm just skimming, but looks like the PR is probably fine.
@rgrunber can you please review in-depth?

[rgrunber]

Yup handleChanged(..) is identical between BaseDocumentLifeCycleHandler and DocumentLifeCycleHandler (except for an empty newline), so the removal is definitely fine. rangeLength seems to be deprecated in TextDocumentContentChangeEvent in favour of range, and the logical way to get the length is to convert the range to file offsets and subtract start from end. I'll have a closer look here to confirm it's really fine, but likely is given the tests are passing.

• Could we also remove https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/DocumentLifeCycleHandlerTest.java#L871 ? We're not using getRangeLength() anywhere with your change so I'm guessing that's not needed either.

• Can we also remove sharedASTProvider in DocumentLifeCycleHandler ? It looks like with your change, it isn't used at all, and is a duplicate reference available in the base class. Also I would clean up the imports in this class as a bunch are no longer needed.

[mfussenegger]

Thanks for the review. I think I've addressed all points.

I kept dedicated fixup commits to make the re-review easier. I can squash them once the PR is otherwise good to go.

Btw, locally I get a failure, but only if I run the full suite via ./mvnw verify, not if I try to run the individual test or even the full test class. Also happens on master without this change. Any ideas what could cause this? Maybe some test order dependency?

java.lang.AssertionError
	at org.eclipse.jdt.ls.core.internal.managers.GradleProjectImporterTest.testDeleteClasspath(GradleProjectImporterTest.java:453)

[rgrunber]

I think this change can be merged, though I'd prefer just that small replacement of those lines that calculate length, with the helper method for that.

I think the failing tests happen occasionally. Not sure what causes them, though they're not related to this change. It could be that some orderings of the test runs cause this, or maybe some race condition.

[0dinD]

I was thinking about submitting a PR for code cleanup in DocumentLifeCycleHandler, but if we're going to do that here, may I suggest removing handleOpen() and handleClosed() as well? These overrides seem to do nothing different from the super implementation in BaseDocumentLifeCycleHandler, just like handleChanged().

[rgrunber]

I was thinking about submitting a PR for code cleanup in DocumentLifeCycleHandler, but if we're going to do that here, may I suggest removing handleOpen() and handleClosed() as well? These overrides seem to do nothing different from the super implementation in BaseDocumentLifeCycleHandler, just like handleChanged().

Looks like this was left over from 7869725#diff-9fe4368770feb305d866b29457ae3c14f89ad289350200e3b2ec9e6819a2e73e , which removed the legacy semantic highlighting. @mfussenegger , could you just remove those 2 methods (handleOpen,handleClosed) in DocumentLifeCycleHandler ? I think it should really be the last thing.