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

g:ale_lint_on_enter = 1 does not seem to work anymore (at least for Go) #734

Closed
svanharmelen opened this issue Jul 6, 2017 · 51 comments
Closed
Labels

Comments

@svanharmelen
Copy link
Contributor

svanharmelen commented Jul 6, 2017

I had to manually save the file to trigger the linting (which is probably the cmd shown at the bottom)...


Current Filetype: go
Available Linters: ['go build', 'gofmt', 'golint', 'gometalinter', 'gosimple', 'go vet', 'staticcheck']
Enabled Linters: ['gometalinter']
Linter Variables:

let g:ale_go_gometalinter_options = ' --aggregate --fast --sort=line --vendor --vendored-linters --disable=gas --disable=goconst --disable=gocyclo '
Global Variables:

let g:ale_echo_cursor = 1
let g:ale_echo_msg_error_str = 'Error'
let g:ale_echo_msg_format = '%s'
let g:ale_echo_msg_warning_str = 'Warning'
let g:ale_enabled = 1
let g:ale_keep_list_window_open = 0
let g:ale_lint_delay = 200
let g:ale_lint_on_enter = 1
let g:ale_lint_on_save = 1
let g:ale_lint_on_text_changed = 'always'
let g:ale_linter_aliases = {}
let g:ale_linters = {'go': ['gometalinter'], 'html': [], 'javascript': ['eslint']}
let g:ale_open_list = 0
let g:ale_set_highlights = 0
let g:ale_set_loclist = 1
let g:ale_set_quickfix = 0
let g:ale_set_signs = 1
let g:ale_sign_column_always = 1
let g:ale_sign_error = '✖'
let g:ale_sign_offset = 1000000
let g:ale_sign_warning = '⚠'
let g:ale_statusline_format = ['%d error(s)', '%d warning(s)', 'OK']
let g:ale_warn_about_trailing_whitespace = 0
Command History:

(finished - exit code 1) ['/usr/local/bin/bash', '-c', 'gometalinter --include=''^/Users/svanharmelen/Documents/GoCode/src/github.com/hashicorp/packer/builder/cloudstack/step_create_instance.go.*$'' --aggregate --fast --sort=line --vendor --vendored-linters --disable=gas --disable=goconst --disable=gocyclo ''/Users/svanharmelen/Documents/GoCode/src/github.com/hashicorp/packer/builder/cloudstack''']

/Users/svanharmelen/Documents/GoCode/src/github.com/hashicorp/packer/builder/cloudstack/step_create_instance.go:68::warning: declaration of "err" shadows declaration at step_create_instance.go:51 (vetshadow) ---
@w0rp
Copy link
Member

w0rp commented Jul 6, 2017

Can you produce a project I can test this with? I'm not really a Go user, so I'm not the best at testing it.

@w0rp w0rp added the bug label Jul 6, 2017
@svanharmelen
Copy link
Contributor Author

Sure...

package main

func main() {
	// Create an error message
	bla
}

func Bla() {
	// Does nothing but should have a comment
}

This should give you an error and a warning.

Now I just noticed that this only doesn't work when I open a saved session (using vim-session). If I manually open the file using NERDTree it does work.

@w0rp
Copy link
Member

w0rp commented Jul 6, 2017

Ah, interesting. I might be able to repeat that easily with other languages too.

@svanharmelen
Copy link
Contributor Author

Yes, I guess it should not be limited to Go indeed...

@w0rp
Copy link
Member

w0rp commented Jul 6, 2017

I can't repeat this bug. I installed gometalinter and I see some errors if I open the file in various ways.

@svanharmelen
Copy link
Contributor Author

Anything I can do to help make it repeatable? Did you also try with vim-session? I can make a small video if that would help in any way...

@w0rp
Copy link
Member

w0rp commented Jul 9, 2017

If you can record the bug with https://asciinema.org/, that usually works best.

@svanharmelen
Copy link
Contributor Author

Ok... So I learned that it only fails when NERDTree is open when I save the session. In the screencast you can see pretty clear that it only marks the issues after I :w

jul-10-2017 10-52-48

@w0rp
Copy link
Member

w0rp commented Jul 10, 2017

