FS0058 warning on incorrect indentation incorrectly thrown with method attributes

[abelbraaksma]

When using attributes with let-bindings the calculation for the beginning of the new statement is off by 1, and sometimes more: if the attribute includes parens, it requires these parens to start past the previous let-binding.

It is correct, however, for class members.

Repro steps

Let bindings repro

This throws FS0058 on "let"

module Literals =
    [<Literal>] let First = 1
    [<Literal>] let Second = 2
    //..........^.............    // location of the FS0058 warnings

Minimal indentation to prevent warning (which oddly suggests it is an inner let-binding, but this is not how it gets compiled)

module Literals =
    [<Literal>] let First = 1
     [<Literal>] let Second = 2

Let bindings with parens repro

module Literals =
    [<CompiledName("Boolean")>]  let boolean = true
    [<CompiledName("MyString")>] let myString = "test"
    //............^..............^....................   // locations of the FS0058 warnings

Minimal indentation to prevent both warnings (to prevent only the second, one space is enough):

module Literals =
    [<CompiledName("Boolean")>]  let boolean = true
                    [<CompiledName("MyString")>] let myString = "test"

This obviously makes this utterly unreadable.

Positive example of the same scenario with members

No warning is thrown when using the same scenario on members, the keywords, here static member can appear right underneath one another, as they should.

module Literals =
    type LitTest =
        [<CompiledName("TryMe1")>] static member TryMe() = ()
        [<CompiledName("Really")>] static member TryMe2() = ()

Expected behavior

No warnings in either case.

Known workarounds

The workarounds are:

1. Don't use attributes on let-bindings (may not be possible)
2. Don't use them on the same line (lesser readability)
3. Ignore the warning with #nowarn "58", but this will, unfortunately, apply to the whole file
4. (edit) Use let [<attrib>] identifier instead of [<attrib>] let identifier; this may well be the best and easiest workaround (but I still believe it is an error in offset calculation for the original issue).

Related information

Tested with most recent (as of today) versions of F# 4.0 on VS2015, Update 3. Not tested yet with F# 4.1. I believe this behavior appeared on older versions of F# as well, so it may have been raised before, but I couldn't find it reported.

While not a huge issue of sorts, it is often desirable to be able to align declarations for readability. It just clutters the eye when you require multiple lines for simple constants (whereas most F# statements are beautifully succinct). Usually F# does an excellent job at it, but somehow it fails with the omnipresent let-binding in conjunction with attributes.

[dsyme]

@abelbraaksma Please list this as a language suggestion at http://github.com/fsharp/fslang-suggestions. While sub-optimal we're not going to treat this as a bug fix, but would treat it as a language update/revision through an RFC process. That's because the act of making fixes in this area would almost certainly be a breaking change and we would have to be incredibly careful

The current recommendation is to always use the workaround "Don't use them on the same line (lesser readability)"

Thanks
Don

[abelbraaksma]

That's because the act of making fixes in this area would almost certainly be a breaking change and we would have to be incredibly careful

@dsyme I'm not sure I agree with you that it'll be a breaking change, as it applies to a scenario that currently throws a warning. People that ignore the warning will see the warning disappear, but the compiled result is the same.

Or perhaps you mean that people that extra-indent to get rid of the warning would (after fixing this) find their indented code to act as local-scoped variables? Yes, that would be breaking (though this may be a rare or non-existing in-the-wild situation).

I'll prepare a language suggestion.

[dsyme]

@dsyme I'm not sure I agree with you that it'll be a breaking change

@abelbraaksma You're right - I'm more referring to the risk of cascading changes to other code, which is best assessed through an RFC process I think.

[dsyme]

@abelbraaksma I think you've created a http://github.com/fsharp/fslang-suggestions for this?

[abelbraaksma]

I raised it here: fsharp/fslang-suggestions#514