tidy-requires: module paths w/in require subforms should sort

[Bogdanp]

This suggestion conflicts with this racket-review rule which ensures that uses of require subforms sort according to their module path. Another example can be found here.

[jackfirth]

I agree with this, and others have mentioned the possibility to me too. But I'd also like to keep Resyntax and racket-mode in sync when it comes to require tidying. I'll reach out to Greg to discuss this.

[greghendershott]

This sounds good in principle. I'd need to reload my brain with details about multi-in; the other clauses seem straightforward. I'll comment back here later...

[greghendershott]

So actually. Having finished my first coffee. I realized that racket-tidy-requires already works the way you described in your email, as in your example:

(require a
         b
         c
         (only-in a/extra x))

becomes

(require a
         (only-in a/extra x)
         b
         c)

Was there maybe some other example where it does not? Maybe I have a bug in some cases. If so, I'll fix. But the intent is definitely to sort by module (within each meta level).

TL;DR: I think you could go ahead and change resyntax to sort that way, and if some bug turns up, then it's on me to fix that.

[jackfirth]

Ah, I was going off the description here rather than actually installing racket-mode and seeing what it does, since I'm not an emacs user. What does it do if the require subform doesn't contain exactly one collection in it? Like (prefix-in foo: (combine-in a b c)), for example.

[Bogdanp]

Review didn't support combine-in at all (because I never use it), so I just added it:

Bogdanp/racket-review@6ce349f#diff-dd9e24dda16c8ecc44babd298601cf79cbe4cd068711b7e6f8b8135691492e2a

Running tidy requires on that same file results in:

#lang racket/base
(require (prefix-in combined: (combine-in threading))
         (combine-in)
         racket/string
         (prefix-in combined: (combine-in threading racket/base)))

[greghendershott]

Ah, I was going off the description here rather than actually installing racket-mode and seeing what it does, since I'm not an emacs user.

Although that's intended as user docs, not a full spec, it is ambiguous; I can improve that.

What does it do if the require subform doesn't contain exactly one collection in it? Like (prefix-in foo: (combine-in a b c)), for example.

It's not even attempting to handle combine-in, yet.

I started a branch last week to refactor some of this (very old) code of mine, looking at the require grammar.

Among other things, I did add combine-in -- but I didn't consider the empty (combine-in) case. Thanks for pointing it out!

My instinct is to prune empty (combine-in) clauses. Although "tidy" shouldn't try to prune unused imports -- that's the job of a "stronger" command in Racket Mode, that runs the unused requires analysis -- pruning no-meaning, no-op "import nothing" clauses like (combine-in) is probably OK or in fact desirable. What do you think?

In any case it begs the question, if we're sorting by module, but (combine-in) has zero modules, where to sort it -- which I think was why you asked, @jackfirth.

EDIT: I saw the empty (combine-in) from @Bogdanp 's example, and overlooked your actual question, @jackfirth . I'm proposing to sort by the module of the first require-spec (if any) in a combine-in form. So a for (combine-in a b c).

---

Generally: It looks like I should re-read the grammar carefully, looking for any more edge cases or bad assumptions.

[jackfirth]

Perhaps the three of us ought to standardize on a shared implementation? A minimal-dependency package that we all reuse would get rid of any inconsistency problems.

[Bogdanp]

In any case it begs the question, if we're sorting by module, but (combine-in) has zero modules, where to sort it -- which I think was why you asked, @jackfirth.

IMO, pruning empty (combine-in) (and other no-op) forms is the way to go.

EDIT: I saw the empty (combine-in) from @Bogdanp 's example, and overlooked your actual question, @jackfirth . I'm proposing to sort by the module of the first require-spec (if any) in a combine-in form. So a for (combine-in a b c).

That's what review does, and it expects any nested specs to be sorted by the same rules.

Perhaps the three of us ought to standardize on a shared implementation? A minimal-dependency package that we all reuse would get rid of any inconsistency problems.

I'm not against this, but my package only lints and doesn't propose changes, so something like this might end up complicating it. The rules themselves are straightforward IMO:

1. (for-syntax) requires must appear at the beginning.
2. Relative requires must appear at the end.
3. Specs are sorted according to the first module path, meaning
   a) module paths sort as themselves and
   b) require subforms sort as their first module path child.
4. The same rules apply to subforms, recursively.

Here's an example of a well-sorted require form according to these rules:

(require (for-syntax racket/base
                     syntax/parse/pre)
         deta
         (prefix-in http: net/http-easy)
         racket/contract/base
         (except-in racket/list group-by)
         racket/string
         threading
         web-server/http
         "../a.rkt"
         "b.rkt")

Running racket-tidy-requires on this form appears to leave it mostly unchanged:

(require (for-syntax racket/base
                     syntax/parse/pre)
         deta
         (prefix-in http: net/http-easy)
         racket/contract/base
         (except-in racket/list
                    group-by)
         racket/string
         threading
         web-server/http
         "../a.rkt"
         "b.rkt")

