fsharp/fslang-suggestions

Relax hash directives (`#token`) to take values other than strings, and formalize their syntax

abelbraaksma opened this issue · 8 comments

abelbraaksma commented

I propose we allow hash directives to take integers, floats, and (long) identifiers, so we can write #time on and #nowarn 1234. At the same time, this syntax is not prescriptive in the F# spec, I propose to formalize it so we can reason about it.

The existing way of approaching this problem in F# is: write #time "on" or #nowarn "1234".

Pros and Cons

The advantages of making this adjustment to F# are

We can now simplify the use of, and expand the scope of allowed types for existing directives. This allows tokens like #nowarn to take not only a number, but also FS1104 as input, which is the form in which warnings are output. It is also the form that's accepted already on the commandline.

Formalizing the syntax helps to reason about it, and makes it easier to add new directives or expand on existing ones.

Before:

#nowarn "0070"
#nowarn "FS1140"  // error
#nowarn "0025" "1140" "1234"
#time "on"
#r "System.Core"

After:

#nowarn 70
#nowarn 25 1140 1234
#nowarn "FS1140" // string is still allowed
#nowarn FS1140
#time on
#r System.Core

The disadvantages of making this adjustment to F# are

None that I can think of.

Extra information

Please note that this change does not intend to change the behavior of these constructs. It merely allows more freedom with the syntax of the arguments, and formalizes the syntax of the status quo.

Estimated cost (XS, S, M, L, XL, XXL): XS

This work has already been started (see dotnet/fsharp#17206). @dsyme, if that PR is accepted, could you do the honors and mark this approved so that we can create an RFC for this?

Related suggestions:

Affidavit (please submit!)

Please tick these items by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on StackOverflow) and I have searched StackOverflow for discussions of this issue
  • This is a language change and not purely a tooling change (e.g. compiler bug, editor support, warning/error messages, new warning, non-breaking optimisation) belonging to the compiler and tooling repository
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

smoothdeveloper commented

Note that the #load, I believe, allow to take extra parameters on separate lines:

#load 
  "filea.fsx"
  "fileb.fsx"

Should similar be allowed with #nowarn or should it be restricted to only parse arguments on a single line?

brianrourkeboll commented

Should diagnostic number prefixes other than FS be allowed in the new syntax (even if there is no plan for additional tooling support for them in the immediate term)?

C# supports the CS diagnostic prefix as well as others in #pragma warning disable/restore, e.g., IDE (style) and CA (code analysis):

pragma warning disable IDE0059 // Unnecessary assignment of a value
i = 5;
#pragma warning restore IDE0059 // Unnecessary assignment of a value
#pragma warning disable CA1831 // Use AsSpan or AsMemory instead of Range-based indexers when appropriate
ReadOnlySpan<char> slice = str[1..3];
#pragma warning restore CA1831 // Use AsSpan or AsMemory instead of Range-based indexers when appropriate
abelbraaksma commented

@brianrourkeboll, they already are allowed (it's just that FS is explicitly disallowed, no idea why). However, they don't work: the IDE warnings are, unfortunately, currently not suppressed in F#.

image

And the disable/restore cycle is a linked issue, which we should tackle separately (I linked #278 in the original message).

  • 👍1
abelbraaksma commented

@smoothdeveloper, good point, but the multiline feature is already allowed for #nowarn. I didn't know that. Another reason to formalize the ABNF (or whatchamacallit) for this, as this is not clear from the specs.

image

KevinRansom commented

@smoothdeveloper

The parser knows nothing about any specific directives, except for SOURCE_DIRECTORY | SOURCE_LINE| SOURCE_FILE. I had no intent to teach the parser about the specific directives, although that may be a way to go.

The current valid format is:

  • #directive name list of quoted string arguments.

This change alters it to:

  • #directive name list of [quoted string|Int32|Ident|LongIdent] arguments.

  • #if/#else/#endif is an entirely separate mechanism.

TBH:
... there is no current directive that requires LongIdent, although the #h PR : dotnet/fsharp#17140 suggests a good scenario for it.

  • 👍1
KevinRansom commented

@brianrourkeboll, we could and perhaps should pass the Nuget nowarns through to the nuget package manager. We currently don't.

abelbraaksma commented

@dsyme there seems to be some consensus here and @KevinRansom already worked on an implementation. This does not change existing behavior of anything, only allows a little more syntax with hash directives.

Would agree to this change?

I'll create the (small) RFC for this, to further Kevin's PR.

dsyme commented

Yes this is fine, agreed it makes sense

  • 👍3