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

rustfmt ignore deprecated #493

Closed
austinabell opened this issue Jun 5, 2020 · 13 comments
Closed

rustfmt ignore deprecated #493

austinabell opened this issue Jun 5, 2020 · 13 comments

Comments

@austinabell
Copy link

Now in Rust 1.44, the

#![cfg_attr(rustfmt, rustfmt_skip)]

tags are now deprecated in favour of #![rustfmt::skip], I tried to replace the tags but for some reason it doesn't work with the autogen code, and I didn't dig deeper.

To replicate, I think it's just sufficient when updating to 1.44 and run a cargo fmt and it will format all autogen files in this repo or otherwise.

@Elrendio
Copy link
Contributor

Elrendio commented Jun 7, 2020

See rust-lang/rust#73078 to see why #![rustfmt::skip] doesn't work.

In short: it's a bug in rustfmt, should be fixed asap. I think it's best to migrate to #![rustfmt::skip] and wait for rustfmt to fix their bug rather than add #[rustfmt::skip] on all items on the generated code.

@hds
Copy link

hds commented Jun 23, 2020

The change in PR #495 breaks compilation of protobuf 2.15 (at least for me). I know the error is in rustfmt, but until it's fixed, perhaps a more conservative approach could be taken?

The PR mentions that this issue should be fixed in 1.44.1, but so far I see the same behaviour adding the two tags to the default example in the Rust playground:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=392753339f4652381c92297a5831c8a4

Would it be acceptable to revert to the original tag (as in 2.14)?

@stepancheg
Copy link
Owner

@hds can you please provide full example, which does not work?

AFAIU,

#![allow(unused_attributes)]
#![rustfmt::skip] 

does not work in root module, but works fine in other modules.

@stepancheg
Copy link
Owner

Just in case, this file is committed to the repostory, it compiles just fine.

@stepancheg
Copy link
Owner

... it is tested with Rust 1.44.1.

@dvc94ch
Copy link

dvc94ch commented Jun 30, 2020

Same here, it works locally, but breaks ci build. Very strange. If I could only reproduce this locally, it just works no matter what I try...

  |
9 | #![rustfmt::skip]
  | ^^^^^^^^^^^^^^^^^
  |
  = note: for more information, see https://github.com/rust-lang/rust/issues/54726
  = help: add `#![feature(custom_inner_attributes)]` to the crate attributes to enable

error[E0658]: non-builtin inner attributes are unstable
 --> /home/runner/.cargo/registry/src/github.lhy31512.workers.dev-1ecc6299db9ec823/protobuf-2.15.1/src/well_known_types/wrappers.rs:9:1
  |
9 | #![rustfmt::skip]
  | ^^^^^^^^^^^^^^^^^
  |
  = note: for more information, see https://github.com/rust-lang/rust/issues/54726
  = help: add `#![feature(custom_inner_attributes)]` to the crate attributes to enable

   Compiling rand_hc v0.1.0
   Compiling rand_isaac v0.1.1
   Compiling rand_xorshift v0.1.1
error: aborting due to 13 previous errors

For more information about this error, try `rustc --explain E0658`.
error: could not compile `protobuf`.
warning: build failed, waiting for other jobs to finish...
error: build failed
##[error]Process completed with exit code 101.
  Complete job

full log here: https://github.com/sunshine-protocol/sunshine-bounty/pull/106/checks?check_run_id=820586662

@stepancheg
Copy link
Owner

@dvc94ch your logs say all jobs install rust nightly-2020-02-05, which is quite old.

@stepancheg
Copy link
Owner

... even stable jobs seems to install the same nightly.

@stepancheg
Copy link
Owner

Yep,
Screenshot 2020-06-30 at 02 07 11

@dvc94ch
Copy link

dvc94ch commented Jun 30, 2020

Thanks, that was indeed the problem

@Xaeroxe
Copy link

Xaeroxe commented Jun 30, 2020

Protobuf 2.15 isn't compiling for rust 1.43 oddly. Protobuf 2.14 does compile using the same compiler.

@stepancheg
Copy link
Owner

stepancheg commented Jul 1, 2020

@Xaeroxe it is compatible with Rust mininum version 1.44.1, it is documented in changelog.

Unfortunately, rust-protobuf dependency on rustfmt macros and clippy make it very hard to make the library compatible with a wide range of rust compiler versions.

Life would be much easier if rustfmt supported comment markers, not just rust attributes. And @generated sounds like a great option for me. Add a like to this issue: rust-lang/rustfmt#3958.

@hds
Copy link

hds commented Jul 2, 2020

@hds can you please provide full example, which does not work?

AFAIU,

#![allow(unused_attributes)]
#![rustfmt::skip] 

does not work in root module, but works fine in other modules.

I retested locally ensuring 1.44.1 and you are right, it does compile fine outside of the root module.

stepancheg added a commit to stepancheg/rustfmt that referenced this issue Jun 20, 2021
`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* In particular, Phabricator tool which was spawned from Facebook
  internal tool [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker handling:
[one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable all-at-once at text level marker might be easier
to use and more reliable for generated code.
stepancheg added a commit to stepancheg/rustfmt that referenced this issue Jun 20, 2021
`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker handling:
[one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable all-at-once at text level marker might be easier
to use and more reliable for generated code.
stepancheg added a commit to stepancheg/rustfmt that referenced this issue Jun 20, 2021
`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker handling:
[one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable all-at-once at text level marker might be easier
to use and more reliable for generated code.
stepancheg added a commit to stepancheg/rustfmt that referenced this issue Jun 20, 2021
`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by a human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker handling:
[one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable all-at-once at text level marker might be easier
to use and more reliable for generated code.
stepancheg added a commit to stepancheg/rustfmt that referenced this issue Jun 20, 2021
`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by a human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker handling:
[one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable whole-file-at-once all-tools-at-once at text level
marker might be easier to use and more reliable for generated code.
stepancheg added a commit to stepancheg/rustfmt that referenced this issue Jun 20, 2021
`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by a human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker handling:
[one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable whole-file-at-once all-tools-at-once text level
marker might be easier to use and more reliable for generated code.
stepancheg added a commit to stepancheg/rustfmt that referenced this issue Jun 20, 2021
`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by a human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker
handling: [one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).
(Also, rust-protobuf started generating `@generated` marker
[6 years ago](https://git.io/JnV5h)).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable whole-file-at-once all-tools-at-once text level
marker might be easier to use and more reliable for generated code.
stepancheg pushed a commit to stepancheg/rustfmt that referenced this issue Jun 22, 2021
This is a copy of rust-lang#4296 with these changes:
* file is not reopened again to find if the file is generated
* first five lines are scanned for `@generated` marker instead of one
* no attempt is made to only search for marker in comments

`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by a human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker
handling: [one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).
(Also, rust-protobuf started generating `@generated` marker
[6 years ago](https://git.io/JnV5h)).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable whole-file-at-once all-tools-at-once text level
marker might be easier to use and more reliable for generated code.
stepancheg pushed a commit to stepancheg/rustfmt that referenced this issue Jun 22, 2021
This is a copy of rust-lang#4296 with these changes:
* file is not reopened again to find if the file is generated
* first five lines are scanned for `@generated` marker instead of one
* no attempt is made to only search for marker in comments

`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by a human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker
handling: [one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).
(Also, rust-protobuf started generating `@generated` marker
[6 years ago](https://git.io/JnV5h)).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable whole-file-at-once all-tools-at-once text level
marker might be easier to use and more reliable for generated code.
calebcartwright pushed a commit to rust-lang/rustfmt that referenced this issue Sep 8, 2021
This is a copy of #4296 with these changes:
* file is not reopened again to find if the file is generated
* first five lines are scanned for `@generated` marker instead of one
* no attempt is made to only search for marker in comments

`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by a human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker
handling: [one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).
(Also, rust-protobuf started generating `@generated` marker
[6 years ago](https://git.io/JnV5h)).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable whole-file-at-once all-tools-at-once text level
marker might be easier to use and more reliable for generated code.
calebcartwright pushed a commit to calebcartwright/rustfmt that referenced this issue Sep 15, 2021
This is a copy of rust-lang#4296 with these changes:
* file is not reopened again to find if the file is generated
* first five lines are scanned for `@generated` marker instead of one
* no attempt is made to only search for marker in comments

`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by a human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker
handling: [one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).
(Also, rust-protobuf started generating `@generated` marker
[6 years ago](https://git.io/JnV5h)).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable whole-file-at-once all-tools-at-once text level
marker might be easier to use and more reliable for generated code.
calebcartwright pushed a commit to rust-lang/rustfmt that referenced this issue Sep 15, 2021
This is a copy of #4296 with these changes:
* file is not reopened again to find if the file is generated
* first five lines are scanned for `@generated` marker instead of one
* no attempt is made to only search for marker in comments

`@generated` marker is used by certain tools to understand that the
file is generated, so it should be treated differently than a file
written by a human:
* linters should not be invoked on these files,
* diffs in these files are less important,
* and these files should not be reformatted.

This PR proposes builtin support for `@generated` marker.

I have not found a standard for a generated file marker, but:
* Facebook [uses `@generated` marker](https://tinyurl.com/fb-generated)
* Phabricator tool which was spawned from Facebook internal tool
  [also understands `@generated` marker](https://git.io/JnVHa)
* Cargo inserts `@generated` marker into [generated Cargo.lock files](https://git.io/JnVHP)

My personal story is that rust-protobuf project which I maintain
was broken twice because of incompatibilities/bugs in rustfmt marker
handling: [one](stepancheg/rust-protobuf#493),
[two](stepancheg/rust-protobuf#551).
(Also, rust-protobuf started generating `@generated` marker
[6 years ago](https://git.io/JnV5h)).

While rustfmt AST markers are useful to apply to a certain AST
elements, disable whole-file-at-once all-tools-at-once text level
marker might be easier to use and more reliable for generated code.
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 a pull request may close this issue.

6 participants