The only change is the indentation of except-in, which review has no opinion on. Review currently also doesn't have an opinion on where to put for-label, for-meta, etc. requires, but I'm happy to follow whatever you two prefer there.

[greghendershott]

I'm also not opposed to code sharing. But ATM I'm rewriting some of the old code. So that it still passes the old tests. But also handles some holes like combine-in. And is less smelly, to me, in some spots.

When the dust settles, I can touch base re sharing.

Few quick points about that, meanwhile:

• My "outer API" is text => text: My Racket back end gets strings from Emacs, and returns strings. That doesn't mean a symbol or even syntax API couldn't be added, as well. But status quo, that's my edge data.

• Racket Mode attempts to support old versions of Racket. Currently as old as 6.12.

• Racket Mode is installed as an Emacs package, not a Racket package. It doesn't depend on any raco setup. Some optional features work if the necessary libs were installed by the user, otherwise they gracefully-ish don't work. Tidying requires fits fine in this category, I think. Already it needs macro-debugger-lib installed.

• For a certain private project I had an infatuation with multi-in, and added support for the simpler flavors of that. That complicates my current code, somewhat, for better or worse.

[greghendershott]

IMHO what review does is reasonably a "subset" of what resyntax or Racket Mode might do. IOW the latter might format or arrange things in a way that's nice, but doesn't necessarily rise to the level of a code review warning or recommendation. I think?

It's probably more important for resyntax and Racket Mode to be in sync.

Also to be clear, I don't even know how many Racket Mode users actively use racket-tidy-requires, racket-trim-requires, and racket-base-requires. And when a project uses resyntax, my feelings wouldn't be hurt if the project forbid using those Racket Mode commands on source code, and lets resyntax prevail, when it's being applied to all the code all the time.

[greghendershott]

I took a fresh look at the old "tidying" code and came up with this, squashed commit: greghendershott/racket-mode@ecde6e8

It tries to be more principled about following the require spec grammar.

It also tries to reduce or elide useless forms -- even just as part of just basic tidying. (This is independent of the "stronger" business of using macro-debugger-lib to identify modules that needn't be imported at all, which IIUC is a non-goal for resyntax.)

IIUC the sorting shouldn't contradict what you're doing @Bogdanp in racket-review.

@jackfirth I'm not sure if this is something that you would like to reuse?

If so, I could look at making this available as a package (although I might "vendor" that same source directly into the repo for Racket Mode... I would need to think about the pros and cons).

[jackfirth]

@greghendershott For Resyntax to reuse it, it would have to use syntax objects instead of plain datums. Resyntax relies on syntax properties and source locations in the inputs and outputs of refactoring rules to figure various things out. So whatever tidying system Resyntax uses has to accept and return a (require ...) syntax object.

[greghendershott]

In that commit using syntax classes to drive most of the work, I do store syntax objects in the "sortable" structs.

Currently I convert to datums because the result of my internal tidy function going into a tidy-pretty-format function anyway -- currently my outer API returns a string, with everything pre-indented.

For some alternative API (e.g. for you), I could trivially avoid conversion back to datums. Probably a bit more work to ensure that I'm manipulating syntax objects without losing srcloc and props, everywhere. But probably straightforward.

Two questions:

1. How does resyntax handle indentation? (If you get a stx obj, how/when do you convert back to text?)

2. How does resyntax handle the case where the original program has multiple require forms within a module?

   In my notion of "tidy", these ought to be combined into one require form. And so the API is you're responsible for giving me a list of zero or more old require forms that you found in the program, deleting them all, and inserting at the site of the first one the new pre-formatted text I give back to you.

   And so I wonder what syntax object(s) you would desire to supply, as well as to get back?

I guess with both questions, my meta-question is, "What's your ideal API?" Using syntax objects, OK, but other details like this?

[greghendershott]

I think part of the impedance mismatch is that racket-tidy-requires doesn't just sort existing require forms.

It also "gathers" multiple require forms within a module, and consolidates them into just one.

So if some file has

(require c)
(require b)
(require a)

it will produce

(require a
         b
         c)

Also, the racket-base-requires variant -- which adds requires to change from #lang racket to racket/base -- can add a require form when the original file had zero.

So it is pretty basic to the API that the result is some sort of "change list" -- a list of inserts and deletes. The special case of a delete+insert at the same position, can be presented instead as a replace (which can help the client handle surrounding whitespace more nicely). So the change list consists of "delete", "insert", and "replace".

---

I think what this means is that resyntax, status quo, isn't attempting to do "as much" as racket-tidy-requires. And maybe never wants to attempt more? I don't know.

I think this also means is that any shared library would need to offer an API that allows access to the "sorting" aspect -- but not "gathering"/consolidating.

(And of course, you'd want that to work as syntax => syntax. But that alone isn't the biggest deal.)