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

Implement proof of concept for formatting specific line ranges #959

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@kamalmarhubi
Contributor

kamalmarhubi commented Apr 22, 2016

This series of commits adds a proof of concept of restricting formatting to specific line ranges, controlled by the --experimental-file-lines option. It includes a rough demonstration of formatting for statements. Later commits will extend this to more AST node type and refine the formatting more, and remove the experimental- prefix on the option name.

The goal is to make incremental formatting possible, and allow improving the formatting of a codebase one change at a time. This is much better than requiring that an entire project be formatted in one go in order to get the benefits of rustfmt. In later commits, we can add a tool to format a project based on a diff which will allow adding rustfmt to, eg, a git pre-commit hook without risk of introducing unrelated changes across a codebase.

There is some heuristic work needed to follow on from this to enable incremental formatting of all AST node types. For some, we will want to format when a change overlaps the AST node; for others, only when the change fully contains the AST node. As an example, we won't want to reformat an entire function because one line in it changed, but we would want to reformat an entire function signature if part of it changed. This will be done in later commits.

Outline of the commits in the series:

  • commits 1-2 improve debuggability
  • commits 3-4 are refactorings to config to make way for the config field that tracks lines to be formatted
  • commit 5 moves some codemap related utilities to their own module
  • commit 6 adds utilities for working with line ranges, and looking up line ranges in the codemap
  • commit 7 adds a field to the Config struct to track lines to be formatted
  • commit 8 adds a proof of concept for formatting statements within the specified line ranges
  • commit 9 adds a command line option for specifying line ranges to be formatted

Refs #434

kamalmarhubi added some commits Apr 21, 2016

config: Allow excluding config settings from --config-help output
This is preparation for restricting formatting to specific line ranges.
That option does not make sense for use in the config file, and so
should not appear in config documentation.

Refs #434
visitor: Handle specified line ranges in visit_stmt
This commit adds a very rough implementation of handling the specified
line ranges in `config.file_lines_map` for statements. It reformats a
statement if its span is fully contained in the set of lines specified
for the file.

The implementation here is intended as a proof of concept, and
demonstration that the machinery added in the preceding commits is
functional. A final implementation would likely hook in via the
`Rewrite` trait.

Refs #434
config: Move override_value() parsing method to ConfigType trait
This commit moves `Config::override_value()` parsing from `FromStr` to
our own trait. This allows more control over parsing, and in particular
allows us to define parsing on types from `std` such as `Option`. This
will be used to handle restricting formatting to specific line ranges.

Refs #434
config: Add field for restricting formating to specific lines
This commit adds a `file_lines_map` field to `Config`. This is an
optional field that when present stores a map from file names to sets of
lines to format. The code to use this field will come in a later commit.

This includes code to parse a file name / line set specification that is
used for `Config::override_value()` and will be used to parse the
command line option.

Adding `nom` as a dependency for parsing increases the incremental
rustfmt build time by 0.96s on a machine where it takes ~23s to build
rustfmt.

Refs #434
utils: Move codemap related utilities to a dedicated module
This commit adds a `codemap` module, and moves the `CodemapSpanUtils`
added in #857 to it. This is preparation for adding more `Codemap`
specific utilities.

Refs #434
codemap: Add utilities for dealing with line ranges
This commit adds `LineRange` and `LineSet` types, and assiciated
utilities. They will be used to restrict formatting to specific lines.

Refs #434
rustfmt: Add option to specify line ranges for formatting
This commit adds the `--experimental-file-lines` option to rustfmt. This
allows specifying line ranges to format from the command line. We will
remove the `experimental-` prefix once we have covered all AST elements,
and are satisfied with the functionality.

Refs #434
@kamalmarhubi

This comment has been minimized.

Show comment
Hide comment
@kamalmarhubi

kamalmarhubi Apr 22, 2016

Contributor

Finally! This has been really close for a while, and I spent some time on a train yesterday and in the sun this afternoon pushing it to a PR-worthy state. The commits are broken up logically and should be reviewable individually.

r? @nrc

Contributor

kamalmarhubi commented Apr 22, 2016

