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

Better error message on Flycheck Error message #11891

Merged
merged 4 commits into from
Apr 5, 2022
Merged

Better error message on Flycheck Error message #11891

merged 4 commits into from
Apr 5, 2022

Conversation

flipbit03
Copy link
Contributor

I have commented on this S-unactionable issue that the Flycheck error message should maybe provide a hint about what tool it actually runs. Searching on some places on the Internet I've found multiple people, including myself, losing copious amounts of time on the same issue. So I've decided to make this very small PR :-)

From an user experience standpoint, the current error message is unhelpful to the end user, because the end user does not know exactly what it needs to check/fix (outdated, broken, or missing cargo clippy). In my own case, cargo clippy was actually missing altogether (developing off rust:1.59.0-bullseye official Docker image).

Thanks in advance!

@bjorn3
Copy link
Member

bjorn3 commented Apr 3, 2022

I think it should only show that hint when you actually configured rust-analyzer to use clippy. rust-analyzer.checkOnSave.command defaults to check.

@Veykril
Copy link
Member

Veykril commented Apr 3, 2022

Ye we use check by default. We do record stderr in this code path already though, so we should just emit that with the error message as the other match arm already does

@flipbit03
Copy link
Contributor Author

flipbit03 commented Apr 3, 2022

This is strange. In my currently configured setup (Devcontainer/Codespaces) I do not have rust-analyzer.checkOnSave.command set to anything (so it should use the default value as you mentioned, check). Do you have any extra pointers on why adding clippy to the container fixed it?

Here's the Dockerfile for my Devcontainer (it's rather simple):

FROM rust:1.59.0-bullseye

# This fixed rust-analyzer error on each save.
RUN rustup component add clippy

# Install GCC (for Python), Python3 dev headers and PostgreSQL's headers/libraries.
RUN apt-get update && export DEBIAN_FRONTEND=noninteractive \
    && apt-get -y install --no-install-recommends libpq-dev pipenv gcc python3-dev

###############
# Create Jefferson
###############
ARG USER_NAME=jefferson
# Uid does not need to match outside UID in case of VSCode's Devcontainer (it is matched automatically)
ARG USER_UID=1000
ARG USER_GID=$USER_UID

RUN groupadd --gid $USER_GID $USER_NAME \
    && useradd --uid $USER_UID --gid $USER_GID -s /bin/bash -m $USER_NAME \
    && apt-get update \
    && apt-get -y install --no-install-recommends sudo \
    && echo $USER_NAME ALL=\(root\) NOPASSWD:ALL > /etc/sudoers.d/$USER_NAME \
    && chmod 0440 /etc/sudoers.d/$USER_NAME \
    # Docker without sudo support
    && groupadd docker \
    && usermod -a -G docker $USER_NAME

# Become Jefferson \o/
USER $USER_NAME

Thanks in advance!

NINJA EDIT: "rust-analyzer.checkOnSave.command": "clippy" was tucked in Devcontainer's devcontainer.json 🤦

Sorry for any trouble

@Veykril
Copy link
Member

Veykril commented Apr 4, 2022

Good to know you figured out why :)
Regarding this PR, do you care to implement what I've noted down earlier?

We do record stderr in this code path already though, so we should just emit that with the error message as the other match arm already does

If not that's fine then I'll do that in a bit, just wanna know if we can close this PR otherwise.

@flipbit03
Copy link
Contributor Author

@Veykril I am going to try to make it emit stderr. Updating this PR in a few. Thank you for the patience! (I'm looking to learn more and more about Rust in general 🙏 )

@flipbit03
Copy link
Contributor Author

@Veykril as requested, the PR has been updated and we are now outputting the contents of stderr together with the final error message. Thank you for your time in reviewing this!

@flipbit03 flipbit03 changed the title Better error message hinting about cargo clippy Better error message on Flycheck Error message (from: unactionable error message if we are using clippy as the checker) Apr 4, 2022
"Cargo watcher failed, the command produced no valid metadata (exit code: {:?})",
output.status
"Cargo watcher failed, the command produced no valid metadata (exit code: {:?})\nCargo's stderr output:\n{}",
output.status, from_utf8(&output.stderr).unwrap_or("(Error: Could not fetch Cargo's stderr output)")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use from_utf8_lossy instead? That is infallible. In any case the error should say that the stderr is not valid UTF-8 if you keep using from_utf8.

Copy link
Member

Choose a reason for hiding this comment

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

You don't want to use output.stderr here, we use streaming_output to fill in the error output into the error local here which is what contains the error (See the Err(e) branch below this one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your time @bjorn3 and @Veykril. I have updated my PR, now using error String variable to build the final output message.

@flipbit03 flipbit03 requested review from Veykril and bjorn3 April 5, 2022 14:19
@Veykril
Copy link
Member

Veykril commented Apr 5, 2022

Thanks!
bors r+

@bors
Copy link
Contributor

bors bot commented Apr 5, 2022

@bors bors bot merged commit ee6becc into rust-lang:master Apr 5, 2022
@flipbit03 flipbit03 deleted the chore/better_cargo_clippy_error_messages branch April 6, 2022 02:56
@Veykril Veykril changed the title Better error message on Flycheck Error message (from: unactionable error message if we are using clippy as the checker) Better error message on Flycheck Error message Apr 6, 2022
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.

3 participants