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

Discussion about automated fixes #35550

Open
bwilkerson opened this issue Jan 3, 2019 · 5 comments
Open

Discussion about automated fixes #35550

bwilkerson opened this issue Jan 3, 2019 · 5 comments
Labels
analyzer-bulk-fix area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-question A question about expected behavior or functionality

Comments

@bwilkerson
Copy link
Member

@munificent @danrubel

I think it's great that we added some updating support to dartfmt to help our users. However, we have now started working on dartfix as a tool for automating code changes. I think it's worth having a discussion about what the best path forward is from our users perspective.

Should we have a single tool that automates code changes? If so, should that be dartfmt or dartfix, or should we have two tools that apply fixes, with dartfmt performing syntactic fixes while dartfix performs semantic fixes?

Personally, I think all of the fixes should be implemented in one tool, but I'm interested to know other people's thoughts.

(This might impact dart-lang/dart_style#753 and #57860.)

@bwilkerson bwilkerson added the type-question A question about expected behavior or functionality label Jan 3, 2019
@munificent
Copy link
Member

munificent commented Jan 3, 2019

I look at dartfmt and dartfix as being mostly complementary tools with also probably some overlap. As long as we don't waste a ton of time re-implementing the same fixes twice, I don't think the overlap is hugely problematic. I care more that users get shipped the fix at all than I care which tools ships it.

I also think there are many non-overlapping cases for the tools:

  • Any fix that requires static analysis isn't and will probably never be a good fit for dartfmt. We don't want dartfmt to do analysis because it's very slow, and we want to be able to format files that contain type errors or aren't part of a well-structured program. Often, formatting is the first step towards fixing up a broken program, so it needs to be accessible even to code containing static errors.

    (This does raise an interesting question around the difference between "static", "compile-time", "type", and "syntax" errors. In practice, that seems to mostly be an academic question.)

  • Any fix that requires human interaction "Should I change X to Y or Z? Please choose." isn't a good fit for dartfmt. Dartfmt is a batch mode, non-interactive tool.

  • Fixes that are purely syntactic are a good fit for dartfmt. Dartfmt can probably apply them faster than dartfix can, and can apply them even when the code can't be analyzed. Using the formatter sends a clearer signal to users that the change has no semantic affect. Syntax changes typically affect the formatting, so applying them in dartfmt lets you get output that both contains the change and is still correctly formatted.

Pragmatically, people are using and like the fixes dartfmt supports. It's a well-known tool that users trust and know how to run. I don't think it will make users happy to remove these features now in the name of abstract ideals of orthogonality, so we're stuck with them.

@bwilkerson
Copy link
Member Author

I understand your arguments, but I still think it could be confusing to new users who have no opinion where this feature should live. Having a single feature split between two tools just seems wrong to me.

Perhaps a better time to tackle this would be if we move to a single command with sub-commands, such as dart format and dart fix.

@mit-mit @kevmoo Curious whether you have opinions.

@vsmenon vsmenon added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Jan 4, 2019
@stereotype441 stereotype441 added the P2 A bug or feature request we're likely to work on label Jan 4, 2019
@munificent
Copy link
Member

Will dartfix support fixes that can be applied to code containing static errors? If so, and the performance is good enough, I'd be fine with removing the fixes from dartfmt and putting them only in dartfix if it seems like our users are OK with it.

@bwilkerson
Copy link
Member Author

Yes. At least one of the fixes it already supports depends on looking for a semantic error in order to figure out which code to fix.

@kwalrath
Copy link
Contributor

kwalrath commented Jan 5, 2019

It'll be easier for me to explain if there's only one tool that makes these kinds of fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-bulk-fix area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-question A question about expected behavior or functionality
Projects
None yet
Development

No branches or pull requests

6 participants