I haven't tried to repeat that yet myself, but I've got a hunch as to what the problem is. The event for the buffer being created is being fired while Vim has the buffer for NERDTree open, so ALE doesn't check the buffer when it's created.

I bet this can be fixed by removing the delay for checking the buffer and by passing the buffer number from the event on to the function for checking the buffer.

@jonaz
Copy link

jonaz commented Jul 11, 2017

I think there is a problem with the regexp for include in gometalinter?

correct

$ gometalinter /tmp/ --include "/tmp/test.go.*$" 
../../../../../../../../../../../tmp/test.go:21:9:error: undeclared name: asfd (gotype)

incorrect

$ gometalinter /tmp/ --include "^/tmp/test.go.*$" 

As you can see if i add ^ the beginning like this plugin does it doesnt work.

Thats because the output seems to always output relative paths and not absolute.

@svanharmelen
Copy link
Contributor Author

@jonaz unless I completely misunderstand your comment, I don't see how this is related? Are you saying that this is the reason that ALE isn't triggered when opening files using OpenSession?

@jonaz
Copy link

jonaz commented Jul 11, 2017

For me it triggers other linters than gometalinter. But gometalinter never runs because the include stuff. I mistakenly thought that only gometalinter was a problem here. I'll guess i'll open a new issue.

@svanharmelen
Copy link
Contributor Author

svanharmelen commented Jul 11, 2017

@jonaz check...

As for your issue, I don't think there is a problem with the regex used for gometalinter. I use this linter daily for months now without any issues and before that I used the vim-go version (which uses the exact same regex) for years.

So I guess you should include a way to reproduce this from within vim and add the output of :ALEInfo to show what command was exactly triggered.

See here an example of my output which is exactly what I would expect:


 Current Filetype: go
Available Linters: ['go build', 'gofmt', 'golint', 'gometalinter', 'gosimple', 'go vet', 'staticcheck']
  Enabled Linters: ['gometalinter']
 Linter Variables:

let g:ale_go_gometalinter_options = ' --aggregate --fast --sort=line --vendor --vendored-linters --disable=gas --disable=goconst --disable=gocyclo '
 Global Variables:

let g:ale_echo_cursor = 1
let g:ale_echo_msg_error_str = 'Error'
let g:ale_echo_msg_format = '%s'
let g:ale_echo_msg_warning_str = 'Warning'
let g:ale_enabled = 1
let g:ale_fix_on_save = 0
let g:ale_fixers = {}
let g:ale_keep_list_window_open = 0
let g:ale_lint_delay = 200
let g:ale_lint_on_enter = 1
let g:ale_lint_on_save = 1
let g:ale_lint_on_text_changed = 'always'
let g:ale_linter_aliases = {}
let g:ale_linters = {'go': ['gometalinter'], 'html': [], 'javascript': ['eslint']}
let g:ale_open_list = 0
let g:ale_set_highlights = 0
let g:ale_set_loclist = 1
let g:ale_set_quickfix = 0
let g:ale_set_signs = 1
let g:ale_sign_column_always = 1
let g:ale_sign_error = '✖'
let g:ale_sign_offset = 1000000
let g:ale_sign_warning = '⚠'
let g:ale_statusline_format = ['%d error(s)', '%d warning(s)', 'OK']
let g:ale_warn_about_trailing_whitespace = 0
  Command History:

(finished - exit code 1) ['/usr/local/bin/bash', '-c', 'gometalinter --include=''^/Users/svanharmelen/Documents/GoCode/src/demo/main.go.*$''  --aggregate --fast --sort=line --vendor --vendored-linters --disable=gas --disable=goconst --disable=gocyclo  ''/Users/svanharmelen/Documents/GoCode/src/demo''']

<OUTPUT STARTS>
/Users/svanharmelen/Documents/GoCode/src/demo/main.go:5:2:error: bla (invalid operand) is not used (gotype)
/Users/svanharmelen/Documents/GoCode/src/demo/main.go:5:2:error: undeclared name: bla (gotype)
/Users/svanharmelen/Documents/GoCode/src/demo/main.go:8:1:warning: exported function Bla should have comment or be unexported (golint)
<OUTPUT ENDS>

@jonaz
Copy link

jonaz commented Jul 11, 2017

