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

Add compiler warning for double semicolon #60876

Closed
hellow554 opened this issue May 16, 2019 · 18 comments · Fixed by #62984
Closed

Add compiler warning for double semicolon #60876

hellow554 opened this issue May 16, 2019 · 18 comments · Fixed by #62984
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hellow554
Copy link
Contributor

This could should emit an error:

pub fn foo() {
    return;;
}

I think something similar to this would be a good one:

warning: redundant `;` detected
 --> src/main.rs:X:XX
  |
LL|         return;;
  |                ^
  |                |
  |                help: remove this `;` (this should be a suggestion so rustfix can fix it automatically)
@hellow554
Copy link
Contributor Author

@rustbot modify labels: A-diagnostics C-enhancement E-easy

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels May 16, 2019
@hbina
Copy link
Contributor

hbina commented May 16, 2019

Hi, I would like to resolve this issue. Do you have any lead/pointers that I can use to start?

@hellow554
Copy link
Contributor Author

hellow554 commented May 16, 2019

I'm not entirely sure, I only started recently with hacking the compiler. I think this can be easily done by looking at

';' => {
self.bump();
Ok(token::Semi)
}

You could do something similar to this:

';' => { 
    self.bump(); 
    if self.ch_is(';') {
        self.bump();
        let mut err = self.sess.span_diagnostic.struct_span_warn(self.sp, "redundant `;` detected");
        ...
        err.emit();
    }
    Ok(token::Semi) 
} 

Of course you should also detect more than one, e.g. return ;;;. Also make sure it works for let a = "";;;;;;.

@hbina
Copy link
Contributor

hbina commented May 16, 2019

is it safe to just perform a while loop?
i am not sure how reliable this solution could be.
Especially if there's a space like ; ;

Regardless how to say that this ;;;;;; entirely line is faulty, not just the first character?

@hellow554
Copy link
Contributor Author

hellow554 commented May 16, 2019

is it safe to just perform a while loop?

Go and try it.

Especially if there's a space like ; ;

That could be a problem indeed, because spaces are treated seperatly.

Regardless how to say that this ;;;;;; entirely line is faulty, not just the first character?

You mean something like

fn foo() {
    ;;;;;;;;;;;;;;
}

? I think this should be a different warning. Something like "sole `;` detected", because it does not end any statement at all and should have no right to exist.

I think a person which is more privileged than me should say a word or too as well :) I'm not a rust lang member

@hellow554
Copy link
Contributor Author

hellow554 commented May 16, 2019

Instead of using the lexer level, you could also use the token level, because the lexer converts the bytestream to tokens (as you can see with Ok(token::Semi)) and if you detect two Semi tokens next to each other, you could emit a warning there. Should be also possible. But I'm not entirely sure where to do that.

@Centril
Copy link
Contributor

Centril commented May 16, 2019

cc @petrochenkov

This should be done as new a lint pass (redundant_semicolon) or included in an existing one.

@Centril Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. and removed A-diagnostics Area: Messages for errors, warnings, and lints labels May 16, 2019
@hbina
Copy link
Contributor

hbina commented May 19, 2019

@Centril how does one go about doing that?

@scottmcm
Copy link
Member

A rustfix-able lint for this (along the line of unused_parens, etc) sounds great. (Not an error, of course.)

@Centril
Copy link
Contributor

Centril commented May 23, 2019

We discussed this in the language team meeting today. People felt generally positive about such a lint. However, we would like to not lint on return/break/continue; and return/break x; specifically.

@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels May 23, 2019
@varkor
Copy link
Member

varkor commented May 23, 2019

@hbina: I can give more detailed mentoring advice later, but for now: take a look at an existing lint (e.g. UNUSED_MUST_USE). If you search the compiler source for that, you'll find a bunch of places you need to modify to add a new lint. You'll want to add a new lint type (e.g. REDUNDANT_SEMICOLON) and add a new pass that checks for an empty statement.

There's a slight problem at the moment, in that semicolons are stripped while parsing, and we'd need to preserve this information until we get to the point of linting. So you'll also need to modify the parser to not ignore semicolons (and store them in the statements). (Sorry, this is a bit rushed.)

Feel free to message me on Discord or Zulip for more help, or ask on one of the compiler channels there.

@varkor varkor added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label May 23, 2019
@gautammohan
Copy link

Is this issue currently being worked on? I'm interested in contributing.

@nathanwhit
Copy link
Member

I've done some work on this, and have a working draft ready for review. The PR is at #62984, feedback is appreciated!

@ghost
Copy link

ghost commented Jul 25, 2019

However, we would like to not lint on return/break/continue; and return/break x; specifically.

@Centril Would you mind giving a short explanation? I am curious 😃

@Centril
Copy link
Contributor

Centril commented Jul 25, 2019

@BO41 What sort of explanation? We felt positive about this lint since it removes something which sometimes happens and which is a mistake, iirc. We don't want to lint on return x; since that is commonly accepted as OK/good style.

@iluuu1994
Copy link
Contributor

@Centril I think this issue is only talking about duplicate semicolons. return x;; should warn but return x; shouldn't. This also shouldn't:

fn fn_returning_void() {
    other_fn_returning_void();
}

@scottmcm
Copy link
Member

@BO41 well, rustfmt puts semicolons after return expressions, so it wouldn't be nice to tell people to remove something only for rustfmt to put it back again 🙂

@ghost
Copy link

ghost commented Jul 28, 2019

Thanks @Centril and @scottmcm for the explanation.
I think we got a bit confused, as @iluuu1994 described.

This issue is about multiple semicolons. Not a single after return.
So I think

we would like to not lint on return/break/continue; and return/break x; specifically.

is not really what this is about. As return;; is wrong. And continue;;;;;;;;; too.
But I agree suggesting from break; to break is bad.

Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
Add lint for excess trailing semicolons

