-
Notifications
You must be signed in to change notification settings - Fork 898
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
Comments
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?) |
Another example. Comment in the middle of struct/impl declaration. Input Filestruct X<T>
// where
// T: Clone
{
inner: String,
} After rustfmtstruct X<T> {
inner: String,
} |
Exactly. This was noted in #1911 and fixed for 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
Should we split this issue in 2? (one for the 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 Anyway, when solving this issue we should add comments at uncommon places in the unit tests to make sure they are preserved. |
the problem in #2781 (comment) is fixed in #3495 |
The rest isn't fixed. Another example:
becomes
@topecongiro any reason why this (kind of critical) bug hasn't been fixed yet? It's almost open for a year now. |
@hellow554 Thanks for giving an additional example where the comment is lost. That seems a strange place to put a comment though. |
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 |
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 |
Yet another example: use std::collections::{
HashMap,
// hash_map::Entry
}; |
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 But the comment here serves a useful purpose (about the intent, that we intend for this type definition to conform to the bound). |
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 <[email protected]> ---- 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.)
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
* 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
@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? |
@hellow554 sorry about that, added back in #4245 |
…#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
* 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
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.
The original code used
abstract
, but there is no such keyword in Rust, I usedpub
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.The text was updated successfully, but these errors were encountered: