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 silently deletes the word const when it appears before impl while using the experimental const_trait_impl feature #4084

Closed
slightlyoutofphase opened this issue Mar 14, 2020 · 5 comments · Fixed by #4215
Labels
1x-backport:completed bug Panic, non-idempotency, invalid code, etc.
Milestone

Comments

@slightlyoutofphase
Copy link

slightlyoutofphase commented Mar 14, 2020

For example, this is valid / working code on nightly:

#![allow(incomplete_features)]
#![feature(const_trait_impl)]

#[derive(Debug)]
struct Struct {
    f: f32,
}

impl const std::default::Default for Struct {
    #[inline]
    fn default() -> Self {
        Self { f: 12.5 }
    }
}

const S: Struct = Default::default();

fn main() {
    println!("{:?}", S);
}

However, if you run rustfmt, it will not error but simply quietly delete the word const between impl and std::default. This isn't great when using rustfmt at the crate level as it will of course delete all instances of const for trait impls, most likely breaking the build (and causing you to have to add them all back in).

The deletion seems like odd behavior in general, IMO: I'd expect rustfmt to give a hard error on syntax it thinks is legitimately invalid (as it usually does), and just do its best to normally format anything else.

@calebcartwright
Copy link
Member

rustfmt uses the internal rustc parser (via the rustc-ap-* crates) to generate the AST for the input file(s), and then uses that AST to apply formatting. As such the version of the parser and corresponding AST that rustfmt sees lag a little behind the latest nightly compiler, and it takes a bit for rustfmt to support formatting new syntax.

The current version of the rustc parser that rustfmt is using predates rust-lang/rust#68847 which, I believe, is what added support for const_trait_impl, so that syntax isn't yet supported within rustfmt.

Regarding the absence of an error, if rustfmt's version of the rustc parser encounters any errors parsing the input files then those parser errors will be surfaced, and rustfmt will exit with an error code. You aren't seeing any errors because the rustc parser isn't surfacing any, but const isn't in the generated AST which is why it's not in the output emitted by rustfmt.

Support for const_trait_impl will be added to rustfmt when we update the version of the rustc parser used within rustfmt. For full transparency however, it's probably going to take a while because there's been a lot of upstream changes within the parser that will require a lot of work within rustfmt to make that upgrade.

For the time being you may want to consider leveraging the #[rustfmt::skip] attribute to prevent rustfmt from dropping const

@slightlyoutofphase
Copy link
Author

Thanks for the thorough explanation. That it's more a case of const just not existing in the AST than it being actively removed makes a lot more sense.

I did actually go ahead and add #[rustfmt::skip] shortly after posting this, which seems to work fine for now.

@topecongiro topecongiro added the bug Panic, non-idempotency, invalid code, etc. label Mar 31, 2020
ayazhafiz added a commit to ayazhafiz/rustfmt that referenced this issue May 30, 2020
topecongiro pushed a commit that referenced this issue May 31, 2020
@BellaCoola
Copy link

Hello, I am sorry to post here again, since it seems that this issue had been already closed, but in my experience rustfmt still silently deletes the const keyword when it appears before the impl keyword. Please, is there something else I should do (use some configuration flag) to make this work?

Tested with rustfmt 1.4.30-nightly.

Thank you very much and have a nice day!

@calebcartwright calebcartwright added the 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release label Dec 26, 2020
@calebcartwright
Copy link
Member

thanks for reaching out @BellaCoola. the issue was closed because it's been resolved in source, but hasn't been backported to the 1.x version of the codebase that's distributed through rustup. I've updated the labels on this issue to reflect the backport status and will look into getting this backported in the next release

@BellaCoola
Copy link

BellaCoola commented Dec 28, 2020

Thank you very much! Looking forward to try it :) And also thank you for all your work on rustfmt, it is a great piece of software!

calebcartwright pushed a commit to calebcartwright/rustfmt that referenced this issue Jan 27, 2021
@calebcartwright calebcartwright added 1x-backport:completed and removed 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release labels Jan 27, 2021
@calebcartwright calebcartwright added this to the 1.4.33 milestone Jan 27, 2021
calebcartwright pushed a commit that referenced this issue Jan 28, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 30, 2021
…anxiyn

update rustfmt to v1.4.34

Short summary: Various formatting fixes (several const generic related) and introduction of `imports_granularity` config option

Long summary copied from changelog:

#### Changed
- `merge_imports` configuration has been deprecated in favor of the new `imports_granularity` option. Any existing usage of `merge_imports` will be automatically mapped to the corresponding value on `imports_granularity` with a warning message printed to encourage users to update their config files.

#### Added
- New `imports_granularity` option has been added which succeeds `merge_imports`. This new option supports several additional variants which allow users to merge imports at different levels (crate or module), and even flatten imports to have a single use statement per item. ([PR rust-lang/rustfmt#4634](rust-lang/rustfmt#4634), [PR rust-lang/rustfmt#4639](rust-lang/rustfmt#4639))

See the section on the configuration site for more information
https://rust-lang.github.io/rustfmt/?version=v1.4.33&search=#imports_granularity

#### Fixed
- Fix erroneous removal of `const` keyword on const trait impl ([rust-lang/rustfmt#4084](rust-lang/rustfmt#4084))
- Fix incorrect span usage wit const generics in supertraits ([rust-lang/rustfmt#4204](rust-lang/rustfmt#4204))
- Use correct span for const generic params ([rust-lang/rustfmt#4263](rust-lang/rustfmt#4263))
- Correct span on const generics to include type bounds ([rust-lang/rustfmt#4310](rust-lang/rustfmt#4310))
- Idempotence issue on blocks containing only empty statements ([rust-lang/rustfmt#4627](rust-lang/rustfmt#4627) and [rust-lang#3868](rust-lang/rustfmt#3868))
- Fix issue with semicolon placement on required functions that have a trailing comment that ends in a line-style comment before the semicolon ([rust-lang/rustfmt#4646](rust-lang/rustfmt#4646))
- Avoid shared interned cfg_if symbol since rustfmt can re-initialize the rustc_ast globals on multiple inputs ([rust-lang/rustfmt#4656](rust-lang/rustfmt#4656))
- Don't insert trailing comma on (base-less) rest in struct literals within macros ([rust-lang/rustfmt#4675](rust-lang/rustfmt#4675))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1x-backport:completed bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants