fix: switching from_stderr to false for solhint diagnostics

[ahashim]

This fixes not seeing warnings/errors when running null-ls diagnostics on Solidity files. The change will now make diagnostic messages to actually show up in the proper output buffer.

Here is the NullLsLog output for said diagnostics in debug mode:

[jose-elias-alvarez]

I won't have time to test this for a bit, but could it be due to a version incompatibility? I can see a scenario where upstream switched from stderr to stdout.

[ahashim]

Thanks for looking at this, and there's no rush - i've got it fixed locally via my branch.

It's unclear to me why it stopped working, but I suspect that I did not commit the proper configuration in my original PR, and just didn't notice until now when i ran the linter manually in the console. My nvim & solhint versions are still the same as the ones in that linked PR.

Feel free to verify & test locally before I mark it as ready for review. And thanks again!

[jose-elias-alvarez]

So I tested this out with a fresh install of solhint from npm (version 3.3.7) with a default config file generated with solhint --init and it works for me without this change, but not with it. I wonder what's going on? I'm not familiar with the ecosystem, but it looks like the latest version from npm does output to stderr.

[ahashim]

Interesting, then there must be something wrong with my local configuration. Thanks for verifying this works as intended out the box with vanilla solhint. I'll close this PR for now & investigate my local configuration further.

Thanks again!

[DavideSilva]

@ahashim I am also getting the same issue as you have. solhint does indeed report errors to null-ls. That I can verify by checking the null-ls.log, but the diagnostic only appears if I set the stderr option in the solhint.lua to false.

My nvim version is 0.7.0 and I installed the latest solhint, version 3.3.7, like @jose-elias-alvarez.

Should we reopen this issue until we can figure out exactly what's going on?

[ahashim]

@DavideSilva thanks for verifying. Yes, I'll reopen the PR, but will not mark it as ready until we figure out the root cause of the regression. I currently don't have much bandwidth to investigate, but can create a demo solidity project to experiment with null-ls settings sometime this week.

Appreciate you helping to triage the issue with @jose-elias-alvarez.

[ahashim]

I did some experimenting in a clean environment this morning to reproduce the bug, and here are my findings:

Operating System

Pop!_OS 21.10 x86_64

Software

nvim v0.7.0
solhint v3.3.7

Repository

https://github.com/ahashim/null-ls-solhint-sandbox

Steps to reproduce

1. Ensure you are running the specific versions of the software listed above, and clone the null-ls-solhint-sandbox repository to your local machine.
2. Ensure null-ls is in debug mode
3. In the root directory, run the following command: solhint SolhintTest.sol --formatter=unix, and you will see the following output:

SolhintTest.sol:10:5: Function name must be in mixedCase [Warning/func-name-mixedcase]
SolhintTest.sol:10:62: Code contains empty blocks [Warning/no-empty-blocks]

4. Open SolhintTest.sol in neovim, and check null-ls output with :NullLsLog

Using @jose-elias-alvarez's main branch for the solhint diagnostics builtin, the output is as follows:

Using @ahashim's solhint-bugfix branch for the solhint diagnostics builtin, the output is as follows:

As you can see, the error output for both branches is nil, and @ahashim's solhint-bugfix branch correctly allows the diagnostics to be propagated (in this case to trouble.nvim).

It seems that tests are also passing for the solhint-bugfix branch as well as seen in this CI run. To me, it seems that this PR fixes the bug & produces the intended behavior. However, I'd love to get some more verification on this before proceeding.

@DavideSilva or @jose-elias-alvarez If you can verify for me the above is correct, then perhaps we can figure out where the disconnect is. AFAIK solhint has not changed the way it produces output, so this situation is a bit puzzling to me.

Thanks again for taking the time to help with this 🙏🏾

[DavideSilva]

Thank you for putting this testing repo together. I can confirm that I have exactly the same behavior as you @ahashim.

Using the same Neovim and Solhint versions

Opening the SolhintTest.sol file with Neovim on the main branch of null-ls

