← 返回首页
fix(classpath): fix bug when generated sources are skipped by jdtls by olisikh · Pull Request #489 · nvim-java/nvim-java · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
Resetting focus

fix(classpath): fix bug when generated sources are skipped by jdtls#489

Merged
s1n7ax merged 5 commits into
nvim-java:mainfrom
olisikh:fix/classpath_gen_sources
Jul 3, 2026
Merged

fix(classpath): fix bug when generated sources are skipped by jdtls#489
s1n7ax merged 5 commits into
nvim-java:mainfrom
olisikh:fix/classpath_gen_sources

Conversation

olisikh commented May 20, 2026

Copy link
Copy Markdown
Contributor

Adds an experimental nvim-java workaround for JDTLS import failures caused by nested generated sources under target/generated-sources.

  • opt-in via experimental.fix_generated_sources
  • when enabled, promotes only nested generated roots matching target/generated-sources/**/src/{segment}/java
  • rewrites the parent .classpath entry exclusions so JDT can accept those nested roots
  • includes focused tests for valid and invalid generated-source layouts

olisikh force-pushed the fix/classpath_gen_sources branch from 973ff6f to f7de813 Compare May 20, 2026 18:45
olisikh force-pushed the fix/classpath_gen_sources branch from f7de813 to a70a343 Compare May 20, 2026 18:50

s1n7ax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Inline comments on the specific files/lines. Overall the approach is sound and the opt-in flag is the right call — see per-line notes for asks before merge.

end

return line:gsub('<classpathentry ', '<classpathentry excluding="' .. exclusion_value .. '" ', 1)
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Always overwrites the existing excluding= value. Combined with get_generated_source_exclusions unconditionally seeding annotations/, a project that never had annotations/ excluded will silently gain it, and any pre-existing custom exclusions will be dropped. Parse the current attribute, merge with the new set, and write the union.

olisikh Jun 8, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

set_exclusions now parses existing, merges union, writes back as sorted: old entries preserved, new entries added, no data loss.

end

return absolute_path
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Windows path mismatch. .classpath files always use / for path="..." (Eclipse format), but path_utils.path_separator is \ on Windows. The prefix built here uses backslashes while absolute_path from vim.fs.find may use forward slashes — comparison silently fails. Normalize both sides to / for all classpath-string operations.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Nice catch! Added normalization for slashes

end
end

return nil

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Regex-based XML editing is fragile. Only matches path="target/generated-sources" exactly — a trailing slash, single quotes, or a multiline tag will be missed. At minimum document this assumption in the module docstring; ideally use an XML parser or a more permissive pattern.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

still uses regex matching, but handles both types of quotes: " and '

end
end

for offset = #entry, 1, -1 do

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

add_classpath_entry always returns true, and the caller ignores it anyway. Either drop the return or actually report no-op vs. insert so the caller can use it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

dropped the return


local function get_generated_source_roots(module_root)
local generated_root = path_utils.join(module_root, 'target', 'generated-sources')
if not vim.uv.fs_stat(generated_root) then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Passing vim.pesc(...) with plain = false is confusing — the pesc only matters in pattern mode, but plain mode is what you want here. Use plain = true and drop the pesc call.

Also: limit = math.huge is unbounded — on large monorepos this walk can be slow. Consider a sane cap and a warning when hit, especially since this runs synchronously at setup().

end
end
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

annotations/ is always seeded into exclusions regardless of whether the user's project had it. This is the root of the overwrite behavior flagged on set_exclusions. Either derive the seed from the project's existing excluding= attribute, or document clearly why annotations/ is unconditionally injected.

file_changed = true
end
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

No logging. This silently rewrites user files. Per CLAUDE.md, use java-core.utils.log2 to log.info which .classpath was patched and which roots were added/excluded — critical for debugging an experimental feature where Maven may regenerate .classpath and silently lose the patch.

Also worth a one-line comment noting this is safe to re-run (has_classpath_entry + set_exclusions no-op make it idempotent in the common case).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

added logging


function M.patch(root)
local changed = false
for _, file in ipairs(vim.fs.find('.classpath', { path = root, type = 'file', limit = math.huge })) do

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Missing LuaDoc. Per CLAUDE.md, public API needs ---@ annotations. At minimum:

---@param root string ---@return boolean changed function M.patch(root)

Comment thread lua/java.lua
----------------------------------------------------------------------
-- init --
----------------------------------------------------------------------
if config.experimental.fix_generated_sources then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Runs synchronously at setup(). On large monorepos the recursive vim.fs.find walk could noticeably delay startup. Consider deferring (vim.schedule) or at least logging duration so users can see the cost.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

It has to run in sync I believe, otherwise jdtls would pick up an incorrect .classpath file.
Added duration calculation.

I've ran it on quite big projects with many generated source folders, and it was working quite OK, I did not notice a major hit.

local content = table.concat(vim.fn.readfile(classpath_file), '\n')
assert.is_falsy(content:find('target/generated-sources/src/main/java', 1, true))
end)
end)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Missing test coverage:

  • Idempotency — call patch() twice; second call should return false and leave the file unchanged.
  • Pre-existing excluding="..." attribute — confirm the merge/overwrite behavior (ties into the annotations/ concern in the source file).
  • Multi-module project with multiple .classpath files.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

added 3 more tests

s1n7ax commented Jun 7, 2026

Copy link
Copy Markdown
Member

@olisikh Hi, sorry for taking forever. Could you please these reviews. Could be just AI nagging. Leave a reply if so

olisikh commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

would be cool to merge #488 first, then I could resolve conflicts here and make it ready for merge.

s1n7ax commented Jul 3, 2026

Copy link
Copy Markdown
Member

@olisikh Approved

Hide details View details s1n7ax merged commit 116339b into nvim-java:main Jul 3, 2026
13 of 14 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Footer

© 2026 GitHub, Inc.