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

ghentschke · 2024-02-06T20:30:43Z

Inform the user via markers in the .clangd file when the syntax cannot be parsed, because this leads to problems in the
ClangdConfigurationFileManager. The path to the compile_commands.json cannot be set in the .clangd file when the .clangd file is corrupted. A marker informs the user about the problem:

fixes #247

- Inform the user via markers in the .clangd file when the syntax cannot be parsed, because this leads to problems in the ClangdConfigurationFileManager.

ghentschke · 2024-02-06T20:31:54Z

Please notice: I've to add some unit tests. I'am working on that.

jonahgraham · 2024-02-07T02:00:23Z

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java

+				//throws ScannerException and ParserException:
+				yaml.load(inputStream);
+			} catch (Exception yamlException) {
+				addMarkerToClangdConfig(configFile, yamlException);


Many Yaml exceptions - like the one raised in #245 (comment) - are of type MarkedYAMLException which have some utility methods like getProblemMark and getProblem which will allow you to deconstruct the exception without having to parse the string.

I haven't tried to use them in this context, but it looks like it should do the job

Thank you for hint! It's much easier!

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java

jonahgraham · 2024-02-07T02:25:40Z

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileMonitor.java

+			if (event.getDelta() != null && event.getType() == IResourceChangeEvent.POST_CHANGE) {
+				try {
+					event.getDelta().accept(delta -> {
+						if ((delta.getKind() == IResourceDelta.ADDED || delta.getKind() == IResourceDelta.REMOVED


If the .clangd file is removed we probably don't want to run the checker on it. My reading of this code seems to suggest that a CoreException will be thrown as a result of configFile.getContents()

jonahgraham · 2024-02-07T02:29:24Z

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileMonitor.java

+						return true;
+					});
+				} catch (CoreException e) {
+					StatusManager.getManager().handle(e, ClangdPlugin.PLUGIN_ID);


Why use a StatusManager handle here, but not in the other places that add exceptions to the log?

Sorry, copy and paste error ;-)

- Fetch info from MarkedYAMLException to create a marker for the .clangd file. fixes eclipse-cdt#247

- add unit tests

jonahgraham · 2024-02-07T19:13:51Z

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java

+			} 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$
+				}
+			}


Suggested change

} 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) {

addMarkerToClangdConfig(configFile, yamlException);

} catch (Exception exception) {

//log unexpected exception:

Platform.getLog(getClass())

.error("Expected MarkedYAMLException, but was: " + exception.getMessage(), exception); //$NON-NLS-1$

}

jonahgraham · 2024-02-07T19:19:14Z

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java

+	private ClangdConfigMarker parseYamlException(MarkedYAMLException exception) {
+		var marker = new ClangdConfigMarker();
+		marker.message = exception.getProblem();
+		marker.line = exception.getProblemMark().getLine() + 1; //getLine() is zero based, IMarker wants 1-based


Possible NPE here? A quick look says that MarkedYAMLException probably always has non-null problemMark, but an NPE here would be unfortunate.

jonahgraham · 2024-02-07T19:32:19Z

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java

+		marker.line = exception.getProblemMark().getLine() + 1; //getLine() is zero based, IMarker wants 1-based
+		int index = exception.getProblemMark().getIndex();
+		var buffer = exception.getProblemMark().getBuffer();
+		if (index == buffer.length) {


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)

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); } }

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?

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:

The maker is only visible on the file in the project explorer.

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

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java

jonahgraham · 2024-02-07T19:53:35Z

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java

+		if (index == buffer.length) {
+			index = getIndexOfLastPrintableChar(buffer);
+		}
+		marker.charStart = index;


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:

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

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

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

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.

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.

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)

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:

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.

Sounds good.

jonahgraham · 2024-02-07T20:34:53Z

....clangd.tests/src/org/eclipse/cdt/lsp/internal/clangd/tests/ClangdConfigFileCheckerTest.java

+		// 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:


I assume this is to test that the marker gets created? Instead can you test in a loop for the end condition so that the test doesn't wait longer than needed.

- fix review comments

jonahgraham · 2024-02-08T00:12:25Z

....clangd.tests/src/org/eclipse/cdt/lsp/internal/clangd/tests/ClangdConfigFileCheckerTest.java

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


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);

jonahgraham · 2024-02-08T00:13:19Z

....eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java

+		if (index == buffer.length) {
+			index = getIndexOfLastPrintableChar(buffer);
+		}
+		marker.charStart = index;


Sounds good.

- fix review comments - use IResource.DEPTH_ZERO in deleteMarkers, since the config file has no members.

ghentschke · 2024-02-09T07:17:12Z

@jonahgraham Thank your the many good hints!

[eclipse-cdt#247] Add .clangd configuration file syntax checker

64f8d69

- Inform the user via markers in the .clangd file when the syntax cannot be parsed, because this leads to problems in the ClangdConfigurationFileManager.

ghentschke requested review from ruspl-afed and jonahgraham February 6, 2024 20:30

jonahgraham reviewed Feb 7, 2024

View reviewed changes

[eclipse-cdt#247] Add .clangd configuration file syntax checker

c714fe7

- Fetch info from MarkedYAMLException to create a marker for the .clangd file. fixes eclipse-cdt#247

ghentschke requested a review from jonahgraham February 7, 2024 18:55

[eclipse-cdt#247] Add .clangd configuration file syntax checker

8792280

- add unit tests

jonahgraham reviewed Feb 7, 2024

View reviewed changes

[eclipse-cdt#247] Add .clangd configuration file syntax checker

b64d55a

- fix review comments

jonahgraham reviewed Feb 8, 2024

View reviewed changes

[eclipse-cdt#247] Add .clangd configuration file syntax checker

4493b67

- fix review comments - use IResource.DEPTH_ZERO in deleteMarkers, since the config file has no members.

jonahgraham approved these changes Feb 8, 2024

View reviewed changes

ghentschke merged commit d4bca9e into eclipse-cdt:master Feb 9, 2024
3 checks passed

ghentschke deleted the yaml-syntax-checker branch February 9, 2024 07:26

ghentschke added this to the 1.1.0 milestone Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ghentschke commented Feb 6, 2024 •

edited

Loading

ghentschke commented Feb 6, 2024

jonahgraham Feb 7, 2024

ghentschke Feb 7, 2024

jonahgraham Feb 7, 2024

jonahgraham Feb 7, 2024

ghentschke Feb 7, 2024

jonahgraham Feb 7, 2024

jonahgraham Feb 7, 2024

jonahgraham Feb 7, 2024

ghentschke Feb 7, 2024 •

edited

Loading

jonahgraham Feb 7, 2024

ghentschke Feb 7, 2024

ghentschke Feb 7, 2024

jonahgraham Feb 7, 2024

ghentschke Feb 7, 2024

jonahgraham Feb 7, 2024

jonahgraham Feb 7, 2024

ghentschke Feb 7, 2024

jonahgraham Feb 8, 2024

jonahgraham Feb 7, 2024

jonahgraham Feb 8, 2024

jonahgraham Feb 8, 2024

ghentschke commented Feb 9, 2024

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

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

Conversation

ghentschke commented Feb 6, 2024 • edited Loading

ghentschke commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghentschke Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghentschke commented Feb 9, 2024

ghentschke commented Feb 6, 2024 •

edited

Loading

ghentschke Feb 7, 2024 •

edited

Loading