@svanharmelen it works if i downgrade gometalinter to latest stable

go get -u gopkg.in/alecthomas/gometalinter.v1.

But if i run HEAD with

go get -u github.com/alecthomas/gometalinter

The problem exists

Also tried HEAD and just removed the --include line in /ale_linters/go/gometalinter.vim and that also worked fine.

guess im sticking with v1.

Sorry for polluting your issue :)

@svanharmelen
Copy link
Contributor Author

@jonaz no problem... But in that case you should maybe create an issue with gometalinter to discuss your finding?

@w0rp
Copy link
Member

w0rp commented Jul 11, 2017

That regular expression for the parameter does need to be fixed at some point. The problem with is that a path to a file is used as a regular expression. That's not correct, because a path is not a regular expression. Any special regular expression characters will be interpreted as such.

Instead, the path ought to be escaped such that it can be safely used inside of a regular expression.

@w0rp
Copy link
Member

w0rp commented Jul 11, 2017

Does anyone know what variant of regular expressions that --include parameter uses? Is it PCRE, or something else?

@w0rp
Copy link
Member

w0rp commented Jul 11, 2017

That regex issue is a separate issue, though.

@w0rp
Copy link
Member

w0rp commented Jul 11, 2017

If it's not too slow, try removing the --include parameter completely, capture the filename in the handler regular expression, and use ale#path#IsBufferPath to filter the results after that.

@svanharmelen
Copy link
Contributor Author

That is really really slow and is the reason I added this a while back. My CPU was going crazy without using the --include option.

But working on a fix for this as I noticed that indeed they changed the behaviour in gometalinter.

@w0rp
Copy link
Member

w0rp commented Jul 11, 2017

I see, I thought that's why the parameter was added.

I recommend escaping the path so it's safe to include in the regular expression in that case. It should be easy to write some tests to ensure that the escaping is done properly.

@jonaz
Copy link

jonaz commented Jul 12, 2017

@w0rp seems to be RE2 regexp syntax according to https://golang.org/pkg/regexp/

@w0rp
Copy link
Member

w0rp commented Jul 12, 2017

Yes, we are escaping the paths now, so that issue has been resolved.

@svanharmelen
Copy link
Contributor Author

@w0rp did you change anything related to this issue? I now don't get any linting errors/warning at all when NERDTree is open and I save a file. It flashes and then I don't get the errors in either the gutter or the location list.

If I close NERDTree and write the file it works as expected...

@w0rp
Copy link
Member

w0rp commented Jul 18, 2017

NERDTree is now ignored, so linting is never done for the NERDTree buffer. That was the cause of other problems.

I'll have to look at that later.

@w0rp
Copy link
Member

w0rp commented Jul 18, 2017

Could you record this behaviour with https://asciinema.org/ so I can see exactly what you're doing? If I open NERDTree in a split window I get all of the errors I expect to see.

@svanharmelen
Copy link
Contributor Author

@w0rp see the example below... I continuously write the file to disk and sometimes ALE shows the expected errors, sometimes it doesn't and sometimes you only see the error signs flashing in the gutter really short before they are gone again:

jul-19-2017 11-06-42

@svanharmelen
Copy link
Contributor Author

@w0rp any clue how we can fix this? this inconsistent behaviour is really very annoying and personally I have no clue how to solve it 😞

@w0rp
Copy link
Member

w0rp commented Jul 31, 2017

I've never seen the bug myself, and I don't yet know how to repeat it.

@svanharmelen
Copy link
Contributor Author

Check... I had some issues with ASCII cinema, but I'll try it again and cycle back with (hopefully) more details/info...

@w0rp
Copy link
Member

w0rp commented Jul 31, 2017

I thought about this, and I think I might have an idea about where the bug might be, how to cover it with tests, and how to fix it. I'll let you know.

@svanharmelen
Copy link
Contributor Author

That would be really, really awesome! Standing by for further updates in that case... 👍

@w0rp
Copy link
Member

w0rp commented Jul 31, 2017

@svanharmelen Okay, pull master and try it again. I feel like I might have just fixed your issue. There was a problem when you did the following.

  1. Set ALE to only check a file with linters which can only check the file, and not the buffer.
  2. Check the file for errors. (Lint on save, on enter...)
  3. Check the buffer for errors, so on linters are run. (Lint on text changed...)
  4. All results are cleared.

