fsharp / fslang-design

RFCs and docs related to the F# language design process, see https://github.com/fsharp/fslang-suggestions to submit ideas
518 stars 144 forks source link

[style-guide] Advice on Attributes needs improving #711

Closed pblasucci closed 1 year ago

pblasucci commented 1 year ago

In reviewing the style guide, I notice the advice on attributes is... incomplete? inaccurate? In any event, there are cases not covered. And I think the advice which is present is overly general (though this only becomes obvious when you also consider the missing use cases).

TL;DR -- I propose the following changes:

  1. Extend the guidance to account for attributing mutually-referential type definitions.
  2. Amend the current advice, specifically for constants and units of measure whence the attribute is given after the keyword.
  3. (Probably a stretch, but...) Adjust Fantomas to honor the above (if possible).

The guide effectively partitions all uses of Attributes into two buckets:

I've no qualms about the first bucket. The advice on attributing parameters is solid. :+1:

However, the vastness of "anything other than a parameter" is problematic. The recommendations are very awkward for mutually-referential type definitions.

Example 1a: Attributing mutually-referential type definitions
[<One>]
[<Two>]
type OneRecord = {
    OneField : int
    TwoField : TwoRecord
} 
// ⮟ valid -- but award
and [<One>]
    [<Two>]
    TwoRecord = {
    OneField : int
    TwoField : OneRecord option
}

Really, in these cases, the recommendation should be the same as for parameters (i.e. a single set of brackets containing a list of attributes separated by semicolons).

Example 1b: Attributing mutually-referential type definitions
[<One>]
[<Two>]
type OneRecord = {
    OneField : int
    TwoField : TwoRecord
} 
// ⮟ suggested improvement
and [<One; Two>] TwoRecord = {
    OneField : int
    TwoField : OneRecord option
}

Additionally, the current recommendations lead to extra verbosity on constants and units of measure. There is quite a lot of code "in the wild" which places LiteralAttribute or MeasureAttribute after the let or type keyword (n.b. this is specific to putting the attribute after the keyword; the general advice of having the attribute on its own line is, for other instances, very solid).

Example 2: Attributing constants and units of measure
// ⮟ current guidelines lead to

[<Measure>]
type usd

[<Measure>] 
type eur

 [<Measure>]
type chf

[<Literal>] 
let Success = 0

[<Literal>] 
let Failure = Int32.MinValue

// ⮟ yet, this is seen in many different code bases

type [<Measure>] usd
type [<Measure>] eur
type [<Measure>] chf

let [<Literal>] Success = 0
let [<Literal>] Failure = Int32.MinValue

In conclusion, I know there's no template here, but let me just add:

However, contributing to Fantomas is probably impractical given my available bandwidth (hopefully, this is immaterial to the validity of the suggestion -- otherwise the cart is pulling the horse :wink:).

dsyme commented 1 year ago

For (1a) my advice is really to move to module rec or namespace rec. I don't think we should format as single line.

For (1b), the existing advice is really the advice we want, the single line addiction in F# is too strong :) So my preferred decision is not to change the advice here.