Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ftdetect tests fail in Neovim 0.10 #89

Closed
laniakea64 opened this issue May 28, 2024 · 10 comments
Closed

ftdetect tests fail in Neovim 0.10 #89

laniakea64 opened this issue May 28, 2024 · 10 comments
Labels
bug Something isn't working external The cause of the issue is external to this project

Comments

@laniakea64
Copy link
Collaborator

laniakea64 commented May 28, 2024

$ nvim --version
NVIM v0.10.0
Build type: Release
LuaJIT 2.1.1713484068
Run "nvim -V1 -v" for more info
$ TEST_VIM=nvim just ftdetect
cargo run --bin=test-ftdetect
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/test-ftdetect`
TEST FAILED: FtdetectCase { filename: None, content: Some("#!//usr/local/bin/just -f\n"), not_justfile: true } (file /tmp/.tmppbSGHk/s8F79): unexpectedly detected as 'just'
TEST FAILED: FtdetectCase { filename: None, content: Some("#!/usr/bin/just"), not_justfile: true } (file /tmp/.tmppbSGHk/xVKsDhwT): unexpectedly detected as 'just'
TEST FAILED: FtdetectCase { filename: None, content: Some("#!/usr/bin/just\n"), not_justfile: true } (file /tmp/.tmppbSGHk/a): unexpectedly detected as 'just'
TEST FAILED: FtdetectCase { filename: None, content: Some("#!/usr//bin/env just -f\n"), not_justfile: true } (file /tmp/.tmppbSGHk/DsNnawsInkLm): unexpectedly detected as 'just'
Error: Custom { kind: Other, error: "4/31 tests failed." }
error: Recipe `ftdetect` failed on line 31 with exit code 1

These failures don't occur in Neovim <= 0.9.5 or in Vim.

It's not a bug in the test runner. Neovim 0.10.0 really does detect these as justfiles.

Turns out shebangs with repeated slashes like #!/usr//bin/just -f actually do work (at least on Xubuntu 22.04), so 2 of these 4 test cases are wrong. But the shebangs invoking just without flags are not correct just script shebangs and should not result in detection as justfile.

@laniakea64 laniakea64 added bug Something isn't working external The cause of the issue is external to this project labels May 28, 2024
@laniakea64
Copy link
Collaborator Author

laniakea64 commented May 29, 2024

git bisect of Neovim commit history points to neovim/neovim@1338140 as the culprit, but that looks like it's only duplicating part of our detection of filenames?

Manually removing those lines from runtime/lua/vim/filetype.lua in an installed Neovim doesn't get rid of the problem, but doing so in Neovim source code & compiling that does work? 😕

Where is the incorrect shebang-based filetype detection coming from? Can we block or override it somehow (without impacting any filetypes other than justfiles)?

@laniakea64
Copy link
Collaborator Author

Turns out shebangs with repeated slashes like #!/usr//bin/just -f actually do work (at least on Xubuntu 22.04), so 2 of these 4 test cases are wrong.

Fixed in b46dabb . Remaining failures:

$ TEST_VIM=nvim just ftdetect
cargo run --bin=test-ftdetect
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/test-ftdetect`
TEST FAILED: FtdetectCase { filename: None, content: Some("#!/usr/bin/just"), not_justfile: true } (file /tmp/.tmpGCg6M4/TbqAiKYw1pnNMK): unexpectedly detected as 'just'
TEST FAILED: FtdetectCase { filename: None, content: Some("#!/usr/bin/just\n"), not_justfile: true } (file /tmp/.tmpGCg6M4/vE6k): unexpectedly detected as 'just'
Error: Custom { kind: Other, error: "2/31 tests failed." }
error: Recipe `ftdetect` failed on line 31 with exit code 1

@NoahTheDuke
Copy link
Owner

Hmmmmmm that's very strange! I'm not sure why it being added upstream would change our behavior.

@NoahTheDuke
Copy link
Owner

Looking at the failures, what's wrong with those two lines? They look like valid shebangs to me.

@laniakea64
Copy link
Collaborator Author

They are missing the -f or --justfile flag to run the file as a just script.

Trying to use such shebang in a just script only gives error: No justfile found.

To make such shebang not result in an error, there must be a normal justfile runnable from the same directory as the script is located. In which case just will run the recipe named like the script's filename. So this is not a just script, it's an unusual style of entry point to an external justfile. So it shouldn't be automatically detected as justfile.

@NoahTheDuke
Copy link
Owner

Given that this can be different depending on platform and you can set command line arguments (--working-directory . as in that chapter), it might be worthwhile to be permissive here.

@laniakea64
Copy link
Collaborator Author

it might be worthwhile to be permissive here.

It's already very permissive. The current match is just immediately followed by any non-alphabetic non-newline character.

au BufNewFile,BufRead * if getline(1) =~# '\v^#!/%(\w|[-/])*/%(env%(\s+-S)?\s+)?just\A' | setfiletype just | endif

@laniakea64
Copy link
Collaborator Author

Maybe this is why? - https://github.com/neovim/neovim/pull/22140/files

Typically that would not be an unreasonable assumption. just scripts are unusual in this way.

Initial testing suggests this code might work around the problem, but...somehow this doesn't feel like a good answer.

if has("nvim-0.10")
  call luaeval("vim.filetype.add({ extension = { just = function()\nreturn nil\nend } })")
endif

IIUC a better solution would override this while doing all Neovim filetype detection in Lua, and wrap the autocmds in an if !has("nvim") block or similar?

Also found https://neovim.discourse.group/t/how-to-modify-filetype-detection-for-filename-extension/3994/2 which would explain why editing runtime file didn't work.

@laniakea64
Copy link
Collaborator Author

Ok, I've played with it a bit and think we could try doing filetype detection by name in Lua for Neovim. That way we can disable the problematic extension-based matching without killing the benefits of Lua filetype detection for Neovim users.

Let's keep the autocmd for file contents for both Vim and Neovim.

I'll draft a PR.

@laniakea64
Copy link
Collaborator Author

Fixed by #91 , filetype detection tests now pass locally in Vim 8.2 & 9.1.437, Neovim 0.4.3 / 0.8.3 / 0.9.5 / 0.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working external The cause of the issue is external to this project
Projects
None yet
Development

No branches or pull requests

2 participants