denols root_dir logic contains incorrect logic

[bodograumann]

Description

I haven't read all the previous issues regarding the denols config, but currently the logic contains redundant statements.
In particular "deno.lock" is contained both in project_root and in deno_lock_root. The condition for starting the LS i

    if
      (deno_lock_root and (not project_root or #deno_lock_root > #project_root))
      or (deno_root and (not project_root or #deno_root >= #project_root))
    then

However, when deno_lock_root is true, project_root will also always be true.
Similarly #deno_lock_root > #project_root can never be true, because if the lock file is closer than all other project files, it is still determining project_root.

Maybe "deno.lock" should be removed from root_markers?

[justinmk]

hmm, WDYT @isaacs

[isaacs]

It's possible that this could be simplified, sure. I sort of just hacked at it until it behaved properly, and there were a few other edge cases that were brought up at the time.

The intention is that if a deno.lock or deno.json file is found closer than other root markers (or at the same level), then it's a deno file. If there's a deno.lock file that's further away than other root markers, then it might not be.

For example, you might have something like this in a complicated monorepo:

./.git/
./deno.lock
./deno.json
./src/foo.ts   <-- deno file
./npmproject/ <-- nested non-deno project
./npmproject/package-lock.json
./npmproject/package.json
./npmproject/src/foo.ts  <-- not a deno file
./npmproject/packages/denoproject/ <-- sub-workspace in the non-deno project
./npmproject/packages/denoproject/deno.json
./npmproject/packages/denoproject/src/foo.ts <-- this is a deno file

I ran into this on the getsentry/sentry-javascript project, which is mostly traditional typescript that should be handled by ts_ls, but has a deno SDK in packages/deno which is code that runs on Deno.

[bodograumann]

Interesting example. From reading the code I don't think the current config works that way.
The only non-deno root marker is .git. So the result of root_dir will be the same for ./src/foo.ts and ./npmproject/src/foo.ts. Both will be marked as deno files, because deno_root and project_root are the same.

So we should check for package.json, package-lock.json and the likes.
A similar thing happens in the vtsls and eslint configs. So wouldn't it make sense to have shared logic?
Then at least the various lsps would be attached consistently.
The shared function could determine what the current project type is (npm, deno, bun) by looking at the closest root marker.
It would have to apply a (configurable?) preference for the situation where there are multiple different markers in the same root directory.
Everywhere we need to distinguish this we could then just call this project type function and e.g. activate the ls in denols.lua or skip activation in vtsls.lua and eslint.lua if the project is a deno project.
I guess if there is no difference for bun projects, the function could simply be is_deno_project:

local prefer_deno = false
function is_deno_project()
  local deno_root = vim.fs.root(bufnr, { 'deno.json', 'deno.jsonc', 'deno.lock' })
  if not deno_root then
    return false
  end

  local other_root = vim.fs.root(bufnr, { 'package-lock.json', 'yarn.lock', 'pnpm-lock.yaml', 'bun.lockb', 'bun.lock' })
  return not other_root or #deno_root > #other_root or deno_root == other_root and prefer_deno 
end

Currently there is a distinction between deno project root and deno lock root in the code (which does not have any effect, because both are always also the project_root atm). So maybe that should be taken into account for deciding when we have both a deno root marker and another root marker in the same directory, but I don't know what the reasoning there might be.

[isaacs]

@bodograumann Yeah, I think that makes sense, but the real takeaway here is that we probably could benefit from more comprehensive test cases to be defined explicitly. I doubt anyone is particular about how it happens, per se, but it'd be good to just lay out basically every project nesting combination possible, make a decision about whether each one should be considered "deno" or "not deno", and then make sure the code passes.