Changing the stderr option to false in the solhint.lua diagnostic file, essentially using the ahashim:solhint-bugfix branch, the warnings now appear in Neovim

[jose-elias-alvarez]

Alright, not 100% sure what's going on but I can confirm that I see the behavior you describe with your reproduction. Let me know if this is okay to merge (I went ahead and pushed a fix that was causing CI to fail for an unrelated reason).

[ahashim]

Thanks for testing this & confirming the expected behavior @jose-elias-alvarez & @DavideSilva. I suspect the error was still mine when I initially committed the PR (and just didn't realize it for a bit).

Regardless, I'm happy to see this working again. Thank you for fixing the tests, and feel free to merge at your earliest convenience ✌🏾

[jose-elias-alvarez]

Thanks all!

[KalleChen]

Hi guys, @ahashim @jose-elias-alvarez although this PR was closed, but seems that this setting is not 100% sure, so I'm here to share what I have found.

Currently, I encounter a similar error with my project but I have to set stderr back to true to get my diagnostics. But after testing null-ls-solhint-sandbox repo, I have the same behavior above(setting stderr to false to get diagnostics).

After comparing the difference between my project and null-ls-solhint-sandbox, I found that if there're errors in solhint, the output will be in stderr. But like above null-ls-solhint-sandbox repo only have warning in solhint, so the output will not in stderr. Here is what I test with null-ls-solhint-sandbox

The following is what I have tested to reproduce.

I set the compile-version in .solhint.json to ^0.9.0 to get the error.

Then I can't get the diagnostics.

After setting `sdterr` back to true to get diagnostics.

But I'm not familiar with solhint, so I don't know why this happens.

[ahashim]

Thank for you investigating & sharing your findings @KalleChen. This seems like a configuration that should be solved on the solhint side of things in order for the null-ls plugin to have consistent behavior.

According to the solhint repository, the compiler version configuration option is turned on only if the user enables the {“extends”: “solhint:recommended”} flag in their config. This seems to be a sensible recommendation on their side, but is not part of the solhint:default rule set that comes out of the box. As a result I changed the null-ls std_err flag to false by default so errors could be viewed without any configuration changes when setting up a default solhint project.

Perhaps this needs to be rectified in the solhint repository so errors are always sent to std_err regardless of the ruleset, and as such can be caught by null-ls w/ a default flag of std_err set to true.

If that makes sense, i'll start an investigation on the solhint repo & can open up a PR to adjust the error output settings.

What are your thoughts?

Thank you again for helping to investigate.

[KalleChen]

@ahashim Thanks for your reply. Yeah, I think it should be rectified in the solhint repository. In my test case, changing console.log in printReports [solhint.js] to console.error would be the simplest way to make sure it sent message to std_err. But I'm not sure if that will affect other projects which use solhint, therefore, I'm still figuring out why solhint sends to std_err if there're errors.

Besides, while I investigated the solhint repo, it seems that the main maintainer fvictorio has no permission to write the repo, but he can still publish it on npm. You can found his response here. And the updated repo is here.

Also, I've opened an PR here to add formatter support for stdin process in solhint. Once that is merged and is published, we can then use stdin for null-ls here for real-time diagnostics. But there're still other issue for solhint-plugin-prettier while using stdin, I've open an issue here

Thanks again.

[KalleChen]

@ahashim Ok, I've found why this will happen. It seems that if there're Errors in diagnostics reports the process will exit with 1, therefore, the console.log output will be sent to std_err. I've opened the PR to fix this.

What're your thoughts about this?

[ahashim]

Thanks for the thorough investigation @KalleChen.

I have indeed been able to reproduce the issue on my end, and agree with your conclusions. I think we should move the discussion over to the PR you created to fix it (seeing as how this isn't a null-ls issue at this point).

Thank you again for getting to the bottom of this! 💪🏾

[jose-elias-alvarez]

Thanks all for investigating! The behavior we were seeing was pretty confusing, so I think the upstream solution makes sense.