Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#247] Add .clangd configuration file syntax checker #249

Merged
merged 5 commits into from
Feb 9, 2024
Merged

[#247] Add .clangd configuration file syntax checker #249

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
64f8d69
[#247] Add .clangd configuration file syntax checker
ghentschke Feb 6, 2024
c714fe7
[#247] Add .clangd configuration file syntax checker
ghentschke Feb 7, 2024
8792280
[#247] Add .clangd configuration file syntax checker
ghentschke Feb 7, 2024
b64d55a
[#247] Add .clangd configuration file syntax checker
ghentschke Feb 7, 2024
4493b67
[#247] Add .clangd configuration file syntax checker
ghentschke Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions ...lipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,23 +44,24 @@ public void checkConfigFile(IFile configFile) {
removeMarkerFromClangdConfig(configFile);
//throws ScannerException and ParserException:
yaml.load(inputStream);
} catch (Exception exception) {
if (exception instanceof MarkedYAMLException yamlException) {
addMarkerToClangdConfig(configFile, yamlException);
} else {
//log unexpected exception:
Platform.getLog(getClass())
.error("Expected MarkedYAMLException, but was: " + exception.getMessage(), exception); //$NON-NLS-1$
} catch (MarkedYAMLException yamlException) {
// re-read the file, because the buffer which comes along with MarkedYAMLException is limited to ~800 bytes.
try (var reReadStream = configFile.getContents()) {
addMarkerToClangdConfig(configFile, yamlException, reReadStream.readAllBytes());
}
} catch (Exception exception) {
//log unexpected exception:
Platform.getLog(getClass()).error("Expected MarkedYAMLException, but was: " + exception.getMessage(), //$NON-NLS-1$
exception);
}
} catch (IOException | CoreException e) {
Platform.getLog(getClass()).error(e.getMessage(), e);
}
}

private void addMarkerToClangdConfig(IFile configFile, MarkedYAMLException yamlException) {
private void addMarkerToClangdConfig(IFile configFile, MarkedYAMLException yamlException, byte[] buffer) {
try {
var configMarker = parseYamlException(yamlException);
var configMarker = parseYamlException(yamlException, buffer);
var marker = configFile.createMarker(CLANGD_MARKER);
marker.setAttribute(IMarker.MESSAGE, configMarker.message);
marker.setAttribute(IMarker.SEVERITY, IMarker.SEVERITY_ERROR);
Expand Down Expand Up @@ -93,21 +94,28 @@ private class ClangdConfigMarker {
* @param file
* @return
*/
private ClangdConfigMarker parseYamlException(MarkedYAMLException exception) {
private ClangdConfigMarker parseYamlException(MarkedYAMLException exception, byte[] buffer) {
var marker = new ClangdConfigMarker();
marker.message = exception.getProblem();
marker.line = exception.getProblemMark().getLine() + 1; //getLine() is zero based, IMarker wants 1-based
int index = exception.getProblemMark().getIndex();
var buffer = exception.getProblemMark().getBuffer();
var problemMark = exception.getProblemMark();
if (problemMark == null) {
return marker;
}
marker.line = problemMark.getLine() + 1; //getLine() is zero based, IMarker wants 1-based
int index = problemMark.getIndex();
if (index == buffer.length) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is an incorrect assumption in this if - that is that buffer is the whole file contents. This is probably true for small files, but not so for bigger files. getPointer() is the index in the buffer, and getIndex() is the index in the file (but in code points, not bytes or characters)

Copy link
Contributor Author

@ghentschke ghentschke Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is an incorrect assumption in this if - that is that buffer is the whole file contents. This is probably true for small files, but not so for bigger files

This implies that we should/could not rely on the buffer delivered with the exception? If so, I've to re-read the file in the MarkedYAMLException catch clause to determine the document/file length:

			} catch (MarkedYAMLException yamlException) {
                                try (var reReadStream = configFile.getContents()) {
				     var bufferLength = reReadStream.readAllBytes().length;
				      addMarkerToClangdConfig(configFile, yamlException, bufferLength);
                               }
			}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code snippet is the number of bytes in the file :-( number of bytes, chars and code points can all be different.

Turning the code point index back into char pr byte index is a non-trivial operation. I would say it probably isn't worth it.

What is the problem that using the length of the file is trying to solve? Is it an out of bound exception on the charEnd by just adding 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the problem that using the length of the file is trying to solve? Is it an out of bound exception on the charEnd by just adding 1?

When problemMark.getIndex() == buffer.length() then the marker index points to the non visible \r or \n character and the marker cannot be displayed:
image
The maker is only visible on the file in the project explorer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, as above, when there is no \r\n, index + 1 > buffer.length

index = getIndexOfLastPrintableChar(buffer);
// When index == buffer.length() the marker index points to the non visible
// \r or \n character and the marker is not displayed in the editor.
// Or, even worse, there is no next line and index + 1 would be > buffer.length
// Therefore we have to find the last visible char:
index = getIndexOfLastVisibleChar(buffer);
}
marker.charStart = index;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a small error here, index is in codePoints, but charStart is measured in characters. So you can get some slightly offset files. Consider this file that uses tab incorrectly:

bob:	bob

renders the error as expected:

image

But if you use a character requiring surrogate pairs such as 😂 you get an off by one error. If there are lots of such characters before the error in the file it can be quite far off.

b😂b:	bob

image

alice: 😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂
john: 😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂
bob:	bob

image

In this case since the error is a single character wide, you can end up having no red underline at all because the range is less than a displayed character:

alice: 😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂
john: 😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂😂
bob:	bob

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems hard to solve. As you mentioned above, all we have is the number of bytes:

That code snippet is the number of bytes in the file :-( number of bytes, chars and code points can all be different.

Turning the code point index back into char pr byte index is a non-trivial operation. I would say it probably isn't worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - it is a small (but interesting) problem. I am not convinced you need to solve it, but just be aware of it.

You can choose to not solve it, and leave it as a known problem that it will get the charStart/End correct only if bytes == chars == code points. I don't know how often this will be a real world problem.

Same thing for the above end of file.

I think it is ok to have this deficiency because the error marker will be present on the file and error message will be correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS If you do want to solve the problem, you can read the number of code points in the file and then you will have exact indexes. To do this you need to do BOM handling (see org.yaml.snakeyaml.reader.UnicodeReader) and convert char to code point (see org.yaml.snakeyaml.reader.StreamReader.update() or org.yaml.snakeyaml.error.Mark.toCodePoints(char[]) for code you may be able to call)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - it is a small (but interesting) problem. I am not convinced you need to solve it, but just be aware of it.
You can choose to not solve it, and leave it as a known problem that it will get the charStart/End correct only if bytes == chars == code points. I don't know how often this will be a real world problem.

I'll see if I can solve this later. I can open a separate issue for that.

Same thing for the above end of file.

It is indeed a problem on larger files. I've to re-read the file into a buffer stream to get the end of the buffer. The buffer in the MarkedYAMLException was limited to ~800 bytes:
image

I think it is ok to have this deficiency because the error marker will be present on the file and error message will be correct.

IMO the missing error marker at the end of the file is more relevant, because I think it is more a real world problem to forget the final } than using non UTF-8 characters in a txt file. So I would like to add the re-read of the file to the catch clause at mentioned here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

marker.charEnd = index + 1;
return marker;
}

private int getIndexOfLastPrintableChar(int[] buffer) {
private int getIndexOfLastVisibleChar(byte[] buffer) {
for (int i = buffer.length - 1; i >= 0; i--) {
if ('\r' != ((char) buffer[i]) && '\n' != ((char) buffer[i])) {
return i;
Expand Down
17 changes: 11 additions & 6 deletions ...angd.tests/src/org/eclipse/cdt/lsp/internal/clangd/tests/ClangdConfigFileCheckerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.eclipse.cdt.lsp.internal.clangd.ClangdConfigFileChecker;
import org.eclipse.cdt.lsp.internal.clangd.ClangdConfigFileMonitor;
import org.eclipse.core.resources.IFile;
import org.eclipse.core.resources.IMarker;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.runtime.CoreException;
Expand Down Expand Up @@ -132,15 +133,19 @@ void testInvalidYamlSyntaxMissingBrace() throws IOException, CoreException {
// GIVEN an existing .clangd configuration file with invalid yaml syntax (missing closing brace):
// WHEN a new .clang file gets added to the project (should trigger ClangdConfigFileMonitor.checkJob).
var configFile = createConfigFile(INVALID_YAML_SYNTAX_MISSING_BRACE);
// AND the UI thread sleeps for 1s:
try {
Thread.sleep(1000);
} catch (InterruptedException e) {
e.printStackTrace();
int cnt = 0;
var marker = new IMarker[] {};
while (marker.length == 0 && cnt < 20) {
marker = configFile.findMarkers(ClangdConfigFileChecker.CLANGD_MARKER, false, IResource.DEPTH_ZERO);
cnt++;
try {
Thread.sleep(50);
} catch (InterruptedException e) {
e.printStackTrace();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In test you can just let the InterruptedException be thrown by the test method. It makes the test shorter, and if a thread is interrupted here that would be unexpected, so having the stacktrace saved in the test results (instead of just stderr) is good.

Suggested change
try {
Thread.sleep(50);
} catch (InterruptedException e) {
e.printStackTrace();
}
Thread.sleep(50);

}
// THEN we expect that an ClangdConfigFileChecker.CLANGD_MARKER has been added in the meantime,
// because the ClangdConfigFileMonitor.checkJob, which calls the ClangdConfigFileChecker().checkConfigFile, should have been run after a delay of 100ms:
var marker = configFile.findMarkers(ClangdConfigFileChecker.CLANGD_MARKER, false, IResource.DEPTH_ZERO);
assertNotNull(marker);
assertEquals(1, marker.length, ERROR_MSG);
}
Expand Down
Loading