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 4 commits
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
3 changes: 2 additions & 1 deletion bundles/org.eclipse.cdt.lsp.clangd/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ Require-Bundle: org.eclipse.cdt.lsp;bundle-version="0.0.0",
org.eclipse.ui.ide;bundle-version="0.0.0",
org.eclipse.ui.workbench;bundle-version="0.0.0",
org.eclipse.ui.workbench.texteditor;bundle-version="0.0.0",
org.eclipse.core.variables;bundle-version="0.0.0"
org.eclipse.core.variables;bundle-version="0.0.0",
org.yaml.snakeyaml;bundle-version="0.0.0"
Service-Component: OSGI-INF/org.eclipse.cdt.lsp.clangd.BuiltinClangdOptionsDefaults.xml,
OSGI-INF/org.eclipse.cdt.lsp.clangd.ClangdConfigurationFileManager.xml,
OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.ClangdConfigurationAccess.xml,
Expand Down
11 changes: 11 additions & 0 deletions bundles/org.eclipse.cdt.lsp.clangd/plugin.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,5 +110,16 @@
</command>
</menuContribution>
</extension>
<extension
id="org.eclipse.cdt.lsp.clangd.config.marker"
name=".clangd yaml Problem"
point="org.eclipse.core.resources.markers">
<super
type="org.eclipse.core.resources.problemmarker">
</super>
<persistent
value="true">
</persistent>
</extension>

</plugin>
126 changes: 126 additions & 0 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
@@ -0,0 +1,126 @@
/*******************************************************************************
* 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) {
if (!configFile.exists()) {
return;
}
Yaml yaml = new Yaml();
try (var inputStream = configFile.getContents()) {
jonahgraham marked this conversation as resolved.
Show resolved Hide resolved
try {
removeMarkerFromClangdConfig(configFile);
//throws ScannerException and ParserException:
yaml.load(inputStream);
} 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, byte[] buffer) {
try {
var configMarker = parseYamlException(yamlException, buffer);
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, byte[] buffer) {
var marker = new ClangdConfigMarker();
marker.message = exception.getProblem();
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

// 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 getIndexOfLastVisibleChar(byte[] 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);
}
}
87 changes: 87 additions & 0 deletions ...lipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdConfigFileMonitor.java
View file
Open in desktop
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);
}
}
4 changes: 4 additions & 0 deletions ...g.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/editor/ClangdPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package org.eclipse.cdt.lsp.internal.clangd.editor;

import org.eclipse.cdt.lsp.internal.clangd.CProjectChangeMonitor;
import org.eclipse.cdt.lsp.internal.clangd.ClangdConfigFileMonitor;
import org.eclipse.core.resources.IWorkspace;
import org.eclipse.ui.plugin.AbstractUIPlugin;
import org.osgi.framework.BundleContext;
Expand All @@ -27,6 +28,7 @@ public class ClangdPlugin extends AbstractUIPlugin {
private IWorkspace workspace;
private CompileCommandsMonitor compileCommandsMonitor;
private CProjectChangeMonitor cProjectChangeMonitor;
private ClangdConfigFileMonitor configFileMonitor;

// The plug-in ID
public static final String PLUGIN_ID = "org.eclipse.cdt.lsp.clangd"; //$NON-NLS-1$
Expand All @@ -49,13 +51,15 @@ public void start(BundleContext context) throws Exception {
workspace = workspaceTracker.getService();
compileCommandsMonitor = new CompileCommandsMonitor(workspace).start();
cProjectChangeMonitor = new CProjectChangeMonitor().start();
configFileMonitor = new ClangdConfigFileMonitor(workspace).start();
}

@Override
public void stop(BundleContext context) throws Exception {
plugin = null;
compileCommandsMonitor.stop();
cProjectChangeMonitor.stop();
configFileMonitor.stop();
super.stop(context);
}

Expand Down
Loading
Loading