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

- fix review comments
ghentschke · Feb 7, 2024 · b64d55a · b64d55a
1 parent 8792280
commit b64d55a
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 21 deletions.
diff --git a/...lipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java b/...lipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileChecker.java
@@ -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);
@@ -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) {
-			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;
 		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;

diff --git a/...angd.tests/src/org/eclipse/cdt/lsp/internal/clangd/tests/ClangdConfigFileCheckerTest.java b/...angd.tests/src/org/eclipse/cdt/lsp/internal/clangd/tests/ClangdConfigFileCheckerTest.java
@@ -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;
@@ -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();
+			}
 		}
 		// 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);
 	}