Sanitize the line and column values if they have been reported as negative when converting the to an LSP position

[cpedlar-syn]

Purpose

LSP locations are defined as uint in the specification and the computation for the column values due to the tabs is not correct. Passing a negative value to an LSP server can cause them to report errors.

Goals

Prevent passing negative values to LSP servers

Approach

This particular change prevents the passing of the value, but it may be better to not pass a position at all to the LSP servers instead of a sanitized one. Additionally, the real fix is probably resolving the computation for the column value but at this time I'm not confident what that may be. I'll try to take another look at the computation for the column and see if I can determine why it is incorrect in at least some cases.

Samples

I have a method with the following format in some test code. Hovering over the word "bad" in the method will cause the incorrect column to be reported.

public class Foo {
	public static String giveNull() {
		String bad = null;
		bad += "foo";   // bad
		return bad;
	}
}

Here is a screenshot of the data at that time.

[nixel2007]

I'm not sure that incrementing column number by tab * tabSize is a correct behavior at all. Lsp position is a position in characters. Tab is a character.

[cpedlar-syn]

I'm not sure that incrementing column number by tab * tabSize is a correct behavior at all. Lsp position is a position in characters. Tab is a character.

I was thinking that as well but hadn't had time to really reflect on it. Would that imply that column would just be lineBeforeTextoffset().length() ?

[github-actions]

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

[nixel2007]

Please review

[NipunaRanasinghe]

@cpedlar-syn as our client library itself derive the column number using the offset, sanitising the errornous (negative) outputs might hide the real issue and ends up having unexpected results.

I also feel like we can completely get rid of tabsize based calculations in here, which seems to be a regression introduced with #228.

[NipunaRanasinghe]

@nixel2007 Appreciate your review on #323.

@cpedlar-syn will you be able to verify whether your issue also resolves with the changes proposed with the above PR?

[cpedlar-syn]

@NipunaRanasinghe Yes, I can try to adjust the change to just remove the tab based calculations. I also thought that may be the more correct solution.

I'll try to find some time today to make the changes and circle back, however, if I don't get to it today I won't have another opportunity for ~2 weeks but I will try to make the change as soon as possible.

[github-actions]

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

[nixel2007]

Ping

[NipunaRanasinghe]

Ping

@nixel2007 we can simply close this if #323 looks good.

[cpedlar-syn]

Well #323 now looks the same as this commit I believe. So either should have the same result. This also seems to resolve my issues.

Edit:

I just took too long to get back to this change :-)

[github-actions]

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

[cpedlar-syn]

Closing this PR since the change was merged via another PR.