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

In Rust 2018, use ::foo::bar and use foo::bar are distinct #3104

Closed
nikomatsakis opened this issue Oct 15, 2018 · 20 comments
Closed

In Rust 2018, use ::foo::bar and use foo::bar are distinct #3104

nikomatsakis opened this issue Oct 15, 2018 · 20 comments
Labels
a-edition issues using an older `edition` value a-imports `use` syntax bug Panic, non-idempotency, invalid code, etc. p-high

Comments

@nikomatsakis
Copy link

I had a problem where rustfmt was reformatting use ::foo::bar to use foo::bar -- the problem is that, in Rust 2018, use ::foo::bar is sometimes needed to disambiguate:

See e.g.

mod rayon { }

use rayon::join;

fn main() {
    join(|| (), ||());
}

which -- in Rust 2018 -- fails with an ambiguity error (playground) if you have rayon in the prelude. Changing to use ::rayon::join disambiguates and is accepted (playground).

@nrc nrc added bug Panic, non-idempotency, invalid code, etc. editon support labels Oct 15, 2018
@nrc nrc added this to the 1.0 (edition rc1) milestone Oct 15, 2018
@topecongiro
Copy link
Contributor

You need to add edition = "2018" to rustfmt.toml. Or should rustfmt defaults to edition 2018?

@12101111
Copy link

Why don't rustfmt read edition = "2018" in Cargo.toml

@topecongiro
Copy link
Contributor

Why don't rustfmt read edition = "2018" in Cargo.toml

Good point. No technical reason, we did not consider it at the first place 😞
I think rustfmt should just overwrite the edition config option if the edition is set in Cargo.toml.

@nikomatsakis
Copy link
Author

I think rustfmt should definitely not require a rustfmt.toml to work. I didn't even know that was a thing.

(OK, I guess I did, but I had forgotten.)

@nikomatsakis
Copy link
Author

Also: I tend to run rustfmt via the emacs integration, which may or may not be passing --edition etc. I think we should probably be conservative if we don't have a "positive indication" of the edition.

@nikomatsakis
Copy link
Author

Basically I'm just saying: we should consider all the ways people run rustfmt :)

@eddyb
Copy link
Member

eddyb commented Oct 18, 2018

Does cargo fmt pass --edition to rustfmt? I think that's an important part of the workflow.
(but people running rustfmt manually seems a bit worrying wrt Cargo-centric bits)

@nrc nrc added the p-high label Oct 18, 2018
@nrc
Copy link
Member

nrc commented Oct 18, 2018

OK, we need to make a decision here, I guess. I think cargo-fmt should definitely just check the Cargo.toml, and then rustfmt.toml if there is no edition specified. In principle rustfmt either as a binary or a library should be able to run independent of Cargo, so we can't rely on Cargo.toml. However, should we look for Cargo.toml if there is one? I think it is right to default to 2015, since that is what Rust does too. However, I'm not sure if that leaves us in a good place for people running rustfmt via an editor or the RLS (I think the RLS could set the edition, if it doesn't already. I assume editors can't do that?).

cc @topecongiro @scampi @YaLTeR @otavio

@otavio
Copy link
Contributor

otavio commented Oct 18, 2018

My understanding is that when Rust 2018 is released, cargo will default to it ... or am I mistaken?

If an editor would call rustfmt on save, it is the editor's responsibility to properly pass the --edition accordingly. If using RLS it is alleviated as the language server must be aware of the edition in use.

@otavio
Copy link
Contributor

otavio commented Oct 18, 2018

@nikomatsakis

Also: I tend to run rustfmt via the emacs integration, which may or may not be passing --edition etc. I think we should probably be conservative if we don't have a "positive indication" of the edition.

I do as well. I use lsp-mode and in this case, it relies on RLS to know about the edition in use, I assume. If the editor does it by itself, it is on its own to "discover" it and Cargo.toml seems to be the first logical place to look I guess.

@nikomatsakis I assume that for "conservative" you mean to follow the default as Rust compiler uses. Am I right?

@YaLTeR
Copy link
Contributor

YaLTeR commented Oct 19, 2018

I agree that rustfmt should check Cargo.toml for edition (since it already checks rustfmt.toml, I don't see anything wrong with checking another relevant file).

As for the default behavior (no Cargo.toml and not specified in rustfmt.toml), I think defaulting to the 2015 edition is the right choice in order to stay compatible with the current default behavior. That is, if someone's currently formatting free-standing .rs files with default settings, they won't get sudden breakages.

I never really ran rustfmt from editors by itself, only through the RLS (which should be able to report the correct edition), doesn't rustfmt read rustfmt.toml when ran through editors? If not then it should probably be editors' responsibility to set the correct edition (so rustfmt should support a flag like that).

@topecongiro
Copy link
Contributor

topecongiro commented Oct 19, 2018

Todos:

  • rustfmt: add --edition command line option
  • cargo-fmt: read Cargo.toml and pass --edition flag to rustfmt if necessary

I think that when multiple editions are specified, the precedence should be

  1. edition passed as command line argument
  2. edition specified in Cargo.toml
  3. edition specified in rustfmt.toml
  4. the default edition (2015 for now)

@YaLTeR
Copy link
Contributor

YaLTeR commented Oct 19, 2018

  1. edition specified in Cargo.toml
  2. edition specified in rustfmt.toml

Maybe also show a warning if it comes down to these two and they explicitly specify different editions? Like "warning: different editions specified in Cargo.toml and rustfmt.toml; using the X edition".

@nrc
Copy link
Member

nrc commented Oct 20, 2018

  • check that there is API for passing the edition in the config and that the RLS is doing this correctly (based on Cargo.toml).

@otavio
Copy link
Contributor

otavio commented Oct 22, 2018

I'd like to inform I am looking at this issue, so there is no duplicated work.

@nrc
Copy link
Member

nrc commented Oct 22, 2018

  • - stabilise the edition option

@nrc nrc closed this as completed in de0b661 Oct 25, 2018
@nrc nrc reopened this Oct 25, 2018
@otavio
Copy link
Contributor

otavio commented Oct 25, 2018

What is missing on this one?

@otavio
Copy link
Contributor

otavio commented Oct 25, 2018

I think that the API is already covered by 2eab971. The stabilization is handled by #3134.

@nrc
Copy link
Member

nrc commented Nov 1, 2018

The missing piece here is to prove out the API by having the RLS use it. Once we have that, then we should release Rustfmt and the RLS.

@nrc
Copy link
Member

nrc commented Nov 14, 2018

RLS part: rust-lang/rls#1119

@nrc nrc closed this as completed Nov 15, 2018
@ytmimi ytmimi added a-imports `use` syntax a-edition issues using an older `edition` value labels Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-edition issues using an older `edition` value a-imports `use` syntax bug Panic, non-idempotency, invalid code, etc. p-high
Projects
None yet
Development

No branches or pull requests

8 participants