Conversation
There was a problem hiding this comment.
Notice: It looks like you're using hustcer/deepseek-review, but the CHAT_TOKEN hasn't been configured in your repo's Variables/Secrets. Please ensure this token is set for proper functionality. For step-by-step guidance, refer to the CHAT_TOKEN Config section of README.
87adfa6 to
a3a38cd
Compare
|
@copilot 请用中文review一下 |
@copilot Please review it in Chinese |
|
@github-actions review一下 |
There was a problem hiding this comment.
中文:整体方向正确:用 util.viewLiteral/util.escapeInvalidUtf8 统一处理非法 UTF-8,并在 hover 截断时按字符边界切割,避免产生无效 UTF-8。但有几处需要确认或修正。
script/core/hover/description.lua中的截断逻辑存在边界问题:
local nextCharacter = utf8.offset(view, 0, max + 1)
view = view:sub(1, nextCharacter - 1) .. '...'utf8.offset(s, 0, i) 返回的是包含第 i 个字节的字符的起始位置,而不是“下一个字符”的位置。当 max + 1 恰好落在一个字符的起始字节上时,nextCharacter 就等于 max + 1,于是 sub(1, max) 会把这个字符截掉一半,仍然可能产生无效 UTF-8;当 max + 1 落在字符中间时,返回的是该字符的起始位置,sub(1, nextCharacter - 1) 又会比预期少截一个字符。建议改为先取 utf8.offset(view, max + 1)(即第 max + 1 个字符的起始位置),再 sub(1, nextCharacter - 1);或直接用 utf8.offset(view, max) 得到第 max 个字符的结束位置。另外 utf8.offset 在越界时会返回 nil,虽然此处 #view > max 保证不会越界,但建议加断言或兜底,避免后续改动引入崩溃。
escapeInvalidUtf8的提前返回改变了原有语义,需要确认调用方是否依赖“总是返回新字符串”:
function m.escapeInvalidUtf8(str)
if utf8Len(str) then
return str
end
...viewString 中原本是 if not utf8Len(str) then str = escapeInvalidUtf8(str) end,现在等价,没问题。但 m.escapeInvalidUtf8 现在是公开 API,返回的可能是入参本身(同一引用)。如果调用方会修改返回值,可能意外修改原字符串。建议在文档/注释中说明,或统一返回副本。
-
util.viewLiteral的行为需要确认:('%q'):format(key)与viewLiteral的输出是否在所有情况下一致?测试里期望'"\\194"',说明viewLiteral对非法字节做了转义。但%q对\0、\n、\r等的转义规则与viewLiteral是否一致?如果viewLiteral使用了viewString的[[...]]长字符串形式,那么对于包含]]或换行的 key,输出格式会与原来的%q不同,可能影响 completion label 的展示或客户端解析。建议补充针对\n、\0、]]等边界 key 的测试。 -
测试断言
assert(utf8.len(value))和assert(utf8.len(json.encode {...}))只验证了“不是 nil”,即字符串是合法 UTF-8。这符合本次修复目标,但utf8.len对空字符串返回 0(真值),对非法 UTF-8 返回nil, pos。建议在断言失败时输出位置信息,便于定位,例如assert(utf8.len(value), ('invalid utf8 at %d'):format(select(2, utf8.len(value))))。 -
changelog.md描述准确,但建议同时说明 hover 截断现在按字符边界处理,避免用户误以为只修了 completion。
English: The overall direction is correct: unifying invalid UTF-8 handling via util.viewLiteral/util.escapeInvalidUtf8 and truncating hover output on character boundaries avoids producing invalid UTF-8. However, several points need confirmation or fixes.
- The truncation logic in
script/core/hover/description.luahas an off-by-one/boundary issue:
local nextCharacter = utf8.offset(view, 0, max + 1)
view = view:sub(1, nextCharacter - 1) .. '...'utf8.offset(s, 0, i) returns the start position of the character containing byte i, not the position of the "next" character. When max + 1 happens to be the first byte of a character, nextCharacter equals max + 1, so sub(1, max) cuts that character in half and can still yield invalid UTF-8. When max + 1 falls inside a character, it returns that character's start, so sub(1, nextCharacter - 1) truncates one character less than intended. Prefer utf8.offset(view, max + 1) (start of the (max+1)-th character) then sub(1, nextCharacter - 1), or use utf8.offset(view, max) to get the end of the max-th character. Also, utf8.offset returns nil when out of range; although #view > max guards this today, add an assertion or fallback to avoid future crashes.
- The early return in
escapeInvalidUtf8changes semantics; confirm callers don't rely on always getting a new string:
function m.escapeInvalidUtf8(str)
if utf8Len(str) then
return str
end
...In viewString this is equivalent to the previous if not utf8Len(str) then str = escapeInvalidUtf8(str) end, so it's fine there. But m.escapeInvalidUtf8 is now public API and may return the input by reference. If callers mutate the result, they could unexpectedly mutate the original string. Document this or return a copy consistently.
-
Confirm
util.viewLiteralbehavior: is its output identical to('%q'):format(key)in all cases? The test expects'"\\194"', showing it escapes invalid bytes. But doesviewLiteralmatch%qfor\0,\n,\r, etc.? IfviewLiteralusesviewString's[[...]]long-string form, keys containing]]or newlines will render differently from%q, potentially affecting completion label display or client parsing. Add tests for boundary keys like\n,\0,]]. -
The test assertions
assert(utf8.len(value))andassert(utf8.len(json.encode {...}))only verify "not nil", i.e. the string is valid UTF-8. That matches the fix's goal, bututf8.lenreturns 0 (truthy) for empty strings andnil, posfor invalid UTF-8. Include the position in the failure message for easier debugging, e.g.assert(utf8.len(value), ('invalid utf8 at %d'):format(select(2, utf8.len(value)))). -
The
changelog.mdentry is accurate, but also mention that hover truncation now respects character boundaries, so users don't assume only completion was fixed.
It is possible for lua-language-server to send invalid utf-8.
Some clients (e.g. Zed) will drop the connection if an LSP sends invalid data.
Hovering this will trigger the invalid UTF-8 in the response.
Similarly enum completions and labels for this include raw 0xC2: