Allow ParsedHashDirectives to take non string arguments

[KevinRansom]

Requires fslang approval : fsharp/fslang-suggestions#1368

Fixes: #17209 , #17240

• Allow ParsedHashDirectives to take non string arguments
• Support LangVersion flag
• Add integer arguments for nowarn
• Allow nowarn to be prefixed with FS
• #nowarn tests
• Allow time to take non string argument (on/off)
• #time tests
• #r tests

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Warning

No PR link found in some release notes, please consider adding it.

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/8.0.400.md	No current pull request URL (#17206) found, please consider adding it
LanguageFeatures.fsi	docs/release-notes/.Language/preview.md	No current pull request URL (#17206) found, please consider adding it

[abelbraaksma]

Fixes: #17209

did you (also) mean #17240? It is mentioned in the preview.md release notes. And this closed suggestion: fsharp/fslang-suggestions#1364.

Considering this is a language change now (if I understand it correctly #nowarn "12" and #nowarn 12 are now both allowed, same for #time "on" and #time on), I propose we (I mean: I don't mind doing it) write an RFC for this. I don't mean to slow-down progress, but otherwise we won't catch this in the F# spec effort.

[abelbraaksma]

I really like this change, just a few questions:

[abelbraaksma]

Why are FSBLAH and "FSBLAH", and ACME and "ACME" not an error? Conversely, if these aren't errors, why is FS and "FS" an error?

Just tested it with latest RTM, has this always been an error?

and even this:

this is so odd...

[KevinRansom]

Deleted the original because I was spouting garbage.

replaced with:
Okay, I looked a bit harder, it is as you would suspect "by design"; back in the olden days, I.e pre-dotnet SDK "ACME" would have originally failed, however, because in the dotnet SDK different build targets use
<NoWarn></NoWarn> for disabling warnings

For example:

• C# uses CS, F# uses FS, Nuget uses NU.

Probably it would have been smarter to filter that out in the build target, but we missed that opportunity, so now a valid nowarn value is:
FSn or n - where n is any valid 32bit decimal integer or XXXX
And any other value is ignored, because it could be targeted at a different build target.

[abelbraaksma]

Considering the long ident change, does this mean #r "System.Core" and #r System.Core are now both allowed?

EDIT: checking your referenced issues, the answer is: yes 👍. I've tried to capture all these changes here: fsharp/fslang-suggestions#1368.

[KevinRansom]

Considering the long ident change, does this mean #r "System.Core" and #r System.Core are now both allowed?

Sigh!!!!, I didn't intend for that to work.
#I and #r and #r nuget
require :, \ and / which means we would have to teach the parser paths and URIs.

[KevinRansom]

@abelbraaksma huge thanks for taking care of the fslang suggestion, I miss having an active PM to keep me on the true path of righteousness.

[abelbraaksma]

I miss having an active PM to keep me on the true path of righteousness.

You're too kind!!! Are you offering me job? 😆jk. But seriously, no worries, I'm happy to help :).

require :, \ and / which means we would have to teach the parser paths and URIs.

Not entirely. It could just be that simple paths that are in scope are allowed without quotes, anything else needs quotes. Just like you don't need quotes on the commandline, unless you have spaces in the file names.

But we can decide that in the lang suggestion, of course.

Another way for this change is that you keep the information after you parse it. That way, it is not that all hash directives can now understand all kinds of arguments. That way, you can pick and choose per directive.

[abelbraaksma]

Some suggestions, nits below:

[psfinaki]

Marking this as waiting for RFC then. Otherwise LGTM :)

[edgarfgp]

Now with #17140 merged. Looking forward to have this in 👍🏻

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

Petr said it's fine

[nojaf]

Nitpick: this should have had some tests in https://github.com/dotnet/fsharp/tree/main/tests/service/data/SyntaxTree/ParsedHashDirective

[Martin521]

@KevinRansom
I have a question regarding this PR, specifically about this function, which I will need to touch for my current work on "scoped #nowarn".

1. Do we really need the feature flag here? The only incompatibility scenario that I see here without that flag is the following:

   • Somebody has "illegally" used the new feature already, say #nowarn FS0025.
   • The directive was silently swallowed by earlier compilers, with no effect.
   • The file contains a match expression that creates a warning FS0025.
   • They somehow rely on that warning being emitted.
   • They now switch to the F# 9.0 compiler, with language version, say, 6.0.
   • The warning is suppressed.

   This scenario (specifically the point about relying on a warning in legacy code) seems to be so unlikely that I wonder if we can remove the flag in this function.

2. Should we still want to safeguard against such a scenario, then we should, for the String case, pass the flag as last argument to the GetWarningNumber function.

I am not sure if I am overlooking something, I am therefore reaching out for advice. When I touch the function anyway, should I consider 1 or 2 above?