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

A comment is removed after pub keyword in a struct. #2781

Closed
KamilaBorowska opened this issue Jun 10, 2018 · 12 comments · Fixed by #4239
Closed

A comment is removed after pub keyword in a struct. #2781

KamilaBorowska opened this issue Jun 10, 2018 · 12 comments · Fixed by #4239
Labels
a-comments bug Panic, non-idempotency, invalid code, etc.

Comments

@KamilaBorowska
Copy link

KamilaBorowska commented Jun 10, 2018

After reading http://journal.stuffwithstuff.com/2015/09/08/the-hardest-program-ive-ever-written/, I decided to put one of code examples provided here in Rust.

pub // Oh, crap. A line comment.
struct Foo {}

The original code used abstract, but there is no such keyword in Rust, I used pub instead. Rustfmt seeing that removed a comment. This is an edge case, I don't care what it formats down to, but I think comment should be preserved.

@anthonyclays
Copy link

This happens in a very large number of 'uncommon places for comments': http://play.rust-lang.org/?gist=f0e8521555680aed289378e1d8a7d849&version=stable&mode=debug

It'd be nice to at least print a warning when comments get thrown away. Ideally (IMO), rustfmt should move the comment to the first 'logical' place for comments strictly before its original location (but that's debatable: should these comments be moved before or after doc comments / atributes in the case of type / fn definitions?)

@topecongiro topecongiro added bug Panic, non-idempotency, invalid code, etc. a-comments labels Jun 11, 2018
@zacheryph
Copy link

Another example. Comment in the middle of struct/impl declaration.

Input File

struct X<T>
// where
//     T: Clone
{
    inner: String,
}

After rustfmt

struct X<T> {
    inner: String,
}

@hsandt
Copy link

hsandt commented Dec 24, 2018

Exactly. This was noted in #1911 and fixed for impl and trait only in #1925

We can assume the fix for struct would be similar as the cause should be similar, but not sure. I've noticed in particular that a comment just before a non-commented is not removed, as if protected. But I don't have an old rustfmt right now to check if this was also the case for impl and trait.

struct X<T>
// this will remain
where
    T: Clone
{
    inner: T
}

Should we split this issue in 2? (one for the pub part, one for the part before the brace)

I think a developer solving one issue will solve the other in the surrounding code at the same time, but I can't confirm until we find the exact cause.

Inspired by the fix for impl and trait, I searched for find_uncommented("{") and find_uncommented("where") in rustfmt/src/items.rs and found some occurrences, but not inside format_struct_struct itself.

Anyway, when solving this issue we should add comments at uncommon places in the unit tests to make sure they are preserved.

@scampi
Copy link
Contributor

scampi commented Apr 8, 2019

the problem in #2781 (comment) is fixed in #3495

@hellow554
Copy link
Contributor

hellow554 commented May 28, 2019

The rest isn't fixed.

Another example:

pub fn a(_: &/*'static*/()) {}

becomes

pub fn a(_: &()) {}

@topecongiro any reason why this (kind of critical) bug hasn't been fixed yet? It's almost open for a year now.

@scampi
Copy link
Contributor

scampi commented May 28, 2019

@hellow554 Thanks for giving an additional example where the comment is lost. That seems a strange place to put a comment though.
You are welcome to propose a PR to help resolve this issue faster too ;o)

@scampi
Copy link
Contributor

scampi commented May 28, 2019

Maybe we could try to create some kind of code generator that inserts comments at random places and see where comments are lost. The reason is that the span between some token is not taken into consideration and therefore you get lost comments. But I feel like that would make the code base harder to maintain. Or a re-design would be needed to facilitate how these missing spans are retained, at the moment it creeps in every rewrite method and the missing span needs to be carefully handled...

@paulstansifer
Copy link

