SE-0250: Swift Code Style Guidelines and Formatter

(Matthew Johnson) #204

I am not talking about personal preferences here. What I am talking about is idioms that require idiosyncratic formatting, such as the declarative and fluent idioms demonstrated upthread.

I very strongly believe that programmers should not need to modify the default configuration of the formatter just because they are programming in an idiom such as these. If Swift is going to have an "official" formatter its default configuration should be compatible with idioms that many Swift programmers use, and ideally compatible with idioms invented in the future. The point I am making is that this is in tension with the goal of absolute consistency and indeed limits the scope of rules that may be applied by default.

4 Likes
(Jeremy David Giesbrecht) #205

May I contribute a concrete example that occurred during the development of SwiftLint?

Users requested a rule about spacing line comments and a pull request was submitted to implement it. The rule was basically that line comments should be spaced like this:

statement() // some comment

I assume we can all agree that the following look sloppy and the deviation is not likely a conscious decision:

statement()//some comment
statement() //some comment
statement() //  some comment

But when the rule was tested against real code, examples of meaningful deviation were quickly discovered, including forms like the following:

enum Stuff {
    case short                           // Notes...
    case mediumLength                    // More notes...
    case somethingWhichIsMuchLongerStill // Still more notes...
}
//   ∞         n + 1     2n − 1
//   ∑    ( (−1)      __θ________ )
// n = 1               (2n − 1)!

The pull request was returned for revision to account for these. Direction was given on how to adjust implementation, but the pull request authors opted not to invest the effort.

The points that can be learned from it are the following:

  1. There is such thing as meaningful deviation. (Though I personally don’t think it should be interpreted quite as broadly as some here seem to think on either side of the discussion; I do not consider this to apply to things like brace positioning.)

  2. The existence of meaningful deviation does not invalidate the usefulness of rule. Even the code bases containing those exceptions would have benefitted from having the other 99% of their comments formatted consistently.

  3. As long as the meaningful deviation is identified by the tool developers, they can usually find a reasonable implementation that recognizes the difference between thoughtful exceptions and sloppiness.


I do agree with @masters3d that there are some kinds of variation better handled other ways, (though again, I do not interpret that as broadly as others here):

  • Configuration: Project A wants English identifiers. Project B wants Chinese identifiers. (But in my opinion colon spacing is not a candidate for configuration.)
  • Path level exceptions: Generated files (like package test manifests) in the event their generators are not up to date with the current guidelines.
  • A directive based escape hatch:
    // @noformat
    doStuff()
    // @format
    
    While in theory I believe a sufficiently well designed tool should never need this, the reality is there will always be minor bugs and oversights in any release. The ability to use directives like this enables the tool to still be used on the rest of the project without breaking the problem spots where the issue manifests.
5 Likes
(Matthew Johnson) #206

This is a great example of meaningful deviation. A related example is column alignment in switch statements which I use when there is a single statement in each case and line length is acceptable.

switch {
case .first(let x):  return x.value
case .second(let y): return y.value
}

I find the effort to maintain this is very much worth the gain in readability. An official formatter must respect choices like this. If it is able to recognize and automate the idiom even better! On the other hand, if it were to increase the effort required to use this style I would find it unacceptable.

These kinds of idiomatic style decisions are much different than things like colon hugging where there is no meaningful reason to deviate and which should be applied universally and consistently throughout a codebase.

6 Likes
(Jeremy David Giesbrecht) #207

Is should add the caveat that every time I say “such‐an‐such a rule has no meaningful exceptions,” what I really mean is, “I have never seen a meaningful exception yet.” Anyone is welcome to prove me wrong by demonstrating with an example. (Though it must exhibit clear and strong reasoning; it cannot just be a difference out of habit.)

I think the tool developers should maintain the same attitude as they do their work and the tool evolves. Do you agree @allevato?

4 Likes
(Tony Allevato) #208

Absolutely, no question. It's impossible to predict what requirements might arise in the future, so the idea of building a tool that is static and never considering new patterns or choices that might emerge is neither realistic nor desirable.

3 Likes
(Chéyo Jiménez) #209

Understood. I highly doubt that the formater you described will be delivered by the swift project. The good thing is that nobody is forcing anybody to use this formater. Honestly I am probably going to keep contributing to SwiftLint and make it what I want. Look at the swift package manager. Most people use other package managers. Same thing we got going on here. If anything a swift-format tool could just help on making a configuration file that other tools could adopt.

1 Like
(Nobody1707) #210

I do this too, but I line up the colons instead of the dot.

1 Like
(Matthew Johnson) #211

I would find it pretty unfortunate to see Swift ship with a formatter that is by default incompatible with the use of common (and new!) idioms. That is why I am participating heavily in this discussion. My hope is that @allevato will take these concerns seriously and the default configuration of the tool will not result in users fighting with the tool just because they have adopted an idiom it doesn't understand.

The example of the SwiftLint developers abandon a PR instead of doing the extra work to respect the idiom is a great one. My concern is that with an official tool the PR may get merged and the extra work punted to the future or considered out of scope, leaving users fighting with the tool for some period of time, or perhaps even indefinitely. If the goal is for the tool to be widely adopted I think we need a more conservative approach of "first do no harm" (at least for the default configuration).

1 Like
(Tony Allevato) #212

As far as I'm concerned, making everyone happy is an unrealistic non-goal. We've already seen feedback in this thread where some folks say that the formatter should enforce a single style without configuration, and some say that it should be highly configurable so that it can enforce whatever style preferences they desire. It's simply not possible to satisfy both of those audiences at the same time.

Rather, my goal is to try to provide the best value for the most people. There are some folks who argue that if a formatter doesn't do exactly what they think it should, then the Swift project shouldn't adopt one. I think that's unfair to the thousands (or more?) programmers out there who simply don't have strong opinions about specific style guidelines and just want a great experience that works out of the box that keeps their code clean and consistent by enforcing reasonable guidelines.

