null safe start #36
null safe start #36
Conversation
- mini fixes - причесан лексер
- рефаторинг классов визитоеторов и лиснеров - рефакторинг парсера - доработка генератора
WalkthroughМассовый рефакторинг: добавлены Lombok-аннотации ( Changes
Sequence Diagram(s)(Пропущено — изменения в основном касаются массовой миграции аннотаций/аксессоров и не вводят новый последовательный поток взаимодействий между ≥3 компонентами.) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/main/java/org/antlr/v4/runtime/TokenSource.java (2)
67-72:⚠️ Potential issue | 🟡 MinorИнтерфейс TokenSource должен быть помечен аннотацией
@NullMarked.JavaDoc для
getSourceName()утверждает "non-null, non-empty string", но интерфейс не помечен@NullMarked. Без этой аннотации неаннотированные возвращаемые типы рассматриваются как неявно nullable согласно спецификации jspecify. Аналогичные интерфейсы в том же пакете (TokenStream,ParserErrorListener) уже помечены@NullMarked. Реализации этого интерфейса, такие какLexer, также помечены@NullMarked.Добавьте
@NullMarkedна уровне интерфейса для обеспечения консистентности аннотаций nullability и соответствия JavaDoc.
74-80:⚠️ Potential issue | 🔴 CriticalОбработка null в setTokenFactory требует проверки.
Удаление аннотации
@NotNullс параметраfactoryв интерфейсеTokenSourceпозволяет передаватьnull. Однако реализации (ListTokenSourceиLexer) инициализируютtokenFactoryполем по умолчанию (CommonTokenFactory.DEFAULT) и используют его напрямую без проверок на null. Это приведёт кNullPointerExceptionв runtime при вызове методов вроде_factory.create(), если будет переданnull. Убедитесь, что либо (1) все реализации добавляют явную проверкуif (factory == null) throw new NullPointerException(...), либо (2)@NotNullостаётся на параметре, либо (3) документируется, что null передавать нельзя.src/main/java/org/antlr/v4/runtime/tree/pattern/RuleTagToken.java (1)
62-70:⚠️ Potential issue | 🟡 MinorНесоответствие между документацией и реализацией проверки
ruleName.Javadoc указывает
@throws IllegalArgumentException if ruleName is null or empty, но код на строке 63 выполняет толькоruleName.isEmpty(). Если переданnull, будет выброшеноNullPointerException, а неIllegalArgumentExceptionкак задокументировано.🛡️ Предлагаемое исправление для соответствия документации
public RuleTagToken(String ruleName, int bypassTokenType, `@Nullable` String label) { - if (ruleName.isEmpty()) { + if (ruleName == null || ruleName.isEmpty()) { throw new IllegalArgumentException("ruleName cannot be null or empty."); }src/main/java/org/antlr/v4/tool/Alternative.java (1)
84-87:⚠️ Potential issue | 🟡 MinorОтсутствует аннотация
@Nullableдля согласованности с интерфейсом.Метод
resolveToAttribute(String x, ActionAST node)переопределяет метод интерфейсаAttributeResolver, который теперь объявлен с@Nullable Attribute. Для согласованности и корректной работы статического анализа следует добавить аннотацию.🔧 Предлагаемое исправление
`@Override` + `@Nullable` public Attribute resolveToAttribute(String x, ActionAST node) { return rule.resolveToAttribute(x, node); // reuse that code }src/main/java/org/antlr/v4/runtime/tree/pattern/ParseTreeMatch.java (1)
63-77:⚠️ Potential issue | 🟡 MinorJavadoc упоминает
IllegalArgumentException, но валидация отсутствует.Документация указывает, что конструктор выбрасывает
IllegalArgumentExceptionдля null-значенийtree,patternиlabels, однако код валидации отсутствует.🛡️ Предлагаемое исправление — добавить валидацию или обновить Javadoc
Вариант 1 — добавить валидацию:
public ParseTreeMatch(ParseTree tree, ParseTreePattern pattern, MultiMap<String, ParseTree> labels, `@Nullable` ParseTree mismatchedNode) { + if (tree == null) { + throw new IllegalArgumentException("tree cannot be null"); + } + if (pattern == null) { + throw new IllegalArgumentException("pattern cannot be null"); + } + if (labels == null) { + throw new IllegalArgumentException("labels cannot be null"); + } this.tree = tree;Вариант 2 — удалить неактуальный Javadoc о выбросе исключений.
src/main/java/org/antlr/v4/runtime/BufferedTokenStream.java (1)
196-199:⚠️ Potential issue | 🟠 MajorПотенциальный NPE:
LA(int)вызываетgetType()на nullableToken.Метод
LT(int k)аннотирован как@Nullableи возвращаетnullприk == 0. ОднакоLA(int i)вызываетLT(i).getType()без null-проверки, что приведёт к NPE приi == 0.🐛 Предлагаемое исправление
`@Override` public int LA(int i) { - return LT(i).getType(); + Token token = LT(i); + return token != null ? token.getType() : Token.INVALID_TYPE; }Альтернативно, если
LA(0)не должен вызываться, добавьте проверку с исключением.src/main/java/org/antlr/v4/runtime/Parser.java (1)
342-352:⚠️ Potential issue | 🟠 MajorНе добавляйте null‑listener в список.
Сейчас
addParseListenerпринимаетnullи кладёт его в_parseListeners, что приведёт к NPE вtriggerEnterRuleEvent/triggerExitRuleEventпри обходе слушателей. Это также расходится с Javadoc на Line 344, где обещан NPE сразу. Предлагаю явно валидировать вход.💡 Возможное исправление
public void addParseListener(ParseTreeListener listener) { + if (listener == null) { + throw new NullPointerException("listener"); + } if (_parseListeners == null) { _parseListeners = new ArrayList<>(); } this._parseListeners.add(listener); }
🤖 Fix all issues with AI agents
In `@src/main/java/org/antlr/v4/runtime/DefaultErrorStrategy.java`:
- Around line 557-569: getTokenErrorDisplay currently calls
escapeWSAndQuote(getSymbolText(t)) but getSymbolText(Token) can return null
(e.g., EOF/synthetic tokens), causing NPE; update the logic to handle null token
text by checking the result of getSymbolText (or making getSymbolText never
return null) and using a fallback string derived from the token type (via
getSymbolType) such as a readable "<EOF>"/"<tokenType:NN>" or other descriptive
placeholder before calling escapeWSAndQuote; reference getTokenErrorDisplay,
getSymbolText, getSymbolType and escapeWSAndQuote when implementing the guard
and fallback.
In `@src/main/java/org/antlr/v4/runtime/Lexer.java`:
- Around line 223-229: В методе setInputStream обнаружена лишняя генерация
tokenFactorySourcePair с null: не создавайте Tuple.create(this,
this.inputStream) сразу после обнуления inputStream; вместо этого либо удалите
промежуточную строку и просто выполните this.inputStream = inputStream;
this.tokenFactorySourcePair = Tuple.create(this, this.inputStream); reset();
либо поменяйте порядок так, чтобы присвоение this.inputStream = inputStream
происходило до вызова Tuple.create; отредактируйте метод setInputStream,
затрагивая именно setInputStream, tokenFactorySourcePair, Tuple.create и
reset(), чтобы пара никогда не создавалась с null-потоком.
- Around line 240-243: Метод emit() был изменён неверно — нужно вернуть
совместимый с ANTLR API сигнатуру public Token emit(): в методе (в котором
сейчас вызывается tokenFactory.create(tokenFactorySourcePair, type, text,
channel, tokenStartCharIndex, getCharIndex() - 1, tokenStartLine,
tokenStartCharPositionInLine)) оставить присваивание поля token и в конце
вернуть этот token; восстановите видимость public и тип возвращаемого значения
Token для метода emit(), чтобы подклассы, переопределяющие emit(), продолжали
получать и возвращать созданный токен.
In `@src/main/java/org/antlr/v4/runtime/tree/AbstractParseTreeVisitor.java`:
- Around line 94-97: Метод defaultResult() в классе AbstractParseTreeVisitor был
сделан private, что ломает API для подклассов; верните ему видимость protected
(т.е. изменить модификатор доступа обратно на protected для метода
defaultResult()) чтобы подклассы могли переопределять начальное значение
агрегации, оставив сигнатуру метода и аннотацию `@Nullable` без изменений.
In `@src/main/java/org/antlr/v4/runtime/tree/ParseTree.java`:
- Line 37: The `@Nullable` annotation is currently placed before the type
parameter in the method declaration of ParseTree.accept, which makes it a method
annotation instead of annotating the return type; update the signature of accept
in interface ParseTree from "@Nullable <T> T accept(ParseTreeVisitor<? extends
T> visitor)" to place `@Nullable` after the type parameter so it reads "<T>
`@Nullable` T accept(ParseTreeVisitor<? extends T> visitor)" ensuring TYPE_USE
semantics for the return type.
In `@src/main/java/org/antlr/v4/Tool.java`:
- Around line 964-966: haveOutputDir() always returns true because handleArgs()
defaults outputDirectory to "."; fix by distinguishing "was -o provided" from
default value: add a new boolean field (e.g., outputDirectorySpecified) set to
true only when handleArgs() processes an explicit -o, update haveOutputDir() to
return outputDirectorySpecified (or return outputDirectorySpecified ||
outputDirectory != null if you keep both semantics), and use that updated
predicate in getOutputDirectory() and any other places that rely on knowing
whether the user explicitly supplied -o; refer to methods haveOutputDir(),
handleArgs(), getOutputDirectory(), and the outputDirectory field when making
the change.
In `@src/main/java/org/antlr/v4/tool/ast/ActionAST.java`:
- Around line 21-23: Поле scope в классе ActionAST помечено без модификатора
доступа, и Lombok сгенерирует package-private getScope()/setScope(), что ломает
доступ из других пакетов; исправьте это либо явно сделав аксессоры публичными
через Lombok (например, добавить `@Getter`(AccessLevel.PUBLIC) и
`@Setter`(AccessLevel.PUBLIC) над полем/классом), либо объявив само поле public
(public GrammarAST scope = null;), чтобы гарантировать доступность методов из
пакетов org.antlr.v4.codegen.model и org.antlr.v4.semantics; измените
соответствующую строку с поле/аннотациями в ActionAST.
In `@src/main/java/org/antlr/v4/tool/ast/GrammarRootAST.java`:
- Around line 74-81: getGrammarName() is `@Nullable` so callers must guard against
null to avoid NPEs; update the usages in Tool.sortGrammarByTokenVocab() and the
import routine where root.getGrammarName() is used. In
sortGrammarByTokenVocab(), replace direct
root.getGrammarName().equals(grammarName) with a null-safe comparison (e.g.
Objects.equals(root.getGrammarName(), grammarName) or check
root.getGrammarName() != null before calling equals) or skip/continue when the
grammar name is null. In the import code before calling
importedGrammars.put(root.getGrammarName(), imported) check for null and handle
it (e.g. skip storing, use a fallback key, or log and continue) so no null key
is inserted and no NPE occurs.
In `@src/main/java/org/antlr/v4/tool/Grammar.java`:
- Around line 684-695: getImportedGrammar currently iterates importedGrammars
(declared `@Nullable`) without checking for null, causing an NPE before
loadImportedGrammars initializes it; update getImportedGrammar(String name) to
first check if importedGrammars is null and return null (or empty result) if so,
then iterate the list, referencing the importedGrammars field and the
getImportedGrammar method to locate the change; ensure you do not alter the
method signature and keep behavior of returning null when no matching Grammar is
found.
🟡 Minor comments (12)
src/test/java/org/antlr/v4/test/runtime/java/api/TestIncrementalJavaLexer.java-176-194 (1)
176-194:⚠️ Potential issue | 🟡 MinorНесоответствие между сгенерированным лексером и грамматикой.
В
JavaLetter_sempredиспользуетсяgetInputStream().LA(...), однако в исходной грамматикеTestIncrementalJava.g4фрагментJavaLetter(строки 1215 и 1218) всё ещё использует_input.LA(...). Это может привести к расхождению при повторной генерации лексера.Рекомендуется привести грамматику
TestIncrementalJava.g4в соответствие с этим сгенерированным файлом, обновив фрагментJavaLetterдля использованияgetInputStream().src/test/resources/TestIncrementalJava.g4-1226-1229 (1)
1226-1229:⚠️ Potential issue | 🟡 MinorНесогласованность с фрагментом JavaLetter.
В
JavaLetterOrDigitиспользуетсяgetInputStream().LA(...), но вJavaLetter(строки 1215 и 1218) по-прежнему используется_input.LA(...). Для единообразия и корректной работы null-safe логики необходимо обновить оба фрагмента.🐛 Предлагаемое исправление для JavaLetter
fragment JavaLetter : [a-zA-Z$_] // these are the "java letters" below 0xFF | // covers all characters above 0xFF which are not a surrogate ~[\u0000-\u00FF\uD800-\uDBFF] - {Character.isJavaIdentifierStart(_input.LA(-1))}? + {Character.isJavaIdentifierStart(getInputStream().LA(-1))}? | // covers UTF-16 surrogate pairs encodings for U+10000 to U+10FFFF [\uD800-\uDBFF] [\uDC00-\uDFFF] - {Character.isJavaIdentifierStart(Character.toCodePoint((char)_input.LA(-2), (char)_input.LA(-1)))}? + {Character.isJavaIdentifierStart(Character.toCodePoint((char)getInputStream().LA(-2), (char)getInputStream().LA(-1)))}? ;src/main/java/org/antlr/v4/runtime/tree/pattern/TextChunk.java-12-33 (1)
12-33:⚠️ Potential issue | 🟡 MinorКонтракт конструктора и реализация расходятся.
Javadoc обещает IllegalArgumentException при null, но проверки больше нет — null проходит и попадает в toString. Либо восстановите проверку, либо обновите контракт.🛠️ Возможное исправление
public TextChunk(String text) { + if (text == null) { + throw new IllegalArgumentException("text cannot be null"); + } this.text = text; }src/main/java/org/antlr/v4/runtime/RecognitionException.java-49-62 (1)
49-62:⚠️ Potential issue | 🟡 MinorНекорректный формат JavaDoc.
Комментарий содержит обрывочные фрагменты:
"For","and","exceptions, this is the"— выглядит как повреждённая документация. Вероятно, при конвертации в Lombok-стиль часть текста была потеряна.📝 Предлагаемое исправление
/** - * -- GETTER -- - * Get the ATN state number the parser was in at the time the error - * occurred. For - * and - * - * exceptions, this is the - * - * number. For others, it is the state whose outgoing - * edge we couldn't match. - * <p>If the state number is not known, this method returns -1.</p> + * The ATN state number the parser was in at the time the error occurred. + * For {`@link` NoViableAltException} and {`@link` InputMismatchException}, + * this is the {`@link` DecisionState} number. For others, it is the state + * whose outgoing edge we couldn't match. + * <p>If the state number is not known, this method returns -1.</p> */ `@Getter` private int offendingState = -1;src/main/java/org/antlr/v4/runtime/dfa/AcceptStateInfo.java-12-33 (1)
12-33:⚠️ Potential issue | 🟡 MinorИсправьте фразу в Javadoc для
lexerActionExecutor.
Line 31: пропущено существительное в описании.✏️ Правка формулировки
- /** - * Gets the which can be used to execute actions and/or commands after the lexer matches a token. - */ + /** + * Gets the lexer action executor which can be used to execute actions and/or commands after the lexer matches a token. + */src/main/java/org/antlr/v4/runtime/atn/ATNConfigSet.java-71-75 (1)
71-75:⚠️ Potential issue | 🟡 MinorПометьте
conflictInfoкак nullable, иначе контракт геттера вводит в заблуждение.Поле сбрасывается в
null(см. Line 392-394), поэтому публичный доступ должен отражать nullable-семантику.💡 Предлагаемая правка
- private ConflictInfo conflictInfo; + private `@Nullable` ConflictInfo conflictInfo;src/main/java/org/antlr/v4/runtime/Recognizer.java-63-65 (1)
63-65:⚠️ Potential issue | 🟡 MinorПотенциальный NPE при неинициализированном
interpreter.Метод
getATN()обращается кinterpreter.atnнапрямую. Еслиinterpreterне был установлен (равен null), произойдёт NullPointerException. Рекомендуется добавить проверку или документировать предусловие.🛡️ Вариант с проверкой
public ATN getATN() { + if (interpreter == null) { + throw new IllegalStateException("Interpreter has not been set"); + } return interpreter.atn; }src/main/java/org/antlr/v4/runtime/Token.java-110-162 (1)
110-162:⚠️ Potential issue | 🟡 MinorПроверьте соответствие значений
Token.EMPTYконтракту интерфейса.Реализация
Token.EMPTY:
getTokenIndex()возвращает0, но документация (строка 86-87) указывает, что-1означает "токен создан искусственно".getLine()возвращает0, но документация (строка 67-68) указываетline=1..n.Рассмотрите возможность использования
-1дляgetTokenIndex()иgetStartIndex()/getStopIndex(), чтобы соответствовать описанному контракту для невалидных токенов.Предлагаемое исправление
`@Override` public int getTokenIndex() { - return 0; + return -1; } `@Override` public int getStartIndex() { - return 0; + return -1; } `@Override` public int getStopIndex() { - return 0; + return -1; }src/main/java/org/antlr/v4/tool/AttributeDict.java-31-34 (1)
31-34:⚠️ Potential issue | 🟡 MinorПотенциальная несогласованность: поле
nameможет бытьnullв@NullMarkedклассе.Поле
nameне инициализировано и не аннотировано как@Nullable, но класс помечен@NullMarked. При вызовеgetName()до установки значения будет возвращёнnull, что нарушает контракт@NullMarked. Рекомендуется либо добавить@Nullable, либо инициализировать поле.🔧 Вариант 1: Добавить `@Nullable`
`@NullMarked` public class AttributeDict { `@Getter` + `@Nullable` public String name;🔧 Вариант 2: Инициализировать поле
`@NullMarked` public class AttributeDict { `@Getter` - public String name; + public String name = "";src/main/java/org/antlr/v4/runtime/tree/pattern/TagChunk.java-59-62 (1)
59-62:⚠️ Potential issue | 🟡 MinorНесоответствие между сообщением об ошибке и валидацией.
Сообщение об ошибке указывает "tag cannot be null or empty", но проверка на
nullотсутствует —tag.isEmpty()вызоветNullPointerExceptionпри передачеnull. Если класс подразумевается как@NullMarked, параметрtagнеявно non-null, и сообщение следует изменить на "tag cannot be empty".🔧 Предложение
public TagChunk(`@Nullable` String label, String tag) { if (tag.isEmpty()) { - throw new IllegalArgumentException("tag cannot be null or empty"); + throw new IllegalArgumentException("tag cannot be empty"); } this.label = label; this.tag = tag; }src/main/java/org/antlr/v4/tool/ANTLRMessage.java-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorПоле
gне инициализировано и не аннотировано@Nullable.В контексте
@NullMarkedкласса, полеpublic Grammar gбез аннотации@Nullableподразумевает non-null контракт. Однако поле не инициализируется в конструкторах, что приведёт к NPE при обращении или нарушит контракт null-безопасности.🛡️ Предлагаемое исправление
- public Grammar g; + public `@Nullable` Grammar g;src/main/java/org/antlr/v4/runtime/Parser.java-960-965 (1)
960-965:⚠️ Potential issue | 🟡 MinorisTrace может вернуть true при отсутствии слушателя.
Если кто-то вызовет
removeParseListener(tracer)илиremoveParseListeners(), полеtracerостанется ненулевым, иisTrace()на Line 965 вернётtrue, хотя слушателя уже нет. Лучше проверять фактическое наличие в списке.💡 Возможное исправление
public boolean isTrace() { - return tracer != null; + return tracer != null && _parseListeners != null && _parseListeners.contains(tracer); }
🧹 Nitpick comments (27)
src/main/java/org/antlr/v4/runtime/tree/xpath/XPathLexerErrorListener.java (1)
19-26: Аннотации@Nullableсоответствуют контракту интерфейса.Добавление
@Nullableк параметрамoffendingSymbolиRecognitionException eкорректно отражает, что эти значения могут быть null при обработке синтаксических ошибок. Это изменение согласуется с общим рефакторингом null-safety по всему проекту.Незначительное замечание: пустая строка внутри пустого тела метода (строка 25) выглядит избыточной.
,
♻️ Опциональное исправление форматирования
public <T extends Integer> void syntaxError(Recognizer<T, ?> recognizer, `@Nullable` T offendingSymbol, int line, int charPositionInLine, String msg, `@Nullable` RecognitionException e) { - }src/test/resources/PositionAdjustingLexer.g4 (1)
64-68: Рассмотрите упрощение метода.Промежуточная переменная
interpreterне добавляет ценности — приведение типа можно выполнить в одну строку.♻️ Предлагаемое упрощение
`@Override` public PositionAdjustingLexerATNSimulator getInterpreter() { - var interpreter = super.getInterpreter(); - return (PositionAdjustingLexerATNSimulator) interpreter; + return (PositionAdjustingLexerATNSimulator) super.getInterpreter(); }src/test/resources/JavaLR.g4 (1)
1203-1206: Несогласованность с фрагментом JavaLetter.В
JavaLetterOrDigitиспользуетсяgetInputStream().LA(...), но вJavaLetter(строки 1192 и 1195) по-прежнему используется_input.LA(...). Для единообразия и последовательного применения null-safe подхода рекомендуется обновить оба фрагмента.♻️ Предлагаемое исправление для JavaLetter
fragment JavaLetter : [a-zA-Z$_] // these are the "java letters" below 0xFF | // covers all characters above 0xFF which are not a surrogate ~[\u0000-\u00FF\uD800-\uDBFF] - {Character.isJavaIdentifierStart(_input.LA(-1))}? + {Character.isJavaIdentifierStart(getInputStream().LA(-1))}? | // covers UTF-16 surrogate pairs encodings for U+10000 to U+10FFFF [\uD800-\uDBFF] [\uDC00-\uDFFF] - {Character.isJavaIdentifierStart(Character.toCodePoint((char)_input.LA(-2), (char)_input.LA(-1)))}? + {Character.isJavaIdentifierStart(Character.toCodePoint((char)getInputStream().LA(-2), (char)getInputStream().LA(-1)))}? ;src/main/java/org/antlr/v4/tool/ast/GrammarASTWithOptions.java (2)
14-16: Смешение фреймворков аннотаций nullability.В файле используются аннотации из разных источников:
org.antlr.v4.runtime.misc.NotNull(строка 14, используется вgetOptions())org.jspecify.annotations.Nullable(строка 16, используется вgetOptionString())Рекомендуется унифицировать использование аннотаций, перейдя полностью на jspecify для согласованности с остальными изменениями в PR.
74-77: Рекомендуется добавить@Nullableк getOptionAST.Метод
getOptionASTтакже может возвращатьnull(строки 75-76), но не аннотирован. Для согласованности и улучшения статического анализа рекомендуется добавить аннотацию.♻️ Предлагаемое изменение
+ `@Nullable` public GrammarAST getOptionAST(String key) { if (options == null) return null; return options.get(key); }src/test/java/org/antlr/v4/test/runtime/java/api/TestIncremental.java (1)
131-134: Рекомендуется добавить проверку на null для getChildren().Переход на использование
getChildren()вместо прямого доступа к полю корректен. Однако, учитывая направленность PR на улучшение обработки null-значений, стоит убедиться, чтоgetChildren()не возвращает null, или добавить защитную проверку.♻️ Предлагаемое улучшение
- for (var child : secondTree.getChildren()) { + var children = secondTree.getChildren(); + assertThat(children).isNotNull(); + for (var child : children) { var incChild = (IncrementalParserRuleContext) child; assertEquals(startingEpoch, incChild.epoch); }src/main/java/org/antlr/v4/runtime/tree/TerminalNodeImpl.java (1)
19-29: Добавьте явную проверкуsymbolна null при создании.Теперь
getSourceInterval(),getText()иtoString()используютsymbolбез защитных проверок, поэтому лучше валидировать входные данные в конструкторе и получить понятный fail-fast.♻️ Предлагаемая правка
+import java.util.Objects; @@ public TerminalNodeImpl(Token symbol) { - this.symbol = symbol; + this.symbol = Objects.requireNonNull(symbol, "symbol"); }src/main/java/org/antlr/v4/runtime/ConsoleErrorListener.java (1)
37-45: Отсутствует аннотация@Nullableна параметреoffendingSymbol.В
BaseErrorListenerиProxyErrorListenerпараметрoffendingSymbolаннотирован как@Nullable T offendingSymbol, однако здесь эта аннотация отсутствует. Для согласованности API рекомендуется добавить аннотацию.♻️ Предлагаемое исправление
`@Override` public <T> void syntaxError(Recognizer<T, ?> recognizer, - T offendingSymbol, + `@Nullable` T offendingSymbol, int line, int charPositionInLine, String msg, `@Nullable` RecognitionException e) {src/test/java/org/antlr/v4/test/tool/PerformanceTest.java (2)
1669-1671: Избыточное использование полного имени аннотации.Аннотация
@org.jspecify.annotations.Nullableиспользуется с полным путём, хотя импортorg.jspecify.annotations.Nullableуже добавлен в строке 53. Для согласованности рекомендуется использовать короткую форму@Nullable.♻️ Предлагаемое исправление
`@Override` public <T extends Token> void syntaxError(Recognizer<T, ?> recognizer, T offendingSymbol, int line, - int charPositionInLine, String msg, `@org.jspecify.annotations.Nullable` RecognitionException e) { + int charPositionInLine, String msg, `@Nullable` RecognitionException e) {
1690-1692: Аналогичная избыточность вDescriptiveLexerErrorListener.Та же проблема с использованием полного пути для аннотации
@Nullable.♻️ Предлагаемое исправление
`@Override` public <T extends Integer> void syntaxError(Recognizer<T, ?> recognizer, T offendingSymbol, int line, - int charPositionInLine, String msg, `@org.jspecify.annotations.Nullable` RecognitionException e) { + int charPositionInLine, String msg, `@Nullable` RecognitionException e) {src/main/java/org/antlr/v4/gui/TreeViewer.java (1)
150-155: Рекомендуется добавить@Getterвместо явного метода.Поле
useCurvedEdgesимеет@Setter, но явный методgetUseCurvedEdges(). Для единообразия можно заменить на@Getter, если нет особой логики в геттере.♻️ Предлагаемое исправление
+ `@Getter` `@Setter` private boolean useCurvedEdges = false; - public boolean getUseCurvedEdges() { - return useCurvedEdges; - }src/main/java/org/antlr/v4/runtime/Lexer.java (1)
330-333: Устаревший метод с комментарием на русском языке.Метод
validateInputStreamпомечен как@Deprecated, но сообщениеsince = "Не используется"на русском языке. Для согласованности с англоязычной кодовой базой рекомендуется использовать английский.♻️ Предлагаемое исправление
- `@Deprecated`(since = "Не используется") + `@Deprecated`(since = "Not used") protected void validateInputStream(ATN atn, CharStream inputStream) { - // метод оставлен для совместимости + // method kept for compatibility }src/main/java/org/antlr/v4/runtime/Recognizer.java (1)
42-48: Рассмотрите добавление валидации в Lombok setter дляinterpreter.Поле
interpreterинициализируется с значениемnullи имеет Lombok-генерируемый сеттер без проверок. МетодgetATN()(строка 65) напрямую обращается кinterpreter.atnбез валидации. Хотя класс аннотирован@NullMarked, что обеспечивает контроль на уровне статического анализатора, Lombok@Setterне применяет constraints на уровне runtime. ЕслиsetInterpreter()вызовется сnull(или если interpreter никогда не будет установлен),getATN()выбросит NPE.Рекомендуется либо использовать custom setter с проверкой
Objects.requireNonNull(), либо явно инициализировать поле при создании объекта.src/main/java/org/antlr/v4/codegen/OutputModelController.java (1)
86-101: Несогласованность: публичные поля с аннотациями Lombok.Поля
codeBlockLevel,root,currentOuterMostAlt,currentBlockиcurrentOuterMostAlternativeBlockобъявлены какpublic, но к ним добавлены аннотации@Getter/@Setter. Lombok сгенерирует публичные аксессоры, но прямой доступ к полям также останется возможным.Рекомендуется либо изменить видимость полей на
private(как в других классах проекта, напримерTextChunk,TokenTagToken), либо удалить аннотации Lombok, если прямой доступ к полям является намеренным.Предлагаемое исправление - сделать поля приватными
`@Getter` - public int codeBlockLevel = -1; + private int codeBlockLevel = -1; public int treeLevel = -1; `@Setter` `@Getter` - public OutputModelObject root; // normally ParserFile, LexerFile, ... + private OutputModelObject root; // normally ParserFile, LexerFile, ... public Stack<RuleFunction> currentRule = new Stack<>(); `@Setter` `@Getter` - public Alternative currentOuterMostAlt; + private Alternative currentOuterMostAlt; `@Getter` `@Setter` - public CodeBlock currentBlock; + private CodeBlock currentBlock; `@Getter` `@Setter` - public CodeBlockForOuterMostAlt currentOuterMostAlternativeBlock; + private CodeBlockForOuterMostAlt currentOuterMostAlternativeBlock;src/main/java/org/antlr/v4/tool/GrammarInterpreterRuleContext.java (1)
24-28: Рекомендуется использовать аннотации на уровне поля вместо класса.
@Getterи@Setterна уровне класса генерируют аксессоры для всех полей, включая унаследованные. Для более точного контроля и предотвращения возможных конфликтов с родительским классомInterpreterRuleContextрекомендуется аннотировать конкретное поле:♻️ Предлагаемый рефакторинг
-@Setter -@Getter public class GrammarInterpreterRuleContext extends InterpreterRuleContext { - + `@Getter` + `@Setter` protected int outerAltNum = 1;src/main/java/org/antlr/v4/runtime/ProxyErrorListener.java (1)
23-29:@NullMarkedкорректно применён, но отсутствует защита отnullв конструкторе.Согласно AI-summary, была удалена проверка
nullдля параметраdelegates. При@NullMarkedпараметр неявно non-null, но отсутствие runtime-проверки означает, что ошибка проявится только при вызовеsyntaxError(строка 42), а не в момент создания объекта.🛡️ Опциональная защитная проверка
public ProxyErrorListener(Collection<? extends ANTLRErrorListener<? super Symbol>> delegates) { + if (delegates == null) { + throw new NullPointerException("delegates cannot be null"); + } this.delegates = delegates; }Альтернативно можно использовать
Objects.requireNonNull(delegates).src/main/java/org/antlr/v4/tool/Attribute.java (1)
50-50: Рассмотрите добавление@Nullableдля поляdict.Поле
dictинициализируется вAttributeDict.add()после создания объектаAttribute. До вызоваadd()поле будетnull, что может привести к NPE при доступе к нему.♻️ Предложение
/** * Who contains us? */ - public AttributeDict dict; + public `@Nullable` AttributeDict dict;src/main/java/org/antlr/v4/runtime/tree/pattern/TagChunk.java (1)
28-29: Отсутствует аннотация@NullMarkedна уровне класса.В отличие от других классов в этом PR (
TokenTagToken,TextChunk), классTagChunkне помечен@NullMarked. Для согласованности с общим подходом к null-safety в проекте рекомендуется добавить эту аннотацию.♻️ Предложение
+import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; /** * Represents a placeholder tag in a tree pattern... */ `@Getter` +@NullMarked class TagChunk extends Chunk {src/main/java/org/antlr/v4/runtime/tree/pattern/ParseTreePatternMatcher.java (1)
83-83: Отсутствует аннотация@NullMarkedна уровне класса.Для согласованности с другими классами в runtime-пакете (например,
BaseErrorListener,DiagnosticErrorListener,TokenStream) рекомендуется добавить@NullMarkedк классуParseTreePatternMatcher.♻️ Предложение
import org.jspecify.annotations.Nullable; +import org.jspecify.annotations.NullMarked; ... +@NullMarked public class ParseTreePatternMatcher {src/main/java/org/antlr/v4/runtime/DiagnosticErrorListener.java (1)
114-114: Неиспользуемый параметр типа<T extends Token>.Параметр типа
Tв методеgetDecisionDescriptionне используется в теле метода. Его можно удалить для повышения читаемости.♻️ Предложение
- protected <T extends Token> String getDecisionDescription(Parser recognizer, DFA dfa) { + protected String getDecisionDescription(Parser recognizer, DFA dfa) {src/main/java/org/antlr/v4/runtime/tree/pattern/ParseTreePattern.java (2)
24-25: Отсутствует аннотация@NullMarkedдля согласованности.Другие классы в этом PR помечены
@NullMarked. Для консистентности рекомендуется добавить аннотацию и к этому классу.♻️ Предлагаемое изменение
+import org.jspecify.annotations.NullMarked; + +@NullMarked `@Value` public class ParseTreePattern {
55-61: Явный конструктор избыточен при использовании@Value.Lombok
@Valueавтоматически генерирует all-args конструктор. Явное определение конструктора с теми же параметрами дублирует функциональность. Однако это может быть оставлено намеренно для документации Javadoc.src/main/java/org/antlr/v4/runtime/tree/pattern/ParseTreeMatch.java (1)
15-24: Отсутствует аннотация@NullMarkedна уровне класса.Импортированы
@NullMarkedи@Nullable, но класс не помечен@NullMarked. Для согласованности с другими классами в PR следует добавить аннотацию.♻️ Предлагаемое изменение
import java.util.List; +@NullMarked public class ParseTreeMatch {src/main/java/org/antlr/v4/tool/ast/GrammarRootAST.java (1)
47-72: Паттерн@Nullableпараметра с проверкой на null.Конструкторы принимают
@Nullable TokenStream, но сразу выбрасываютNullPointerException. Это необычный паттерн — обычно параметр не аннотируется@Nullable, если null недопустим.♻️ Рекомендуемое изменение — убрать `@Nullable` или использовать Objects.requireNonNull
- public GrammarRootAST(Token t, `@Nullable` TokenStream tokenStream) { + public GrammarRootAST(Token t, TokenStream tokenStream) { super(t); - if (tokenStream == null) { - throw new NullPointerException("tokenStream"); - } - - this.tokenStream = tokenStream; + this.tokenStream = Objects.requireNonNull(tokenStream, "tokenStream"); }src/main/java/org/antlr/v4/runtime/BufferedTokenStream.java (1)
70-72: Удалена проверкаtokenSourceна null в конструкторе.Согласно AI-summary, ранее была проверка на null. В контексте
@NullMarkedпараметр подразумевается non-null, но явная валидация защищала бы от ошибок вызывающего кода.🛡️ Предлагаемое изменение — добавить явную валидацию
public BufferedTokenStream(TokenSource tokenSource) { + if (tokenSource == null) { + throw new NullPointerException("tokenSource cannot be null"); + } this.tokenSource = tokenSource; }src/main/java/org/antlr/v4/runtime/ParserRuleContext.java (2)
97-115: Комментарий не соответствует поведению кода.Комментарий на строке 104-105 говорит "copy any error nodes", но цикл копирует все дочерние узлы (
addAnyChild(child)), а не только error nodes. Рекомендуется уточнить комментарий.📝 Предлагаемое изменение комментария
- // copy any error nodes to alt label node - // reset parent pointer for any error nodes + // copy all children to alt label node + // reset parent pointer for error nodes for (var child : ctx.children) {
159-161: Новый методsetChild(int i, TerminalNode t)— проверьте границы.Метод напрямую вызывает
children.set(i, t)без проверки границ. При невалидном индексе будет выброшенIndexOutOfBoundsException, что может быть неочевидно для вызывающего кода.🛡️ Предлагаемое изменение — добавить проверку или документацию
+ /** + * Replace child at index i with the given terminal node. + * + * `@throws` IndexOutOfBoundsException if i is out of range + */ public void setChild(int i, TerminalNode t) { children.set(i, t); }
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/antlr/v4/tool/Alternative.java (1)
96-115:⚠️ Potential issue | 🟠 MajorЗащититесь от
nullприgetRule(...), иначе возможен NPE в ошибочных грамматиках.Если ссылка на правило неразрешима,
getRule(...)может вернутьnull, и вызовresolveRetvalOrPropertyупадёт. Лучше вернутьnull, чтобы диагностика не ломалась.🐛 Возможная правка
if (ruleRefs.get(x) != null) { // rule ref in this alt? // look up rule, ask it to resolve y (must be retval or predefined) - return rule.g.getRule(x).resolveRetvalOrProperty(y); + Rule refRule = rule.g.getRule(x); + return refRule != null ? refRule.resolveRetvalOrProperty(y) : null; } LabelElementPair anyLabelDef = getAnyLabelDef(x); if (anyLabelDef != null && anyLabelDef.type == LabelType.RULE_LABEL) { - return rule.g.getRule(anyLabelDef.element.getText()).resolveRetvalOrProperty(y); + Rule refRule = rule.g.getRule(anyLabelDef.element.getText()); + return refRule != null ? refRule.resolveRetvalOrProperty(y) : null; } else if (anyLabelDef != null) {
🤖 Fix all issues with AI agents
In `@src/main/java/org/antlr/v4/parse/ScopeParser.java`:
- Line 57: The AttributeDict constructor currently forces DictType.ARG which
causes redundant setType(ARG) calls; change AttributeDict so its constructor
does not set a default type (or accept an explicit DictType parameter and only
set if non-null), then update callers: leave the necessary
setType(DictType.RET)/setType(DictType.LOCAL) in RuleCollector and ensure places
that need ARG explicitly call setType(DictType.ARG) (remove the redundant
setType(ARG) invocations in RuleCollector and LeftRecursiveRuleTransformer), and
adjust parseTypedArgList() creation sites to either set the type explicitly or
pass the desired type into the new constructor API.
In `@src/main/java/org/antlr/v4/runtime/LexerInterpreter.java`:
- Around line 22-34: The visibility of several fields in LexerInterpreter
(grammarFileName, atn, ruleNames, channelNames, modeNames, vocabulary) was
changed from protected to private which breaks source compatibility for
subclasses that accessed them directly; restore their original protected
visibility (keep the `@Getter` annotations and existing accessors intact) so
subclasses can continue to reference these fields while preserving encapsulation
for other code.
In `@src/main/java/org/antlr/v4/runtime/ListTokenSource.java`:
- Around line 223-225: Метод setTokenFactory в классе ListTokenSource позволяет
присвоить null в поле tokenFactory, что нарушает инвариант и приводит к NPE в
nextToken; исправьте setTokenFactory так, чтобы оно явно проверяло аргумент
(например с Objects.requireNonNull(factory, "tokenFactory") или явным if
(factory == null) throw new NullPointerException("tokenFactory");) и только
после проверки присваивало this.tokenFactory = factory, чтобы гарантировать, что
tokenFactory никогда не станет null.
In `@src/main/java/org/antlr/v4/runtime/tree/pattern/TagChunk.java`:
- Around line 59-62: The TagChunk constructor currently calls tag.isEmpty()
which will NPE if tag is null; add an explicit null check at the start of the
TagChunk(`@Nullable` String label, String tag) constructor and throw new
IllegalArgumentException("tag cannot be null or empty") (or separate messages)
when tag == null, ensuring the contract/Javadoc is respected before calling
tag.isEmpty().
In `@src/main/java/org/antlr/v4/tool/Alternative.java`:
- Around line 17-27: The jspecify dependency is currently declared as
compileOnly but its annotations (e.g., `@NullMarked` on public classes like
Alternative and `@Nullable` on public fields/methods used across Token,
RuleContext, Parser, Grammar, etc.) appear in the public API, so change the
Gradle configuration in build.gradle.kts to expose jspecify to consumers
(replace compileOnly with api or the equivalent) so downstream projects can
compile against these annotated public symbols.
🧹 Nitpick comments (7)
src/main/java/org/antlr/v4/runtime/tree/pattern/ParseTreeMatch.java (1)
23-36: Использование@Valueвыглядит корректно.Аннотация
@Valueот Lombok корректно применена для создания immutable value-класса. Поля без модификатора доступа будут обработаны Lombok какprivate final, что соответствует паттерну, используемому вParseTreePattern.java.Однако, рекомендуется добавить валидацию на null для параметров
tree,patternиlabelsв конструкторе, поскольку они не помечены@Nullable, но нет явной защиты от передачи null.♻️ Предложение по добавлению валидации null
public ParseTreeMatch(ParseTree tree, ParseTreePattern pattern, MultiMap<String, ParseTree> labels, `@Nullable` ParseTree mismatchedNode) { - - this.tree = tree; - this.pattern = pattern; - this.labels = labels; + this.tree = Objects.requireNonNull(tree, "tree cannot be null"); + this.pattern = Objects.requireNonNull(pattern, "pattern cannot be null"); + this.labels = Objects.requireNonNull(labels, "labels cannot be null"); this.mismatchedNode = mismatchedNode; }Добавьте импорт:
import java.util.Objects;src/main/java/org/antlr/v4/runtime/tree/pattern/RuleTagToken.java (1)
62-70: Несоответствие документации и поведения при null.Javadoc указывает
@throws IllegalArgumentException if ruleName is null or empty, однако приruleName == nullбудет выброшенNullPointerExceptionот вызоваisEmpty(), а неIllegalArgumentException.В контексте
@NullMarkedэто может быть приемлемо (non-nullable параметр не должен быть null), но тогда следует обновить документацию для согласованности.📝 Вариант 1: Обновить документацию
- * `@throws` IllegalArgumentException if {`@code` ruleName} is {`@code` null} or empty. + * `@throws` IllegalArgumentException if {`@code` ruleName} is empty.📝 Вариант 2: Добавить явную проверку на null
public RuleTagToken(String ruleName, int bypassTokenType, `@Nullable` String label) { + if (ruleName == null || ruleName.isEmpty()) { - if (ruleName.isEmpty()) { throw new IllegalArgumentException("ruleName cannot be empty."); }src/main/java/org/antlr/v4/semantics/RuleCollector.java (1)
74-78: Избыточный вызовsetType(ARG)дляr.args.
ScopeParser.parseTypedArgListуже создаётAttributeDictс типомDictType.ARGв конструкторе (см.ScopeParser.java, строка 57). Повторный вызовsetType(AttributeDict.DictType.ARG)здесь не меняет значение и является избыточным.♻️ Предлагаемое упрощение
if (arg != null) { r.args = ScopeParser.parseTypedArgList(arg, arg.getText(), g); - r.args.setType(AttributeDict.DictType.ARG); arg.resolver = r.alt[currentOuterAltNumber]; }src/main/java/org/antlr/v4/tool/AttributeDict.java (1)
67-69: Конструктор требует обязательныйDictType.Конструктор теперь принимает
DictTypeкак обязательный параметр, что гарантирует инициализацию типа при создании объекта. Это улучшает null-safety, но создаёт паттерн, при котором вызывающий код иногда вынужден переопределять тип черезsetType()(например, дляretvalsиlocals).Возможно, стоит рассмотреть добавление фабричных методов или перегруженных версий
parseTypedArgListдля разных типов словарей, чтобы избежать двухэтапной инициализации.src/main/java/org/antlr/v4/analysis/LeftRecursiveRuleTransformer.java (1)
172-177: Избыточный вызовsetType(ARG).Аналогично
RuleCollector.java, здесь вызовsetType(AttributeDict.DictType.ARG)избыточен, так какparseTypedArgListуже создаётAttributeDictс типомARGв конструкторе.♻️ Предлагаемое упрощение
ActionAST arg = (ActionAST) r.ast.getFirstChildWithType(ANTLRParser.ARG_ACTION); if (arg != null) { r.args = ScopeParser.parseTypedArgList(arg, arg.getText(), g); - r.args.setType(AttributeDict.DictType.ARG); arg.resolver = r.alt[1]; // todo: isn't this Rule or something? }src/main/java/org/antlr/v4/tool/Alternative.java (1)
55-61: Сделайте полеactionsнеизменяемым по ссылке.Это уберёт риск случайной переустановки списка извне при сохранении мутабельности содержимого.
♻️ Возможная правка
- public List<ActionAST> actions = new ArrayList<>(); + public final List<ActionAST> actions = new ArrayList<>();src/main/java/org/antlr/v4/runtime/ListTokenSource.java (1)
79-82: Javadoc обещаетNullPointerException, но явная проверка отсутствует.Документация на строках 77-78 указывает
@throws NullPointerException if tokens is null, однако в теле конструктора нет явной проверки. NPE произойдёт позже при обращении кtokens, что затруднит диагностику.🛡️ Предлагаемое исправление для явной проверки
public ListTokenSource(List<? extends Token> tokens, `@Nullable` String sourceName) { + if (tokens == null) { + throw new NullPointerException("tokens cannot be null"); + } this.tokens = tokens; this.sourceName = sourceName; }
Первая часть работ в рамках рефакторинга для улучшения диагностики и использования null значений.
В процессе рефакторинга доработан генератор классов. В прикладных решениях необходимо перегенерировать классы.