I think I found another example (https://github.com/paulstansifer/unseemly/pull/17/files#diff-7328e869d09a114ad85e6ac59329ae4fL45)

    type Err : Debug /*Display*/ + Reifiable + Clone;

...goes to:

    type Err: Debug + Reifiable + Clone;

...I agree that I probably should not have put a comment there, though

@0xd34d10cc
Copy link

Yet another example:

use std::collections::{
  HashMap,
  // hash_map::Entry  
};

@pnkfelix
Copy link
Member

Yet another example:

pub type Export /*: Bound */ = S1;

This is one where I think a comment is justified; today we do not enforce such bounds (i.e. the compiler does not check that the right-hand side of a type definition actually conforms to such a bound), and the compiler actually errors if you try to include it (because we don't want people to mistakenly think that such bounds are enforced).

But the comment here serves a useful purpose (about the intent, that we intend for this type definition to conform to the bound).

pnkfelix added a commit to pnkfelix/cargo-bisect-rustc that referenced this issue May 15, 2020
Note: generates tests into fresh temp dir, which is deleted after testing is
done (regardless of success or failure). You can change the
`which_temp::WhichTempDir` type to revise this behavior.

This infrastructure includes two tests: `tests/cli.rs` and `tests/ice.rs`. Each
uses very different strategies for testing cargo-bisect-rustc.

  1. `tests/cli.rs` uses a so-called meta-build strategy: the test inspects the
     `rustc` version, then generates build files that will inject (or remove,
     e.g. when testing `--regress=success`) `#[rustc_error]` from the source
     code based on the `rustc` version. This way, we get the effect of an error
     that will come or go based solely on the `rustc` version, without any
     dependence on the actual behavior of `rustc` itself (beyond its version
     string format remaining parsable).

     * This strategy should remain usable for the foreseeable future, without
       any need for intervention from `cargo-bisect-rustc` developers.

  2. `tests/ice.rs` uses a totally different strategy: It embeds an ICE that we
     know originated at a certain version of the compiler. The ICE is embedded
     in the file `src/ice/included_main.rs`. The injection point associated with
     the ICE is encoded in the constant `INJECTION_COMMIT`.

     * Over time, since we only keep a certain number of builds associated with
       PR merge commits available to download, the embedded ICE, the
       `INJECTION_COMMIT` definition, and the search bounds defined in
       `INJECTION_LOWER_BOUND` and `INJECTION_UPPER_BOUND` will all have to be
       updated as soon as the commit for `INJECTION_COMMIT` is no longer
       available for download.

     * Thus, this testing strategy requires regular maintenance from the
       `cargo-bisect-rustc` developers. (However, it is more flexible than the
       meta-build strategy, in that you can embed arbitrary failures from the
       recent past using this approach. The meta-build approach can only embed
       things that can be expressed via features like `#[rustc_error]`, which
       cannot currently express ICE's.

----

Includes suggestions from code review

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

----

Includes some coments explaining the `WhichTempDir` type. (That type maybe
should just be an enum rather than a trait you implement... not sure why I made
it so general...)

----

Includes workaround for rustfmt issue.

Specifically, workaround rust-lang/rustfmt#3794 which
was causing CI's attempt to run `cargo fmt -- --check` to erroneously report:

```
% cargo fmt -- --check
error[E0583]: file not found for module `meta_build`
  --> /private/tmp/cbr/tests/cli.rs:11:20
   |
11 |     pub(crate) mod meta_build;
   |                    ^^^^^^^^^^
   |
   = help: name the file either meta_build.rs or meta_build/mod.rs inside the directory "/private/tmp/cbr/tests/cli/cli"

error[E0583]: file not found for module `command_invocation`
  --> /private/tmp/cbr/tests/ice.rs:34:20
   |
34 |     pub(crate) mod command_invocation;
   |                    ^^^^^^^^^^^^^^^^^^
   |
   = help: name the file either command_invocation.rs or command_invocation/mod.rs inside the directory "/private/tmp/cbr/tests/ice/common"

```

----

Includes fix for oversight in my cli test system: it needed to lookup target
binary, not our PATH.

(This functionality is also available via other means, such as
`$CARGO_BIN_EXE_<name>` and https://crates.io/crates/assert_cmd. I opted not to
use the builtin env variable because that is only available in very recent cargo
versions, and I would prefer our test suite to work peven on older versions of
cargo, if that is feasible...)

----

Includes applications of rustfmt suggestions, as well as an expansion of a
comment in a manner compatible with rustfmt.

(Namely, that replaced an inline comment which is erroneously deleted by rustfmt
(see rust-lang/rustfmt#2781 ) with an additional note
in the comment above the definition.)
ayazhafiz added a commit to ayazhafiz/rustfmt that referenced this issue Jun 6, 2020
I don't think this hurts to fix. rust-lang#2781, which surfaced this issue, has
a number of comments relating to similar but slightly different issues
(i.e. dropped comments in other places). I can mark rust-lang#2781 as closed and
then will open new issues for the comments that are not already resolved
or tracked.

Closes rust-lang#2781
calebcartwright pushed a commit that referenced this issue Jun 8, 2020
* Pick up comments between visibility modifier and item name

I don't think this hurts to fix. #2781, which surfaced this issue, has
a number of comments relating to similar but slightly different issues
(i.e. dropped comments in other places). I can mark #2781 as closed and
then will open new issues for the comments that are not already resolved
or tracked.

Closes #2781

* fixup! Pick up comments between visibility modifier and item name

* fixup! Pick up comments between visibility modifier and item name
@hellow554
Copy link
Contributor

@ayazhafiz I see, that you opened a seperate issue for the other problems, but I can't find that you covered my problem and it's still not fixed. Am I missing something?

@ayazhafiz
Copy link
Contributor

@hellow554 sorry about that, added back in #4245

calebcartwright pushed a commit to calebcartwright/rustfmt that referenced this issue Nov 29, 2020
…#4239)

* Pick up comments between visibility modifier and item name

I don't think this hurts to fix. rust-lang#2781, which surfaced this issue, has
a number of comments relating to similar but slightly different issues
(i.e. dropped comments in other places). I can mark rust-lang#2781 as closed and
then will open new issues for the comments that are not already resolved
or tracked.

Closes rust-lang#2781

* fixup! Pick up comments between visibility modifier and item name

* fixup! Pick up comments between visibility modifier and item name
calebcartwright pushed a commit that referenced this issue Nov 29, 2020
* Pick up comments between visibility modifier and item name

I don't think this hurts to fix. #2781, which surfaced this issue, has
a number of comments relating to similar but slightly different issues
(i.e. dropped comments in other places). I can mark #2781 as closed and
then will open new issues for the comments that are not already resolved
or tracked.

Closes #2781

* fixup! Pick up comments between visibility modifier and item name

* fixup! Pick up comments between visibility modifier and item name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-comments bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.