Finally! This has been really close for a while, and I spent some time on a train yesterday and in the sun this afternoon pushing it to a PR-worthy state. The commits are broken up logically and should be reviewable individually.

r? @nrc

@kamalmarhubi

This comment has been minimized.

Show comment
Hide comment
@kamalmarhubi

kamalmarhubi Apr 25, 2016

Contributor

@nrc let me know if you'd like any of the commits pulled out to reduce the size of this PR. I think the ones here make most sense in context of this PR, but the first 5 or even 6 commits could be split out into individual PRs.

Contributor

kamalmarhubi commented Apr 25, 2016

@nrc let me know if you'd like any of the commits pulled out to reduce the size of this PR. I think the ones here make most sense in context of this PR, but the first 5 or even 6 commits could be split out into individual PRs.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Apr 26, 2016

Collaborator

I don't think you need the experimental prefix on the option - everything in Rustfmt is experimental for now!

Collaborator

nrc commented Apr 26, 2016

I don't think you need the experimental prefix on the option - everything in Rustfmt is experimental for now!

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Apr 26, 2016

Collaborator

commits 1 & 2 are obvs fine.

re commit 3, I'm not sure if this is worth doing - if people do add this sort of thing to the config file, what harm does it do? Sure, it's weird, but it doesn't seem bad. Not sure if it is worth the complexity to do it.

Collaborator

nrc commented Apr 26, 2016

commits 1 & 2 are obvs fine.

re commit 3, I'm not sure if this is worth doing - if people do add this sort of thing to the config file, what harm does it do? Sure, it's weird, but it doesn't seem bad. Not sure if it is worth the complexity to do it.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Apr 26, 2016

Collaborator

if we do add commit 3, could we use macro magic to do documenting by default and use a keyword to prevent documenting?

Collaborator

nrc commented Apr 26, 2016

if we do add commit 3, could we use macro magic to do documenting by default and use a keyword to prevent documenting?

Show outdated Hide outdated src/bin/rustfmt.rs Outdated
}
}
mod file_lines_spec_parser {

This comment has been minimized.

@nrc

nrc Apr 26, 2016

Collaborator

Rather than all this parsing code, could we just deserialise a JSON string to some structs?

@nrc

nrc Apr 26, 2016

Collaborator

Rather than all this parsing code, could we just deserialise a JSON string to some structs?

This comment has been minimized.

@nrc

nrc Apr 26, 2016

Collaborator

Could also use an empty array to mean no spec, rather than using an Option, which should mean fewer changes to the config code.

@nrc

nrc Apr 26, 2016

Collaborator

Could also use an empty array to mean no spec, rather than using an Option, which should mean fewer changes to the config code.

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Apr 26, 2016

Contributor

Rather than all this parsing code, could we just deserialise a JSON string to some structs?

I'm kind of against JSON in command line arguments in general, but I'll see what that looks like.

Could also use an empty array to mean no spec, rather than using an Option, which should mean fewer changes to the config code.

You mean empty map? Will give that a shot as well.

@kamalmarhubi

kamalmarhubi Apr 26, 2016

Contributor

Rather than all this parsing code, could we just deserialise a JSON string to some structs?

I'm kind of against JSON in command line arguments in general, but I'll see what that looks like.

Could also use an empty array to mean no spec, rather than using an Option, which should mean fewer changes to the config code.

You mean empty map? Will give that a shot as well.

This comment has been minimized.

@nrc

nrc Apr 26, 2016

Collaborator

I'm kind of against JSON in command line arguments in general,

me too usually, but this does seem to be the kind of thing that will machine generated 99% of the time, so JSON seems appropriate

You mean empty map?

I don't really see why this is a map rather than a list (internally it might need to be a map, but in the API it's just a list of spans, no?)

@nrc

nrc Apr 26, 2016

Collaborator

I'm kind of against JSON in command line arguments in general,

me too usually, but this does seem to be the kind of thing that will machine generated 99% of the time, so JSON seems appropriate

You mean empty map?

I don't really see why this is a map rather than a list (internally it might need to be a map, but in the API it's just a list of spans, no?)

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Apr 26, 2016

Contributor

I don't really see why this is a map rather than a list (internally it might need to be a map, but in the API it's just a list of spans, no?)

That's a interesting thought. It is a list of spans, but provided before we have parsed anything. The way control passes at the moment, we can't resolve the spans in the codemap early enough, since the rustfmt::run handles the parsing and accepts a resolved config. I'll take a look at flow around there to see if we can resolve everything to spans, which would potentially let me get rid of the whole LineSet thing.

@kamalmarhubi

kamalmarhubi Apr 26, 2016

Contributor

I don't really see why this is a map rather than a list (internally it might need to be a map, but in the API it's just a list of spans, no?)

That's a interesting thought. It is a list of spans, but provided before we have parsed anything. The way control passes at the moment, we can't resolve the spans in the codemap early enough, since the rustfmt::run handles the parsing and accepts a resolved config. I'll take a look at flow around there to see if we can resolve everything to spans, which would potentially let me get rid of the whole LineSet thing.

This comment has been minimized.

@nrc

nrc Apr 26, 2016

Collaborator

Sorry, I don't mean spans as in the compiler sense, I mean that as far as the user is concerned, it is a list of line ranges: [foo.rs:3:10, foo.rs:12:22, bar.rs:100:102]. Whether it is a map or a list or uses Spans or not, seems entirely an internal thing.

@nrc

nrc Apr 26, 2016

Collaborator

Sorry, I don't mean spans as in the compiler sense, I mean that as far as the user is concerned, it is a list of line ranges: [foo.rs:3:10, foo.rs:12:22, bar.rs:100:102]. Whether it is a map or a list or uses Spans or not, seems entirely an internal thing.

This comment has been minimized.

@kamalmarhubi

kamalmarhubi May 20, 2016

Contributor

me too usually, but this does seem to be the kind of thing that will machine generated 99% of the time, so JSON seems appropriate

I intend to include a utility within rustfmt (cargo-fmt-diff or something like it) to format automatically based on version control diff. This would use rustfmt as a library, and so avoid going via arguments altogether. I expect that would be used most of the time, removing need for machine generation of arguments for the common case. For the admittedly rare case of a user specifying ranges manually, I think it is preferable to avoid JSON.

@kamalmarhubi

kamalmarhubi May 20, 2016

Contributor

me too usually, but this does seem to be the kind of thing that will machine generated 99% of the time, so JSON seems appropriate

I intend to include a utility within rustfmt (cargo-fmt-diff or something like it) to format automatically based on version control diff. This would use rustfmt as a library, and so avoid going via arguments altogether. I expect that would be used most of the time, removing need for machine generation of arguments for the common case. For the admittedly rare case of a user specifying ranges manually, I think it is preferable to avoid JSON.

This comment has been minimized.

@nrc

nrc May 23, 2016

Collaborator

One good use case I see for this work is for an IDE or similar tool to select a bunch of code and send it to rustfmt for reformatting. In that case, you wouldn't want to use an intermediate tool, and a JSON struct indicating the selected text to reformat seems optimal.

@nrc

nrc May 23, 2016

Collaborator

One good use case I see for this work is for an IDE or similar tool to select a bunch of code and send it to rustfmt for reformatting. In that case, you wouldn't want to use an intermediate tool, and a JSON struct indicating the selected text to reformat seems optimal.

@kamalmarhubi

This comment has been minimized.

Show comment
Hide comment
@kamalmarhubi

kamalmarhubi Apr 26, 2016

Contributor

I don't think you need the experimental prefix on the option - everything in Rustfmt is experimental for now!

This makes sense. Oh also I'm 100% open to bikeshedding on the flag name.

Contributor

kamalmarhubi commented Apr 26, 2016

I don't think you need the experimental prefix on the option - everything in Rustfmt is experimental for now!

This makes sense. Oh also I'm 100% open to bikeshedding on the flag name.

@kamalmarhubi

This comment has been minimized.

Show comment
Hide comment
@kamalmarhubi

kamalmarhubi Apr 26, 2016

Contributor

if we do add commit 3, could we use macro magic to do documenting by default and use a keyword to prevent documenting?

