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

Extract refactor can produce unnecessary_parenthesis trigger #56944

Open
FMorschel opened this issue Oct 23, 2024 · 4 comments
Open

Extract refactor can produce unnecessary_parenthesis trigger #56944

FMorschel opened this issue Oct 23, 2024 · 4 comments
Labels
analyzer-refactoring area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P4 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@FMorschel
Copy link
Contributor

Repro:

class A {
  void a() {
    (1 + 2);
  }
}

Now select (1 + 2) and the option to Extract Method.

Output:

class A {
  void a() {
    b();
  }

  void b() => (1 + 2);   // <-- unnecessary_parenthesis
}
@dart-github-bot
Copy link
Collaborator

Summary: The Dart Extract Method refactoring incorrectly adds unnecessary parentheses when extracting an expression, resulting in a unnecessary_parenthesis lint warning.

@dart-github-bot dart-github-bot added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Oct 23, 2024
@bwilkerson
Copy link
Member

It's true that there are unnecessary parentheses in the extracted method, but there were unnecessary parentheses in the code before it was extracted. The fact that it wasn't reported is a bug in the lint (I opened dart-lang/linter#5112 to report it).

IMO, the extract method refactoring should not change the code being extracted. If there are diagnostics being reported against it, they should remain and users can fix the diagnostics separately.

@FMorschel
Copy link
Contributor Author

FMorschel commented Oct 23, 2024

I agree, but when it is a simple expression it could use the unparenthesized version. So it doesn't create new diagnostics. Say:

void foo() => (bar() + baz()) ? 0 : 1;

Even with dart-lang/linter#4996 merged, there would be no lint on (bar() + baz()) but it would trigger on the extracted code.

Sorry for the bad first example. This one is more similar to the actual code I found the behaviour.

@bwilkerson
Copy link
Member

Understood. And I'm not saying that the refactor can't introduce a lint violation. I just think it's better if the refactoring doesn't try to fix the code. But clearly not everyone agrees with me 😄 , and that's fine.

@a-siva a-siva removed the triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-refactoring area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P4 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

5 participants