Closes rust-lang#60876.
A caveat (not necessarily a negative, but something to consider) with this implementation is that excess semicolons after return/continue/break now also cause an 'unreachable statement' warning.

For the following example:
```
fn main() {
    extra_semis();
}
fn extra_semis() -> i32 {
    let mut sum = 0;;;
    for i in 0..10 {
        if i == 5 {
            continue;;
        } else if i == 9 {
            break;;
        } else {
            sum += i;;
        }
    }
    return sum;;
}
```
The output is:
```
warning: unnecessary trailing semicolons
 --> src/main.rs:5:21
  |
5 |     let mut sum = 0;;;
  |                     ^^ help: remove these semicolons
  |
  = note: `#[warn(redundant_semicolon)]` on by default

warning: unnecessary trailing semicolon
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:12:22
   |
12 |             sum += i;;
   |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^ help: remove this semicolon

warning: unreachable statement
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^
  |
  = note: `#[warn(unreachable_code)]` on by default

warning: unreachable statement
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^

warning: unreachable statement
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^

```
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
Add lint for excess trailing semicolons

Closes rust-lang#60876.
A caveat (not necessarily a negative, but something to consider) with this implementation is that excess semicolons after return/continue/break now also cause an 'unreachable statement' warning.

For the following example:
```
fn main() {
    extra_semis();
}
fn extra_semis() -> i32 {
    let mut sum = 0;;;
    for i in 0..10 {
        if i == 5 {
            continue;;
        } else if i == 9 {
            break;;
        } else {
            sum += i;;
        }
    }
    return sum;;
}
```
The output is:
```
warning: unnecessary trailing semicolons
 --> src/main.rs:5:21
  |
5 |     let mut sum = 0;;;
  |                     ^^ help: remove these semicolons
  |
  = note: `#[warn(redundant_semicolon)]` on by default

warning: unnecessary trailing semicolon
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:12:22
   |
12 |             sum += i;;
   |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^ help: remove this semicolon

warning: unreachable statement
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^
  |
  = note: `#[warn(unreachable_code)]` on by default

warning: unreachable statement
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^

warning: unreachable statement
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^

```
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
Add lint for excess trailing semicolons

Closes rust-lang#60876.
A caveat (not necessarily a negative, but something to consider) with this implementation is that excess semicolons after return/continue/break now also cause an 'unreachable statement' warning.

For the following example:
```
fn main() {
    extra_semis();
}
fn extra_semis() -> i32 {
    let mut sum = 0;;;
    for i in 0..10 {
        if i == 5 {
            continue;;
        } else if i == 9 {
            break;;
        } else {
            sum += i;;
        }
    }
    return sum;;
}
```
The output is:
```
warning: unnecessary trailing semicolons
 --> src/main.rs:5:21
  |
5 |     let mut sum = 0;;;
  |                     ^^ help: remove these semicolons
  |
  = note: `#[warn(redundant_semicolon)]` on by default

warning: unnecessary trailing semicolon
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:12:22
   |
12 |             sum += i;;
   |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^ help: remove this semicolon

warning: unreachable statement
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^
  |
  = note: `#[warn(unreachable_code)]` on by default

warning: unreachable statement
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^

warning: unreachable statement
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^

```
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
Add lint for excess trailing semicolons

Closes rust-lang#60876.
A caveat (not necessarily a negative, but something to consider) with this implementation is that excess semicolons after return/continue/break now also cause an 'unreachable statement' warning.

For the following example:
```
fn main() {
    extra_semis();
}
fn extra_semis() -> i32 {
    let mut sum = 0;;;
    for i in 0..10 {
        if i == 5 {
            continue;;
        } else if i == 9 {
            break;;
        } else {
            sum += i;;
        }
    }
    return sum;;
}
```
The output is:
```
warning: unnecessary trailing semicolons
 --> src/main.rs:5:21
  |
5 |     let mut sum = 0;;;
  |                     ^^ help: remove these semicolons
  |
  = note: `#[warn(redundant_semicolon)]` on by default

warning: unnecessary trailing semicolon
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:12:22
   |
12 |             sum += i;;
   |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^ help: remove this semicolon

warning: unreachable statement
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^
  |
  = note: `#[warn(unreachable_code)]` on by default

warning: unreachable statement
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^

warning: unreachable statement
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^

```
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
Add lint for excess trailing semicolons

Closes rust-lang#60876.
A caveat (not necessarily a negative, but something to consider) with this implementation is that excess semicolons after return/continue/break now also cause an 'unreachable statement' warning.

For the following example:
```
fn main() {
    extra_semis();
}
fn extra_semis() -> i32 {
    let mut sum = 0;;;
    for i in 0..10 {
        if i == 5 {
            continue;;
        } else if i == 9 {
            break;;
        } else {
            sum += i;;
        }
    }
    return sum;;
}
```
The output is:
```
warning: unnecessary trailing semicolons
 --> src/main.rs:5:21
  |
5 |     let mut sum = 0;;;
  |                     ^^ help: remove these semicolons
  |
  = note: `#[warn(redundant_semicolon)]` on by default

warning: unnecessary trailing semicolon
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:12:22
   |
12 |             sum += i;;
   |                      ^ help: remove this semicolon

warning: unnecessary trailing semicolon
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^ help: remove this semicolon

warning: unreachable statement
 --> src/main.rs:8:22
  |
8 |             continue;;
  |                      ^
  |
  = note: `#[warn(unreachable_code)]` on by default

warning: unreachable statement
  --> src/main.rs:10:19
   |
10 |             break;;
   |                   ^

warning: unreachable statement
  --> src/main.rs:15:16
   |
15 |     return sum;;
   |                ^

```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants