Skip to content

Commit 3e886af

Browse files
palukkukoppor
palukku
and
koppor
authored
LSP Refactor to use ranges instead of string matching (#13862)
* Refactor to use ranges instead of string matching * Refactor to enhance field range handling and utilize aliases consistently * improve range retrieval logic * implement getAlias logic of Field * change article highlighting * fix * enhance error and range handling in parser and fix palukku#50 * address comments * fix ParserResult test * Better data structure for map (to avoid checks) * Fix IntelliJ warning * Fix linebreak * Fix alphabetical ordering * fix checkstyle * add epilogPattern to constructor --------- Co-authored-by: Oliver Kopp <[email protected]>
1 parent f0a95a8 commit 3e886af

File tree

10 files changed

+165
-99
lines changed

10 files changed

+165
-99
lines changed

‎jablib/src/main/java/org/jabref/logic/importer/ParserResult.java‎

Lines changed: 60 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import java.nio.file.Path;
44
import java.util.ArrayList;
55
import java.util.Collection;
6+
import java.util.Collections;
67
import java.util.HashSet;
78
import java.util.IdentityHashMap;
89
import java.util.List;
@@ -17,11 +18,15 @@
1718
import org.jabref.model.entry.BibEntry;
1819
import org.jabref.model.entry.BibEntryType;
1920
import org.jabref.model.entry.field.Field;
21+
import org.jabref.model.entry.field.InternalField;
2022
import org.jabref.model.metadata.MetaData;
2123

24+
import com.google.common.collect.Multimap;
25+
import com.google.common.collect.MultimapBuilder;
26+
2227
public class ParserResult {
2328
private final Set<BibEntryType> entryTypes;
24-
private final List<String> warnings = new ArrayList<>();
29+
private final Multimap<Range, String> warnings;
2530
private BibDatabase database;
2631
private MetaData metaData;
2732
private Path file;
@@ -47,11 +52,12 @@ public ParserResult(BibDatabase database, MetaData metaData, Set<BibEntryType> e
4752
this.database = Objects.requireNonNull(database);
4853
this.metaData = Objects.requireNonNull(metaData);
4954
this.entryTypes = Objects.requireNonNull(entryTypes);
55+
this.warnings = MultimapBuilder.hashKeys().hashSetValues().build();
5056
}
5157

5258
public static ParserResult fromErrorMessage(String message) {
5359
ParserResult parserResult = new ParserResult();
54-
parserResult.addWarning(message);
60+
parserResult.addWarning(Range.NULL_RANGE, message);
5561
parserResult.setInvalid(true);
5662
return parserResult;
5763
}
@@ -95,25 +101,31 @@ public void setPath(Path path) {
95101
/**
96102
* Add a parser warning.
97103
*
98-
* @param s String Warning text. Must be pretranslated. Only added if there isn't already a dupe.
104+
* @param s String Warning text. Must be pre-translated. Only added if there isn't already a dupe.
99105
*/
100106
public void addWarning(String s) {
101-
if (!warnings.contains(s)) {
102-
warnings.add(s);
103-
}
107+
addWarning(Range.NULL_RANGE, s);
104108
}
105109

106-
public void addException(Exception exception) {
110+
public void addWarning(Range range, String s) {
111+
warnings.put(range, s);
112+
}
113+
114+
public void addException(Range range, Exception exception) {
107115
String errorMessage = getErrorMessage(exception);
108-
addWarning(errorMessage);
116+
addWarning(range, errorMessage);
109117
}
110118

111119
public boolean hasWarnings() {
112120
return !warnings.isEmpty();
113121
}
114122

115123
public List<String> warnings() {
116-
return new ArrayList<>(warnings);
124+
return new ArrayList<>(warnings.values());
125+
}
126+
127+
public Multimap<Range, String> getWarningsMap() {
128+
return warnings;
117129
}
118130

119131
public boolean isInvalid() {
@@ -162,7 +174,45 @@ public Map<BibEntry, Range> getArticleRanges() {
162174
return articleRanges;
163175
}
164176

165-
public record Range(int startLine, int startColumn, int endLine, int endColumn) {
177+
public record Range(
178+
int startLine,
179+
int startColumn,
180+
int endLine,
181+
int endColumn) {
166182
public static final Range NULL_RANGE = new Range(0, 0, 0, 0);
183+
184+
public Range(int startLine, int startColumn) {
185+
this(startLine, startColumn, startLine, startColumn);
186+
}
187+
}
188+
189+
/// Returns a `Range` indicating that a complete entry is hit. We use the line of the key. No key is found, the complete entry range is used.
190+
public Range getFieldRange(BibEntry entry, Field field) {
191+
Map<Field, Range> rangeMap = fieldRanges.getOrDefault(entry, Collections.emptyMap());
192+
193+
if (rangeMap.isEmpty()) {
194+
return Range.NULL_RANGE;
195+
}
196+
197+
Range range = rangeMap.get(field);
198+
if (range != null) {
199+
return range;
200+
}
201+
202+
return field.getAlias()
203+
.map(rangeMap::get)
204+
.orElseGet(() -> getCompleteEntryIndicator(entry));
205+
}
206+
207+
/// Returns a `Range` indicating that a complete entry is hit. We use the line of the key. No key is found, the complete entry range is used.
208+
public Range getCompleteEntryIndicator(BibEntry entry) {
209+
Map<Field, Range> rangeMap = fieldRanges.getOrDefault(entry, Collections.emptyMap());
210+
Range range = rangeMap.get(InternalField.KEY_FIELD);
211+
if (range != null) {
212+
// this ensures that the line is highlighted from the beginning of the entry so it highlights "@Article{key," (but only if on the same line) and not just the citation key
213+
return new Range(range.startLine(), 0, range.endLine(), range.endColumn());
214+
}
215+
216+
return articleRanges.getOrDefault(entry, Range.NULL_RANGE);
167217
}
168218
}

‎jablib/src/main/java/org/jabref/logic/importer/fileformat/BibtexParser.java‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ public class BibtexParser implements Parser {
9696
private static final String BIB_DESK_ROOT_GROUP_NAME = "BibDeskGroups";
9797
private static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY = DocumentBuilderFactory.newInstance();
9898
private static final int INDEX_RELATIVE_PATH_IN_PLIST = 4;
99+
private final Pattern epilogPattern;
99100
private final Deque<Character> pureTextFromFile = new LinkedList<>();
100101
private final ImportFormatPreferences importFormatPreferences;
101102
private PushbackReader pushbackReader;
@@ -120,6 +121,7 @@ public BibtexParser(ImportFormatPreferences importFormatPreferences, FileUpdateM
120121
this.importFormatPreferences = Objects.requireNonNull(importFormatPreferences);
121122
this.metaDataParser = new MetaDataParser(fileMonitor);
122123
this.parsedBibDeskGroups = new HashMap<>();
124+
this.epilogPattern = Pattern.compile("\\w+\\s*=.*,");
123125
}
124126

125127
public BibtexParser(ImportFormatPreferences importFormatPreferences) {
@@ -270,6 +272,8 @@ private ParserResult parseFileContent() throws IOException {
270272

271273
addBibDeskGroupEntriesToJabRefGroups();
272274

275+
int startLine = line;
276+
int startColumn = column;
273277
try {
274278
MetaData metaData = metaDataParser.parse(
275279
meta,
@@ -296,7 +300,7 @@ private ParserResult parseFileContent() throws IOException {
296300
}
297301
parserResult.setMetaData(metaData);
298302
} catch (ParseException exception) {
299-
parserResult.addException(exception);
303+
parserResult.addException(new ParserResult.Range(startLine, startColumn, line, column), exception);
300304
}
301305

302306
parseRemainingContent();
@@ -309,8 +313,8 @@ private ParserResult parseFileContent() throws IOException {
309313
private void checkEpilog() {
310314
// This is an incomplete and inaccurate try to verify if something went wrong with previous parsing activity even though there were no warnings so far
311315
// regex looks for something like 'identifier = blabla ,'
312-
if (!parserResult.hasWarnings() && Pattern.compile("\\w+\\s*=.*,").matcher(database.getEpilog()).find()) {
313-
parserResult.addWarning("following BibTex fragment has not been parsed:\n" + database.getEpilog());
316+
if (!parserResult.hasWarnings() && epilogPattern.matcher(database.getEpilog()).find()) {
317+
parserResult.addWarning(new ParserResult.Range(line, column, line, column), "following BibTeX fragment has not been parsed:\n" + database.getEpilog());
314318
}
315319
}
316320

@@ -319,6 +323,8 @@ private void parseRemainingContent() {
319323
}
320324

321325
private void parseAndAddEntry(String type) {
326+
int startLine = line;
327+
int startColumn = column;
322328
try {
323329
// collect all comments and the entry type definition in front of the actual entry
324330
// this is at least `@Type`
@@ -347,13 +353,16 @@ private void parseAndAddEntry(String type) {
347353
// This makes the parser more robust:
348354
// If an exception is thrown when parsing an entry, drop the entry and try to resume parsing.
349355
LOGGER.warn("Could not parse entry", ex);
350-
parserResult.addWarning(Localization.lang("Error occurred when parsing entry") + ": '" + ex.getMessage()
351-
+ "'. " + "\n\n" + Localization.lang("JabRef skipped the entry."));
356+
String errorMessage = Localization.lang("Error occurred when parsing entry") + ": '" + ex.getMessage()
357+
+ "'. " + "\n\n" + Localization.lang("JabRef skipped the entry.");
358+
parserResult.addWarning(new ParserResult.Range(startLine, startColumn, line, column), errorMessage);
352359
}
353360
}
354361

355362
private void parseJabRefComment(Map<String, String> meta) {
356363
StringBuilder buffer;
364+
int startLine = line;
365+
int startColumn = column;
357366
try {
358367
buffer = parseBracketedFieldContent();
359368
} catch (IOException e) {
@@ -385,7 +394,7 @@ private void parseJabRefComment(Map<String, String> meta) {
385394
if (typ.isPresent()) {
386395
entryTypes.add(typ.get());
387396
} else {
388-
parserResult.addWarning(Localization.lang("Ill-formed entrytype comment in BIB file") + ": " + comment);
397+
parserResult.addWarning(new ParserResult.Range(startLine, startColumn, line, column), Localization.lang("Ill-formed entrytype comment in BIB file") + ": " + comment);
389398
}
390399

391400
// custom entry types are always re-written by JabRef and not stored in the file
@@ -394,7 +403,7 @@ private void parseJabRefComment(Map<String, String> meta) {
394403
try {
395404
parseBibDeskComment(comment, meta);
396405
} catch (ParseException ex) {
397-
parserResult.addException(ex);
406+
parserResult.addException(new ParserResult.Range(startLine, startColumn, line, column), ex);
398407
}
399408
}
400409
}
@@ -461,11 +470,13 @@ private void parseBibDeskComment(String comment, Map<String, String> meta) throw
461470
}
462471

463472
private void parseBibtexString() throws IOException {
473+
int startLine = line;
474+
int startColumn = column;
464475
BibtexString bibtexString = parseString();
465476
try {
466477
database.addString(bibtexString);
467478
} catch (KeyCollisionException ex) {
468-
parserResult.addWarning(Localization.lang("Duplicate string name: '%0'", bibtexString.getName()));
479+
parserResult.addWarning(new ParserResult.Range(startLine, startColumn, line, column), Localization.lang("Duplicate string name: '%0'", bibtexString.getName()));
469480
}
470481
}
471482

@@ -920,19 +931,19 @@ private String fixKey() throws IOException {
920931

921932
// Finished, now reverse newKey and remove whitespaces:
922933
key = newKey.reverse();
923-
parserResult.addWarning(
934+
parserResult.addWarning(new ParserResult.Range(line, column),
924935
Localization.lang("Line %0: Found corrupted citation key %1.", String.valueOf(line), key.toString()));
925936
}
926937
}
927938
break;
928939

929940
case ',':
930-
parserResult.addWarning(
941+
parserResult.addWarning(new ParserResult.Range(line, column),
931942
Localization.lang("Line %0: Found corrupted citation key %1 (contains whitespaces).", String.valueOf(line), key.toString()));
932943
break;
933944

934945
case '\n':
935-
parserResult.addWarning(
946+
parserResult.addWarning(new ParserResult.Range(line, column),
936947
Localization.lang("Line %0: Found corrupted citation key %1 (comma missing).", String.valueOf(line), key.toString()));
937948
break;
938949

‎jablib/src/main/java/org/jabref/logic/integrity/IntegrityCheckResultErrorFormatWriter.java‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,8 @@
44
import java.io.Writer;
55
import java.nio.file.Path;
66
import java.util.List;
7-
import java.util.Map;
87

98
import org.jabref.logic.importer.ParserResult;
10-
import org.jabref.model.entry.field.Field;
11-
import org.jabref.model.entry.field.InternalField;
129

1310
public class IntegrityCheckResultErrorFormatWriter extends IntegrityCheckResultWriter {
1411

@@ -24,9 +21,7 @@ public IntegrityCheckResultErrorFormatWriter(Writer writer, List<IntegrityMessag
2421
@Override
2522
public void writeFindings() throws IOException {
2623
for (IntegrityMessage message : messages) {
27-
Map<Field, ParserResult.Range> fieldRangeMap = parserResult.getFieldRanges().getOrDefault(message.entry(), Map.of());
28-
ParserResult.Range fieldRange = fieldRangeMap.getOrDefault(message.field(), fieldRangeMap.getOrDefault(InternalField.KEY_FIELD, parserResult.getArticleRanges().getOrDefault(message.entry(), ParserResult.Range.NULL_RANGE)));
29-
24+
ParserResult.Range fieldRange = parserResult.getFieldRange(message.entry(), message.field());
3025
writer.append("%s:%d:%d: %s\n".formatted(
3126
inputFile,
3227
fieldRange.startLine(),

‎jablib/src/main/java/org/jabref/model/entry/field/Field.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import java.util.EnumSet;
44
import java.util.Optional;
55

6+
import org.jabref.model.entry.EntryConverter;
67
import org.jabref.model.strings.StringUtil;
78

89
public interface Field {
@@ -36,7 +37,7 @@ default boolean isDeprecated() {
3637
}
3738

3839
default Optional<Field> getAlias() {
39-
return Optional.empty();
40+
return Optional.ofNullable(EntryConverter.FIELD_ALIASES.get(this));
4041
}
4142

4243
default boolean isNumeric() {

‎jabls/build.gradle.kts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ dependencies {
1414
implementation("org.eclipse.lsp4j:org.eclipse.lsp4j")
1515
implementation("org.eclipse.lsp4j:org.eclipse.lsp4j.websocket")
1616

17+
implementation("com.google.guava:guava")
18+
1719
// route all requests to java.util.logging to SLF4J (which in turn routes to tinylog)
1820
testImplementation("org.slf4j:jul-to-slf4j")
1921
}

‎jabls/src/main/java/module-info.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,12 @@
66

77
requires org.jabref.jablib;
88

9+
requires com.google.common;
10+
requires com.google.gson;
11+
912
requires org.slf4j;
1013

1114
requires org.eclipse.lsp4j;
1215
requires org.eclipse.lsp4j.jsonrpc;
1316
requires org.eclipse.lsp4j.websocket;
14-
requires com.google.gson;
1517
}

‎jabls/src/main/java/org/jabref/languageserver/util/LspConsistencyCheck.java‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@
88
import java.util.Set;
99
import java.util.stream.Collectors;
1010

11+
import org.jabref.logic.importer.ParserResult;
1112
import org.jabref.logic.l10n.Localization;
1213
import org.jabref.logic.quality.consistency.BibliographyConsistencyCheck;
13-
import org.jabref.model.database.BibDatabaseContext;
1414
import org.jabref.model.entry.BibEntryType;
1515
import org.jabref.model.entry.BibEntryTypesManager;
1616
import org.jabref.model.entry.field.BibField;
@@ -21,10 +21,10 @@
2121

2222
public class LspConsistencyCheck {
2323

24-
public List<Diagnostic> check(BibDatabaseContext bibDatabaseContext, String content) {
24+
public List<Diagnostic> check(ParserResult parserResult) {
2525
List<Diagnostic> diagnostics = new ArrayList<>();
2626
BibliographyConsistencyCheck consistencyCheck = new BibliographyConsistencyCheck();
27-
BibliographyConsistencyCheck.Result result = consistencyCheck.check(bibDatabaseContext, (_, _) -> {
27+
BibliographyConsistencyCheck.Result result = consistencyCheck.check(parserResult.getDatabaseContext(), (_, _) -> {
2828
});
2929

3030
List<Field> allReportedFields = result.entryTypeToResultMap().values().stream()
@@ -34,7 +34,7 @@ public List<Diagnostic> check(BibDatabaseContext bibDatabaseContext, String cont
3434
.toList();
3535

3636
result.entryTypeToResultMap().forEach((entryType, entryTypeResult) -> {
37-
Optional<BibEntryType> bibEntryType = new BibEntryTypesManager().enrich(entryType, bibDatabaseContext.getMode());
37+
Optional<BibEntryType> bibEntryType = new BibEntryTypesManager().enrich(entryType, parserResult.getDatabaseContext().getMode());
3838
Set<Field> requiredFields = bibEntryType
3939
.map(BibEntryType::getRequiredFields)
4040
.stream()
@@ -44,9 +44,8 @@ public List<Diagnostic> check(BibDatabaseContext bibDatabaseContext, String cont
4444

4545
entryTypeResult.sortedEntries().forEach(entry -> requiredFields.forEach(requiredField -> {
4646
if (entry.getFieldOrAlias(requiredField).isEmpty()) {
47-
LspDiagnosticBuilder diagnosticBuilder = LspDiagnosticBuilder.create(Localization.lang("Required field \"%0\" is empty.", requiredField.getName()));
47+
LspDiagnosticBuilder diagnosticBuilder = LspDiagnosticBuilder.create(parserResult, Localization.lang("Required field \"%0\" is empty.", requiredField.getName()));
4848
diagnosticBuilder.setSeverity(DiagnosticSeverity.Error);
49-
diagnosticBuilder.setContent(content);
5049
diagnosticBuilder.setEntry(entry);
5150
diagnostics.add(diagnosticBuilder.build());
5251
}
@@ -62,8 +61,7 @@ public List<Diagnostic> check(BibDatabaseContext bibDatabaseContext, String cont
6261

6362
optionalFields.forEach(optionalField -> entryTypeResult.sortedEntries().forEach(entry -> {
6463
if (entry.getFieldOrAlias(optionalField).isEmpty()) {
65-
LspDiagnosticBuilder diagnosticBuilder = LspDiagnosticBuilder.create(Localization.lang("Optional field \"%0\" is empty.", optionalField.getName()));
66-
diagnosticBuilder.setContent(content);
64+
LspDiagnosticBuilder diagnosticBuilder = LspDiagnosticBuilder.create(parserResult, Localization.lang("Optional field \"%0\" is empty.", optionalField.getName()));
6765
diagnosticBuilder.setEntry(entry);
6866
diagnostics.add(diagnosticBuilder.build());
6967
}

0 commit comments

Comments
 (0)