[#247] Add .clangd configuration file syntax checker #249
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,115 @@ | ||||||||||||||||||||||||||||||||||
| /******************************************************************************* | ||||||||||||||||||||||||||||||||||
| * Copyright (c) 2024 Bachmann electronic GmbH and others. | ||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
| * This program and the accompanying materials are made | ||||||||||||||||||||||||||||||||||
| * available under the terms of the Eclipse Public License 2.0 | ||||||||||||||||||||||||||||||||||
| * which is available at https://www.eclipse.org/legal/epl-2.0/ | ||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
| * SPDX-License-Identifier: EPL-2.0 | ||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||
| * Contributors: | ||||||||||||||||||||||||||||||||||
| * Gesa Hentschke (Bachmann electronic GmbH) - initial implementation | ||||||||||||||||||||||||||||||||||
| *******************************************************************************/ | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| package org.eclipse.cdt.lsp.internal.clangd; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import java.io.IOException; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import org.eclipse.cdt.lsp.internal.clangd.editor.ClangdPlugin; | ||||||||||||||||||||||||||||||||||
| import org.eclipse.core.resources.IFile; | ||||||||||||||||||||||||||||||||||
| import org.eclipse.core.resources.IMarker; | ||||||||||||||||||||||||||||||||||
| import org.eclipse.core.resources.IResource; | ||||||||||||||||||||||||||||||||||
| import org.eclipse.core.runtime.CoreException; | ||||||||||||||||||||||||||||||||||
| import org.eclipse.core.runtime.Platform; | ||||||||||||||||||||||||||||||||||
| import org.yaml.snakeyaml.Yaml; | ||||||||||||||||||||||||||||||||||
| import org.yaml.snakeyaml.error.MarkedYAMLException; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Checks the <code>.clangd</code> file for syntax errors and notifies the user via error markers in the file and Problems view. | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| public class ClangdConfigFileChecker { | ||||||||||||||||||||||||||||||||||
| public static final String CLANGD_MARKER = ClangdPlugin.PLUGIN_ID + ".config.marker"; //$NON-NLS-1$ | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Checks if the .clangd file contains valid yaml syntax. Adds error marker to the file if not. | ||||||||||||||||||||||||||||||||||
| * @param configFile | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| public void checkConfigFile(IFile configFile) { | ||||||||||||||||||||||||||||||||||
| Yaml yaml = new Yaml(); | ||||||||||||||||||||||||||||||||||
| try (var inputStream = configFile.getContents()) { | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| 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$ | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||||||||||||||||||||||||||||
| } catch (IOException | CoreException e) { | ||||||||||||||||||||||||||||||||||
| Platform.getLog(getClass()).error(e.getMessage(), e); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| private void addMarkerToClangdConfig(IFile configFile, MarkedYAMLException yamlException) { | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| var configMarker = parseYamlException(yamlException); | ||||||||||||||||||||||||||||||||||
| var marker = configFile.createMarker(CLANGD_MARKER); | ||||||||||||||||||||||||||||||||||
| marker.setAttribute(IMarker.MESSAGE, configMarker.message); | ||||||||||||||||||||||||||||||||||
| marker.setAttribute(IMarker.SEVERITY, IMarker.SEVERITY_ERROR); | ||||||||||||||||||||||||||||||||||
| marker.setAttribute(IMarker.LINE_NUMBER, configMarker.line); | ||||||||||||||||||||||||||||||||||
| marker.setAttribute(IMarker.CHAR_START, configMarker.charStart); | ||||||||||||||||||||||||||||||||||
| marker.setAttribute(IMarker.CHAR_END, configMarker.charEnd); | ||||||||||||||||||||||||||||||||||
| } catch (CoreException core) { | ||||||||||||||||||||||||||||||||||
| Platform.getLog(getClass()).log(core.getStatus()); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| private void removeMarkerFromClangdConfig(IFile configFile) { | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| configFile.deleteMarkers(CLANGD_MARKER, false, IResource.DEPTH_INFINITE); | ||||||||||||||||||||||||||||||||||
| } catch (CoreException e) { | ||||||||||||||||||||||||||||||||||
| Platform.getLog(getClass()).log(e.getStatus()); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| private class ClangdConfigMarker { | ||||||||||||||||||||||||||||||||||
| public String message; | ||||||||||||||||||||||||||||||||||
| public int line = 1; | ||||||||||||||||||||||||||||||||||
| public int charStart = -1; | ||||||||||||||||||||||||||||||||||
| public int charEnd = -1; | ||||||||||||||||||||||||||||||||||
jonahgraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Fetch line and char position information from exception to create a marker for the .clangd file. | ||||||||||||||||||||||||||||||||||
| * @param exception | ||||||||||||||||||||||||||||||||||
| * @param file | ||||||||||||||||||||||||||||||||||
| * @return | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Possible NPE here? A quick look says that MarkedYAMLException probably always has non-null problemMark, but an NPE here would be unfortunate. |
||||||||||||||||||||||||||||||||||
| int index = exception.getProblemMark().getIndex(); | ||||||||||||||||||||||||||||||||||
| var buffer = exception.getProblemMark().getBuffer(); | ||||||||||||||||||||||||||||||||||
| if (index == buffer.length) { | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. 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) There was a problem hiding this comment.
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);
}
}There was a problem hiding this comment. 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? There was a problem hiding this comment.
When There was a problem hiding this comment. Or, as above, when there is no \r\n, index + 1 > buffer.length |
||||||||||||||||||||||||||||||||||
| index = getIndexOfLastPrintableChar(buffer); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| marker.charStart = index; | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. 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: bobrenders the error as expected:
But if you use a character requiring surrogate pairs such as 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
There was a problem hiding this comment. This seems hard to solve. As you mentioned above, all we have is the number of bytes:
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 There was a problem hiding this comment.
I'll see if I can solve this later. I can open a separate issue for that.
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:
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. |
||||||||||||||||||||||||||||||||||
| marker.charEnd = index + 1; | ||||||||||||||||||||||||||||||||||
| return marker; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| private int getIndexOfLastPrintableChar(int[] buffer) { | ||||||||||||||||||||||||||||||||||
| for (int i = buffer.length - 1; i >= 0; i--) { | ||||||||||||||||||||||||||||||||||
| if ('\r' != ((char) buffer[i]) && '\n' != ((char) buffer[i])) { | ||||||||||||||||||||||||||||||||||
| return i; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| return Math.max(0, buffer.length - 2); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| /******************************************************************************* | ||
| * Copyright (c) 2024 Bachmann electronic GmbH and others. | ||
| * | ||
| * This program and the accompanying materials are made | ||
| * available under the terms of the Eclipse Public License 2.0 | ||
| * which is available at https://www.eclipse.org/legal/epl-2.0/ | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| * | ||
| * Contributors: | ||
| * Gesa Hentschke (Bachmann electronic GmbH) - initial implementation | ||
| *******************************************************************************/ | ||
|
|
||
| package org.eclipse.cdt.lsp.internal.clangd; | ||
|
|
||
| import java.util.concurrent.ConcurrentLinkedQueue; | ||
|
|
||
| import org.eclipse.core.resources.IFile; | ||
| import org.eclipse.core.resources.IResourceChangeEvent; | ||
| import org.eclipse.core.resources.IResourceChangeListener; | ||
| import org.eclipse.core.resources.IResourceDelta; | ||
| import org.eclipse.core.resources.IWorkspace; | ||
| import org.eclipse.core.resources.WorkspaceJob; | ||
| import org.eclipse.core.runtime.CoreException; | ||
| import org.eclipse.core.runtime.IProgressMonitor; | ||
| import org.eclipse.core.runtime.IStatus; | ||
| import org.eclipse.core.runtime.Platform; | ||
| import org.eclipse.core.runtime.Status; | ||
|
|
||
| /** | ||
| * Monitor changes in <code>.clangd</code> files in the workspace and triggers a yaml checker | ||
| * to add error markers to the <code>.clangd</code> file when the edits causes yaml loader failures. | ||
| */ | ||
| public class ClangdConfigFileMonitor { | ||
| private static final String CLANGD_CONFIG_FILE = ".clangd"; //$NON-NLS-1$ | ||
| private final ConcurrentLinkedQueue<IFile> pendingFiles = new ConcurrentLinkedQueue<>(); | ||
| private final IWorkspace workspace; | ||
| private final ClangdConfigFileChecker checker = new ClangdConfigFileChecker(); | ||
|
|
||
| private final IResourceChangeListener listener = new IResourceChangeListener() { | ||
| @Override | ||
| public void resourceChanged(IResourceChangeEvent event) { | ||
| if (event.getDelta() != null && event.getType() == IResourceChangeEvent.POST_CHANGE) { | ||
| try { | ||
| event.getDelta().accept(delta -> { | ||
| if ((delta.getKind() == IResourceDelta.ADDED | ||
| || (delta.getFlags() & IResourceDelta.CONTENT) != 0) | ||
| && CLANGD_CONFIG_FILE.equals(delta.getResource().getName())) { | ||
| if (delta.getResource() instanceof IFile file) { | ||
| pendingFiles.add(file); | ||
| checkJob.schedule(100); | ||
| } | ||
| } | ||
| return true; | ||
| }); | ||
| } catch (CoreException e) { | ||
| Platform.getLog(getClass()).log(e.getStatus()); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| public ClangdConfigFileMonitor(IWorkspace workspace) { | ||
| this.workspace = workspace; | ||
| } | ||
|
|
||
| private final WorkspaceJob checkJob = new WorkspaceJob("Check .clangd file") { //$NON-NLS-1$ | ||
|
|
||
| @Override | ||
| public IStatus runInWorkspace(IProgressMonitor monitor) throws CoreException { | ||
| while (pendingFiles.peek() != null) { | ||
| checker.checkConfigFile(pendingFiles.poll()); | ||
| } | ||
| return Status.OK_STATUS; | ||
| } | ||
|
|
||
| }; | ||
|
|
||
| public ClangdConfigFileMonitor start() { | ||
| workspace.addResourceChangeListener(listener); | ||
| return this; | ||
| } | ||
|
|
||
| public void stop() { | ||
| workspace.removeResourceChangeListener(listener); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.