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 for semicolon on last line of function #11482

Merged

Conversation

fhahn
Copy link
Contributor

@fhahn fhahn commented Jan 11, 2014

This is a patch for #8005, thanks @lfairy for the hint.

It seems like block.expr is None, if the last line of a function has a semi colon (= it ends with a statement).

@kmcallister does this error message cover the intended use cases?
I'm not sure about the message, the wording and the span could probably be improved.

if ends_with_stmt {
self.tcx.sess.span_err(
body.stmts.last().span, "not all control paths return a value, maybe \
there is a semicolon on the last line of a this non-void function?");
Copy link
Member

Choose a reason for hiding this comment

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

There is ExprSemi which represents an expression with a semicolon, so I think the compiler can actually answer this question itself.

@lambda-fairy
Copy link
Contributor

I suggest putting the hint in a separate note, and putting the span right on the offending semicolon. Something like this:

note: consider removing this semicolon:
    x * 2;
         ^

@brson
Copy link
Contributor

brson commented Jan 12, 2014

I agree with @lfairy about how to present this. We could probably try even harder and check if the type of the final statement unifies with the function return type.

@fhahn
Copy link
Contributor Author

fhahn commented Jan 12, 2014

Thanks for the feedback!

I've added a note about the semicolon and check if the type of the expression of the last statement matches the return type. But I'm not sure if this is the best way to check if the types match.

let span_semicolon = Span {
lo: span_stmt.hi,
hi: span_stmt.hi,
expn_info: None
Copy link
Member

Choose a reason for hiding this comment

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

This seems a little fishy setting it to None, I think this is used for macros, so are you sure this doesn't mess up the note positioning?

Something like

macro_rules! foo ( () => (fn foo() -> int { 3i; }) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right. Can I pass the expn_info from the surrounding statement to the new span? I've pushed an updated commit with this change.

Copy link
Member

Choose a reason for hiding this comment

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

The best way to check is to add a test :)

(I think this would be tested by a macro like macro_rules! test ( () => { fn foo() -> int { 1i; } } ).)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this macro to the test, the note points to the semicolon in the macro.

bors added a commit that referenced this pull request Jan 13, 2014
…st-stmt, r=alexcrichton

This is a patch for #8005, thanks @lfairy for the hint.

It seems like `block.expr` is None, if the last line of a function has a semi colon (= it ends with a statement).

@kmcallister does this error message cover the intended use cases? 
I'm not sure about the message, the wording and the span could probably be improved.
@bors bors closed this Jan 13, 2014
@bors bors merged commit c74c854 into rust-lang:master Jan 13, 2014
@fhahn fhahn deleted the issue-8005-better-error-msg-semi-last-stmt branch January 13, 2014 20:29
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 28, 2023
…1995

Add colored help to be consistent with Cargo

On rust-lang/cargo#12578, a new colored help message format was introduced. This PR introduces the same styling from that `cargo help` message to our `cargo clippy --help` message.

More information is provided in the original PR, fixes rust-lang#11482. The perfect reviewing process would be that the reviewer installs this branch and checks for themselves, but here are some screenshots, there are some more screenshots in the original issue.

![image](https://github.com/rust-lang/rust-clippy/assets/73757586/0c4d1b6d-5aa2-4146-a401-9ae88f6dedf5)

(Note that the actual text may change in the actual commit, that screenshot is just to test the colors).
Also note that the `color-print` version **should always** be synced with Cargo's `color-print` version to avoid increasing build times in the rust-lang/rust repo.

changelog:Add colors to the `cargo clippy --help` output 🎉.
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.

6 participants