For those who have more specific requirements, we can try to make the tooling work for them too. But in some cases that may just not be possible (it's really a case-by-case basis), and I have absolutely no problem with anyone using different tooling that supports their needs.

5 Likes
(Matthew Johnson) #213

This is not the position I am taking. I certainly hope you don't feel like I am. I am only arguing for a tool that is conservative in its default behavior in order to ensure users don't end up fighting with the tool when they adopt reasonable idioms.

Surely these programmers don't want to face a steep cliff just because they learn and start using an idiom. I hope you will agree that a great experience out of the box means being able to do so without having to configure or fight with the formatter.

What I describe above is categorically different from specific style preferences. It's perfectly acceptable to require configuration in this case. Hopefully nobody would have to face a steep cliff where the tool simply fails to work at all though. Instead, in cases where a desired style convention isn't possible to support in the tool it should be possible to disable the transformations that are incompatible with the desired style. This will allow the rest of the transformations applied by the tool to continue to be used.

2 Likes
(Tony Allevato) #214

Not at all! I've really appreciated your feedback throughout these threads.

This is part of what motivates features like respectsExistingNewLines that I've cited before. That's one concrete way that's been implemented so far we try to address the fact that there may not be a single correct way to handle certain constructs. There are certainly others that we haven't implemented yet; one that I like to think about a lot is multi-line array literal formatting. Scientific computing folks don't want the inter-comma spaces in this to be collapsed:

let m = [
  -1,  0,  1,
   0,  0,  0,
   1,  0, -1,
]

I agree that that is ideal, in a world where the accepted result is a formatter with configurability.

To better articulate my thoughts on this overall, ultimately I think the path we take depends on what the ultimate decision about configurability ends up being:

  • If the Core Team decides that the solution they think is best is a formatter that supports a single canonical style with extremely limited configurability, then that's pretty much that—all the rest of the points you raised are moot.

  • If the Core Team decides that the solution they think is best is a formatter with a particular default style, but which can otherwise be configured differently to support making other decisions, then I think your goals and mine are in total agreement—we should strive for an implementation that both provides as many users as possible with the behavior that they need, and do so in such a way that the default behavior does not get in their way.

4 Likes
(Matthew Johnson) #215

Do you envision that potentially being enabled by default? It’s a blunt approach but might be the best a tool can do, at least at first. It would be far better than people having to fight with or configure the formatter (or worse - feel pressured to abandon useful idioms).

This is another great example of a very useful and well-considered idiom. I’m glad to see that you’re thinking carefully about how to handle these kinds of situations. It’s obviously nontrivial for a tool to distinguish cases like this from code that is just poorly or thoughtlessly formatted.

Awesome! I hope the Core Team sees the wisdom in this approach and especially keeping the defaults conservative. Transformations that are more aggressive and therefore potentially problematic in some contexts should remain opt-in. In some cases, after we gain enough experience with them to be confident they will not be problematic they could eventually become enabled by default.

1 Like
(Tony Allevato) #216

It is in the current implementation, and I think the advantages to doing so far outweigh the benefits of "uniformity" in that case such that I don't see a compelling reason to change it. In particular, imagine that in the following expression, the length of the four arguments exceeded the column limit:

let bounds = CGRect(x: theLeft, y: theTop, width: someWidthExpression, height: someHeightExpression)

I think many people would find the first result below preferable to the second, thanks to the logical grouping:

let bounds = CGRect(
  x: theLeft, y: theTop,
  width: someWidthExpression, height: someHeightExpression)

let bounds = CGRect(
  x: theLeft, y: theTop, width: someWidthExpression,
  height: someHeightExpression)

This is the kind of decision that would require some pretty deep special casing to do automatically, but is much easier to handle if the user just puts their own line break there and the formatter doesn't screw with it.

4 Likes
(Ted Kremenek) #217

SE-0250 Review Adjustment

Hi everyone,

This has been a charged and fractured review conversation. Myself and the rest of the Core Team appreciate how much energy and thought has been poured into this discussion.

The signal from this review and pitch threads, however, has raised questions for me (as review manager) of whether or not the framing of the discussion has been put forth to the community in the optimal way.

Some specific thoughts/concerns:

  1. This discussion is a bit abstract. Some folks (rightly) are concerned on how they can evaluate the idea of having a style guide without having an actual style guide to review.

  2. There should be a broader discussion behind the format tool and its functionality and role. This ties in, however, with the conversation about the style guide itself being too abstract (#1).

  3. Some reviewers commented on the existence of different formatting/linting tools created by the community, and raised some important points:

    A. What would be the benefit/role of having an “official” formatter as part of the Swift project?

    B. What would having an “official” formatter mean for the other tools which may play slightly different or complementary roles for users? Why should swift-format be chosen as the official formatter?

There's a lot going on here, and when combined together these points paint a picture that many members of the community have found it difficult to engage with this discussion, sometimes having conflicting opinions on different pieces of the proposal. It is important that we address this problem, especially given the importance of this overall topic to the community.

For the next step in this conversation, we are going to:

  1. Suspend the review of the proposal as it is framed today.

  2. The Core Team will look on how to re-center the discussion and separate/stage parts of the conversation so that as a community we can all feel we can effectively. The goal is to take into account the feedback and concerns raised in the review thread.

This is not a rejection or approval of what has been put forth in the proposal, but a recognition that the conversation about this important topic needs to be adjusted to allow the community to engage with the topics raised more effectively.

For next steps, my plan is to work with the proposal authors and connect with various folks in the community who raised meta-concerns on this review thread to work out how to continue this discussion. If you would like to provide me specific feedback for working out the path forward for this conversation, please feel free to email me or direct message me on the forums.

I want to extend a heartfelt thanks to Tony Allevato, who has been a patient and active proponent of this conversation.

I also want to again thank everyone who has participated in this discussion. We will be continuing it in the not-so-distant future but in a modified form.

Ted

44 Likes
(Jeremy David Giesbrecht) #218

May I request that, whatever this continuation is, effort would be made to ensure it happens swiftly. (Pun unintended.)

This unanswered question is having far reaching effects. From a thread on GitHub:

The prospect of submitting a PR [to SwiftLint] not particularly inviting. [...]

Another consideration is the discussion of a possible official tool, the question of its scope, and the uncertainty of what that means for other similar tools. I am deferring work related to style checking—no matter the tool: Workspace, SwiftLint, swift-format, or any other—until there is a clearer answer. It is simply impossible right now to tell the difference between time well invested on unique features and time wasted on something about to be replaced.

I suspect it is affecting many projects and developers in similar ways.

4 Likes
(Goffredo Marocchi) #219

JI cannot see how the configurability mentioned here ( https://github.com/apple/swift-evolution/blob/master/proposals/0250-swift-style-guide-and-formatter.md#configurability-of-formatting ) will be meaningful and not be shallow.

How could that be otherwise and still satisfy the, not practical and not without unintended consequences IMHO, “second goal” to allow developers to move from codebase to codebase without any mental load / without having to adapt like they do now in many cases?

Are we really, a bit naively, convinced that you can allow meaningful configurability of formatting and avoid people to change codebase a without having to adapt like they do now? Do we think people will learn all the possible rules and settings for each rule?

1 Like
(Tony Allevato) #220

It's unclear whether your concern is about contributors to projects having to learn new rules or project owners having to learn them, but in either case, the answer is the same:

If a contributor moves to a new project, one of two things will happen:

  • The project doesn't use any kind of automated formatting, so the user has to learn and internalize the style that the project owners prefer, otherwise they'll have to waste time during code reviews being told to clean up their formatting.
  • The project does use automated formatting, so the actual style doesn't matter. The contributor can learn it if they wish, but even if they don't, the tool will automatically apply the desired style.

For project owners who care deeply about specific style decisions,

  • If they don't use an automated formatting tool, then they're probably going to spend a lot of time commenting on the style of other contributors if they care deeply enough that they want to keep their codebase consistent.
  • If they do use an automated formatting tool, then there's a one-time cost to configure it as they wish, but then all they have to do is say "run the tool" (or even better, have their CI reject contributions that aren't properly formatted) and move on with their lives.

So I'm having difficulty seeing where you think this major problem of adapting to new styles or codebases is coming from. The whole point of automated formatting tools is to eliminate those concerns, and as someone who has configured them in my own projects and also contributed to other projects that had existing style rules and the tooling to enforce them, they absolutely do.

4 Likes
(Goffredo Marocchi) #221

Hey Tony,

From the actual proposal:

But creating code style guidelines that are maintained by the language owners and community, along with tooling that allows users to easily adopt those guidelines, provides a number of additional benefits:
[...]
2. Developers can move from one codebase to another without incurring the mental load of learning and conforming to a different style or being required to reconfigure their development environment.

I was commenting points from the proposal, I was not talking about maintainers or contributors really... we have lots of automated tools and I have already expressed I would prefer to see energy poured into powering up SwiftSyntax or any other tool that lowers the barrier of entry to making and maintaining a code linter and / or formatter.

I was comparing the proposal stating the need of “official” style guidelines, what problems they solve (see quoted) and tools promising that

Vs.

the promise of a configurable formatter where every inch of configurability takes away from the weigh behind the quoted point two.

(Ted Kremenek) #222

Thoughts on where to take this discussion

The direction I think this discussion should go is to break it apart into two parts:

  • The style guide itself
  • Technology to support formatting

The two are related, but also policies about both are getting conflated in this discussion. Whether or not Swift gets an "official" or "recommended" style guide is related, but separate, from whether or not there should be formatting technology supported directly or indirectly by the Swift project. Similarly, having an agreed upon style is something we could have without an official formatter.

I think there is real value in discussing formatting technology supported by the Swift project. That discussion should be focused on building technology — which would go in the Development section of the forums. Building technology is something we do routinely with open discussion, but we don't take those through the Evolution process. The purpose of the Evolution process is to discuss changing the definition of the language or API surface of the Standard Library. One motivator for focusing on building such technology support is that the Language Server Protocol supports formatting in its API surface, which is not all that surprising since most engineers expect that editors can perform some amount of code formatting as a standard editing operation. How such code formatting should be configurable or what underlying technology is used to support that is something we can discuss in the forums independent of this style guide. But this does feel like an important direction in the project for Swift to support in some way— irrespective if there is an official style guide or not.

Regarding the official style guide, fundamentally it feels like such a discussion needs to focus around having an actual style guide for the community to discuss. I think there's enough feedback in this review that a style guide would be welcome by a good portion of the community, but that it depends on the details. That's where I think this particular discussion should pick up.

Given the nature of a discussion about an actual style guide with the immense attention that would gather from the community (not to mention limitations on my own time in the near term) I suspect such a discussion at the earliest would come about in June. There are a variety of proposals that need to be considered for Swift 5.1 in the near term, and I want to make sure the community as a whole can focus on important topics with adequate time and attention.

I will hold this thread open a little longer, but then likely suspend it to match with the review itself being suspended.

25 Likes
(Boris Triebel) #223

Great! I am happy that we have this decision for a very sensible mode of operation.

I am happy we did not end up in some kind of Brexit like situation where we decide to start a big endeavour with a fuzzy goal that is driven by ambition, hopes and fears (and lacking the details) and then we end up working through the details and the reality of it with people arguing that "it has already been decided that we want to do this" while we learn what it actually means in practical terms.

2 Likes