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

ALERename: Fix an issue with unlisted buffers #3782

Closed
wants to merge 1 commit into from

Conversation

jeremija
Copy link
Contributor

@jeremija jeremija commented Jun 26, 2021

After calling ALERename on certain symbols, some files would be incorrectly replaced. The whole file would be truncated, and the only contents would be the new variable name.

I was able to consistently reproduce it using the following configuration:

let g:ale_linters = {
\  'go': ['gopls', 'golangci-lint'],
\}

Whenever I rename a symbol in myfile.go which is also referenced from an unopened file myfile_test.go, the whole myfile_test.go is truncated.

I tracked it down to bufnr('/path/to/myfile_test.go') returning number for a buffer that I never opened.

I later noticed that this buffer was unlisted (visible only via :ls!). So there must be something in the golangci-lint linter that opens this file in the background.

This fix adds a check for hidden buffers before applying code actions.

Closes #3781

After calling ALERename on certain symbols, some files would be
incorrectly replaced. The whole file would be truncated, and the only
contents would be the new variable name.

I was able to track it down to the following configuration:

    let g:ale_linters = {
    \  'go': ['gopls', 'golangci-lint'],
    \}

Whenever I tried to rename a symbol in `myfile.go`, which was referenced from an unopened file `myfile_test.go`, the whole `myfile_test.go` would be truncated.

I tracked it down to `bufnr('/path/to/myfile_test.go')` returning number
for a buffer that I never opened.

I later noticed that this buffer was unlisted (visible only via :ls!).
So there must be something in the golangci-lint linter that opens this
file in the background.

This fix adds a check for hidden buffers before applying code actions.

Closes dense-analysis#3781
@jeremija
Copy link
Contributor Author

jeremija commented Jun 26, 2021

cc: @w0rp @liskin @daliusd

Copy link
Contributor

@daliusd daliusd left a comment

Choose a reason for hiding this comment

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

Looks good to me. That’s all I can do however.

@liskin
Copy link
Contributor

liskin commented Jun 26, 2021

@davidatbu Is this the problem you encountered in #3478 (comment)?

(I never use :bd so that's why I never encountered it.)

Copy link
Contributor

@liskin liskin left a comment

Choose a reason for hiding this comment

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

Good catch!

Should the if a:should_save || l:buffer < 0 line become if a:should_save || !l:is_opened?

@jeremija
Copy link
Contributor Author

Hey all, thanks for taking a look at this so quickly! After I made this change yesterday, I realized we can further improve the rename operation, since there are some cases when renaming simply fails after doing it twice.

So I'm going to close this MR in favor of #3783 - I'd appreciate if you could have a look at the change there 🙇‍♂️

@jeremija jeremija closed this Jun 27, 2021
@dnaaun
Copy link

dnaaun commented Jun 27, 2021

@liskin, this was exactly the problem i described in that comment you mentioned! Hope it gets fixed soon.

@jeremija
Copy link
Contributor Author

Thanks @davidatbu, I'd appreciate if you could have a look at MR #3783 to see if you can find any bugs?

I think it should all be resolved now, but it's still possible there are some corner cases I haven't tested.

@dnaaun
Copy link

dnaaun commented Jun 28, 2021 via email

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.

ALERename: unlisted buffers are modified incorrectly
4 participants