Now the results, including signs, will only be cleared if no linters are run and there wasn't a linter in the list of enabled linters which wasn't run because it needed to check the file and not the buffer. It's hard to put that into a sentence. It's probably easier just to read the ale#engine#RunLinters function, which should explain itself.

@w0rp
Copy link
Member

w0rp commented Jul 31, 2017

Showing errors after restoring a session with NerdTREE might still need to be fixed.

@svanharmelen
Copy link
Contributor Author

It is looking better, but the problem is not fixed yet.

But since now the values/results are much more sticky (which looks good/better), I can clearly see that when I have NERDTree open ALE doesn't fire on :w anymore. As soon as I close NERDTree and :w again, it fires just fine.

https://drive.google.com/open?id=0BzfhEeJReB4rWXQ3TGdoS3pjdm8

I know you would like a ASCII cinema better, but just to give some fast insights... I first show the output of ALEInfo which shows ALE fired once. Then I execute :w a couple of times and the show ALEInfo again. As you can (hopefully) see, it now still shows it has only fired once.

Then I change a single char and again :w a couple of time. But still ALEInfo shows that single one time it fired... Hope this helps?

w0rp added a commit that referenced this issue Jul 31, 2017
@w0rp
Copy link
Member

w0rp commented Jul 31, 2017

Okay, try that. Now the delay for linting on enter has been removed, and the buffer numbers from the enter and save events will be used for deciding which buffers to check. That might make it check the Go file buffer if the buffer is somehow restored while the NERDTree buffer is focused. Writing the buffer which contains the Go code should results in the file being checked, and writing the NERDTree buffer should of course do nothing.

@svanharmelen
Copy link
Contributor Author

I think this fixes it! At least I'm no longer able to reproduce this. Let me try some more and if it now stays away, I'll update and close this issue.

Thank you very much for your help and this fix!

@w0rp
Copy link
Member

w0rp commented Aug 1, 2017

Nice. I thought that might have been the issue.

rsrchboy added a commit to rsrchboy/ale that referenced this issue Aug 2, 2017
* upstream/master:
  Cover the SaveEvent function with a test
  dense-analysis#734 - Use the buffer number from the events for entering buffers and saving buffers for checking buffers
  dense-analysis#734 - Do not clear file linter results when no buffers are run
  Add stylelint fixer
  Cover special LSP initialize response handling with Vader tests
  dense-analysis#517 - Get the Rust language server working in a basic way
  When servers never send an initialize response, but instead just publish diagnostics straight away, handle that as an initialize response
  Add some error message handling for LSP, for test purposes
  Fix some bugs so the PHP language server will show errors at least once
@svanharmelen
Copy link
Contributor Author

I don't see the problem anymore 🎉 Really happy this is fixed and I have stable results again! Thanks again!

@w0rp
Copy link
Member

w0rp commented Aug 2, 2017

Cool. 👍

@svanharmelen
Copy link
Contributor Author

svanharmelen commented Oct 6, 2017

@w0rp it seems this issue is back again... I updated the plugin after not doing that for a couple of weeks, and now again when I have NERDTree open it looks like ALE just doesn't fire when I save a file.

It does now automatically lint the files when opening from a session (which was the original issue of this issue that didn't work before), but when I save a buffer after that, ALEInfo shows it didn't do anything...

EDIT: When I call ALELint manually, it does seem to work as expected!

@w0rp
Copy link
Member

w0rp commented Oct 6, 2017

Are you able to trace commits where this does and does not happen?

@w0rp w0rp reopened this Oct 6, 2017
@svanharmelen
Copy link
Contributor Author

No, I'm afraid not... Did try commits of about 2 weeks, 4 weeks and 6 weeks ago, but they all failed. Strange enough calling ALELint directly works just fine. So it must be in the autocmd hook I suppose...

@svanharmelen
Copy link
Contributor Author

svanharmelen commented Oct 15, 2017

@w0rp I found the code that causes ALE not to be triggered (had some time to debug today)...

It's caused by this line: https://github.com/w0rp/ale/blob/master/autoload/ale.vim#L55

if I follow the buffer number that is passed around, that represents the correct buffer. But when I add a line like echom "Filetype: " . &filetype in the body of the failing test, it logs Filetype: nerdtree??

So that is clearly why nothing happens. I just don't understand how &filetype get's to be nerdtree. If I type :echo &filetype when in the buffer I'm testing with, it correctly says go.

Hope this additional info helps to understand and fix the issue?

@svanharmelen
Copy link
Contributor Author

Hmm... When I change line https://github.com/w0rp/ale/blob/master/autoload/ale.vim#L55 to:

if index(g:ale_filetype_blacklist, getbufvar(a:buffer, '&filetype')) >= 0

Then this check works as expected again, but still ALE isn't triggered 😞 Going to debug some more...

@svanharmelen
Copy link
Contributor Author

svanharmelen commented Oct 15, 2017

@w0rp I found and fixed the problems (2) in PR #998 :)

@w0rp w0rp closed this as completed in #998 Oct 15, 2017
w0rp added a commit that referenced this issue Oct 15, 2017
Fix #734 - Use the correct buffer for the filetype blacklist and such
@arthuryangcs
Copy link

@w0rp @svanharmelen I found it still exist when NERDTree is open and the current buffer file is not exist in the local directory.

@svanharmelen
Copy link
Contributor Author

@arthuryangcs are you sure this is the same problem? What does ALEInfo show after you save a buffer? I can imagine this is more related to how gometaliner works (or any other Go linter that is).

They are always executed on the current package, so if your PWD in vim is set to the dir of your local dir, I would not be surprised if ALE does trigger a lint action but on the current local dir. In this case you would need to make sure the PWD in correct for the buffer you're trying to save/lint.

@arthuryangcs
Copy link

arthuryangcs commented Oct 29, 2017

@svanharmelen I add remove_trailing_lines to g:ale_fixers for python and set g:ale_fix_on_save = 1.
If I open a file not exist in the current directory and open NERDTree, g:ale_fix_on_save = 1 not work. If I close the NERDTree or open a file exist in the current directory, It works.

 Current Filetype: python
Available Linters: ['flake8', 'mypy', 'pycodestyle', 'pylint']
  Enabled Linters: ['flake8', 'mypy', 'pylint']
 Linter Variables:
let g:ale_python_flake8_executable = 'flake8'
let g:ale_python_flake8_options = '--ignore E501'
let g:ale_python_flake8_use_global = 0
let g:ale_python_mypy_executable = 'mypy'
let g:ale_python_mypy_options = ''
let g:ale_python_mypy_use_global = 0
let g:ale_python_pylint_executable = 'pylint'
let g:ale_python_pylint_options = '--disable C0301,C0111,C0103'
let g:ale_python_pylint_use_global = 0
 Global Variables:
let g:ale_echo_cursor = 1
let g:ale_echo_msg_error_str = 'Error'
let g:ale_echo_msg_format = '%s'
let g:ale_echo_msg_warning_str = 'Warning'
let g:ale_enabled = 1
let g:ale_fix_on_save = 1
let g:ale_fixers = {'python': ['autopep8', 'isort', 'remove_trailing_lines', 'trim_whitespace']}
let g:ale_keep_list_window_open = 0
let g:ale_lint_delay = 200
let g:ale_lint_on_enter = 1
let g:ale_lint_on_save = 1
let g:ale_lint_on_text_changed = 'always'
let g:ale_linter_aliases = {}
let g:ale_linters = {'go': ['go build', 'gofmt', 'golint', 'go vet']}
let g:ale_open_list = 0
let g:ale_set_highlights = 1
let g:ale_set_loclist = 1
let g:ale_set_quickfix = 0
let g:ale_set_signs = 1
let g:ale_sign_column_always = 1
let g:ale_sign_error = '>>'
let g:ale_sign_offset = 1000000
let g:ale_sign_warning = '--'
let g:ale_statusline_format = ['%d error(s)', '%d warning(s)', 'OK']
let g:ale_warn_about_trailing_whitespace = 1

@w0rp
Copy link
Member

w0rp commented Oct 29, 2017

@arthuryangcs Could you open a new GitHub issue with that information, and any other information you can provide? I can attempt to repeat the bug myself and hopefully fix it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants