Enforce attribute targets on functions

[edgarfgp]

Description

Fixes#8547

Currently the compiler does not enforce AttributeTargets in values and function bindings

Before

openSystem[<AttributeUsage(AttributeTargets.Method)>]typeMethodOnlyAttribute()=inherit System.Attribute()[<MethodOnly>]// Compiles as expected letsomeFunction()="abc"[<MethodOnly>]// CompilesletsomeValue="def"[<AttributeUsage(AttributeTargets.Field)>]typeFieldOnlyAttribute()=inherit System.Attribute()[<FieldOnly>]// Compiles letsomeFunction()="abc"[<FieldOnly>]// CompilesletsomeValue="def"

After

openSystem[<AttributeUsage(AttributeTargets.Method)>]typeMethodOnlyAttribute()=inherit System.Attribute()[<MethodOnly>]// Compiles as expectedletsomeFunction()="abc"[<MethodOnly>]// Does not compile as expectedletsomeValue="def"[<AttributeUsage(AttributeTargets.Field)>]typeFieldOnlyAttribute()=inherit System.Attribute()[<FieldOnly>]// Does not compile as expectedletsomeFunction()="abc"[<FieldOnly>]// Compiles as expectedletsomeValue="def"

Important

AttributeTargets are not covered by the lang spec The new rules are based on me reading document and using sharplab to see what is the produced code in C#

Old Rules

• Values and Function bindings were both using AttributeTargets.Field ||| AttributeTargets.Method ||| AttributeTargets.Property ||| AttributeTargets.ReturnValue

New Rules

• Values : AttributeTargets.Field ||| AttributeTargets.Property ||| AttributeTargets.ReturnValue
• Function bindings: AttributeTargets.Method ||| AttributeTargets.ReturnValue

Links

• AttributeTargets
• AttribueTargets.Field SharpLab
• AttributeTargets.Method SharpLab

Note: this PR only covers module and class let bindings for now. Will create follow up PR for members, properties etc

Checklist

• Test cases added

• Performance benchmarks added in case of performance changes

• Release notes entry updated:

  Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

  Release notes files:

  • If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
  • If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/releae-notes/.Language/preview.md
  • If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.

  Information about the release notes entries format can be found in the documentation.
  Example:

  • More inlines for Result module. (PR #16106)
  • Correctly handle assembly imports with public key token of 0 length. (Issue #16359, PR #16363)
    *while! (Language suggestion #1038, PR #14238)

  If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
LanguageFeatures.fsi	docs/release-notes/.Language/preview.md

[edgarfgp]

This is ready

[vzarytovskii]

Could you please add a small description of the new rules? Like

• Method target:
  • Before: methods, let bindings
  • After: methods, let bindings for functions
    etc, if it's not too much of a hassle.

Will help discoverability of the change + when F# 9 release notes will be prepared.

[edgarfgp]

Could you please add a small description of the new rules? Like

Yeah I was working on this. Updated the PR description.

Please check the tests and sharplab links and let me know if we can add more test cases

[psfinaki]

Nice, useful and clear work, thanks for the good PR description, testing and splitting changes into multiple PRs.

As for the TailCall, yeah I think we should at least put the check behind the flag. Maybe we can have even a separate message specifically for the TailCall case although that does sound a bit ad-hoc.

[edgarfgp]

Nice, useful and clear work, thanks for the good PR description, testing and splitting changes into multiple PRs.

As for the TailCall, yeah I think we should at least put the check behind the flag. Maybe we can have even a separate message specifically for the TailCall case although that does sound a bit ad-hoc.

@vzarytovskii@psfinaki with this PR and checking for the language version flag. We will get

• F# 8 single warning The TailCall attribute should only be applied to recursive functions. as it is now
• F# 9 a waring like previous version + the error for AttributeTargets.

Currently we have 2 error number and messages for enforcing the right AttributeTarget in let bindings

• 3861,chkTailCallAttrOnNonRec,"The TailCall attribute should only be applied to recursive functions."
• 842,tcAttributeIsNotValidForLanguageElement,"This attribute is not valid for use on this language element"

My proposal would be to in F#9 use in both cases 842(Potentially improve the error message to be more friendly and omit / replace 3861

The most used scenario for this ie when you use a testing library like Xunit and you use the wrong target your tests silently be ignored

moduleMyTestsopenXunit[<Fact>]let``This test will be ignored`` = Assert.True(false)[<Fact>let``This test will not be ignored`` ()= Assert.True(false)

Even the compiler had couple tests suffering from this See #16610

Edit: Xunit related issue xunit/xunit#2064

[vzarytovskii]

Yeah, that works, thank!

[psfinaki]

Oh actually if we get rid of that thing with tests, it would be awesome, I've stumbled on that myself multiple times :)

[edgarfgp]

@auduchinok FYI more AttributeTargets changes