The way the macro is set up, I think there needs to be a token available in the pattern both ways. I could be wrong though.

Contributor

kamalmarhubi commented Apr 26, 2016

if we do add commit 3, could we use macro magic to do documenting by default and use a keyword to prevent documenting?

The way the macro is set up, I think there needs to be a token available in the pattern both ways. I could be wrong though.

@kamalmarhubi

This comment has been minimized.

Show comment
Hide comment
@kamalmarhubi

kamalmarhubi Apr 26, 2016

Contributor

re commit 3, I'm not sure if this is worth doing - if people do add this sort of thing to the config file, what harm does it do? Sure, it's weird, but it doesn't seem bad. Not sure if it is worth the complexity to do it.

You've probably figured by now (#795, #801, #871) that I'm not super happy with the way Config conflates the formatting configuration and other configuration. The short version is that I do think it's bad to have nonsensical options available.

The change here is sort of a stop-gap to prevent making the situation worse. If there's no compatibility guarantee for v1.0 and rustfmt.toml, then I'm happy to back that change out of this PR. I do want to do something about this before a v1.0 hits, though!

Contributor

kamalmarhubi commented Apr 26, 2016

re commit 3, I'm not sure if this is worth doing - if people do add this sort of thing to the config file, what harm does it do? Sure, it's weird, but it doesn't seem bad. Not sure if it is worth the complexity to do it.

You've probably figured by now (#795, #801, #871) that I'm not super happy with the way Config conflates the formatting configuration and other configuration. The short version is that I do think it's bad to have nonsensical options available.

The change here is sort of a stop-gap to prevent making the situation worse. If there's no compatibility guarantee for v1.0 and rustfmt.toml, then I'm happy to back that change out of this PR. I do want to do something about this before a v1.0 hits, though!

@kamalmarhubi

This comment has been minimized.

Show comment
Hide comment
@kamalmarhubi

kamalmarhubi Apr 26, 2016

Contributor

@nrc thanks for the comments! It's late here, so I'll push up a few new commits tomorrow. I'll add to the end so GitHub lets us see what's new, but will reorder before merge to keep history tidy.

Contributor

kamalmarhubi commented Apr 26, 2016

@nrc thanks for the comments! It's late here, so I'll push up a few new commits tomorrow. I'll add to the end so GitHub lets us see what's new, but will reorder before merge to keep history tidy.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc May 19, 2016

Collaborator

@kamalmarhubi hey, sorry this dropped off my radar (I blame the lack of notifications on new commits, but I should have checked back in too). What's the status of this PR? Should I review with an eye to merging? Or do you have more work in the pipeline?

Collaborator

nrc commented May 19, 2016

@kamalmarhubi hey, sorry this dropped off my radar (I blame the lack of notifications on new commits, but I should have checked back in too). What's the status of this PR? Should I review with an eye to merging? Or do you have more work in the pipeline?

@kamalmarhubi

This comment has been minimized.

Show comment
Hide comment
@kamalmarhubi

kamalmarhubi May 20, 2016

Contributor

@nrc, no worries. I've been away for a bit myself.

Re this PR, I would like to get it reviewed as is, and merged with whatever changes come out of that review. I want to get the basic infrastructure hammered out and merged in before going too far into further work on it.

I'm replying to a few of the earlier comment threads.

Contributor

kamalmarhubi commented May 20, 2016

@nrc, no worries. I've been away for a bit myself.

Re this PR, I would like to get it reviewed as is, and merged with whatever changes come out of that review. I want to get the basic infrastructure hammered out and merged in before going too far into further work on it.

I'm replying to a few of the earlier comment threads.

@kamalmarhubi

This comment has been minimized.

Show comment
Hide comment
@kamalmarhubi

kamalmarhubi May 26, 2016

Contributor

I addressed your comments re using JSON, and having the user's view being a list of spans rather than a map. The changes were enough that I moved it to a new PR, #1007.

Contributor

kamalmarhubi commented May 26, 2016

I addressed your comments re using JSON, and having the user's view being a list of spans rather than a map. The changes were enough that I moved it to a new PR, #1007.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment