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

Formatter adding -- with multi-import statements #684

Closed
adlerjohn opened this issue Jan 22, 2022 · 2 comments · Fixed by #773
Closed

Formatter adding -- with multi-import statements #684

adlerjohn opened this issue Jan 22, 2022 · 2 comments · Fixed by #773
Assignees
Labels
bug Something isn't working formatter good first issue Good for newcomers

Comments

@adlerjohn
Copy link
Contributor

adlerjohn commented Jan 22, 2022

Minimal repro:

script;
use std::chain::{panic, panic};

fn main() {
}

Output of forc fmt --check:

script;
-use std::chain::{panic, panic};
+--use std::chain::{panic,panic};

fn main() {
}
@adlerjohn adlerjohn added bug Something isn't working formatter labels Jan 22, 2022
@adlerjohn adlerjohn moved this to Todo in Fuel Network Jan 22, 2022
@adlerjohn adlerjohn added the good first issue Good for newcomers label Jan 22, 2022
@iqdecay iqdecay removed their assignment Jan 22, 2022
@iqdecay iqdecay self-assigned this Jan 30, 2022
@iqdecay iqdecay moved this from Todo to In Progress in Fuel Network Jan 31, 2022
@iqdecay
Copy link
Contributor

iqdecay commented Jan 31, 2022

After spending some time digging around on the formatter, it turns out that this issue probably comes from the fact that a single use std::chain::{panic,log_u8} statement creates 2 root nodes, which from my understanding should not be happening. Tagging this as a compiler issue (because it comes from the parsing).
The way I diagnosed this:

  • If you parse the AST as a string you see the lines as they should appear (a leading --- is added to the use statement to signify that they are already formatted).
  • The traverse_for_changes is the thing that creates an extra Change element, which make it so that there are 2 sets of change for the same use statement -> this duplication only happens for use statements importing 2 elements from the same crate/module.
  • This extra change actually comes from the fact that the AST has 2 root nodes per use statement

In my opinion, this is not right.

@iqdecay iqdecay added compiler General compiler. Should eventually become more specific as the issue is triaged and removed formatter labels Jan 31, 2022
@iqdecay iqdecay changed the title formatter adding -- with multi-imports A single use statement with 2 imports creates 2 root nodes in the AST Jan 31, 2022
@iqdecay iqdecay changed the title A single use statement with 2 imports creates 2 root nodes in the AST Formatter adding -- with multi-import statements Feb 1, 2022
@iqdecay
Copy link
Contributor

iqdecay commented Feb 1, 2022

As explained by @mohammadfawaz:

the compiler treats a use statement with with multiple items:
use a::{b, c, d};
the same as multiple use statements, each importing a single item:
use a::b;
use a::c;
use a::d;

We will have to find a way to fix the formatter without changing the AST generation.

@iqdecay iqdecay added enhancement New feature or request formatter and removed compiler General compiler. Should eventually become more specific as the issue is triaged enhancement New feature or request labels Feb 1, 2022
Repository owner moved this from In Progress to Done in Fuel Network Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working formatter good first issue Good for newcomers
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants