Enforce union case declarations AttributeTargets #16764

edgarfgp · 2024-02-26T16:23:01Z

Description

Fixes #3112

UnionCase compiles down to a static Method so should only enforce AttributeTargets.Method SharpLab

Old Rule

AttributeTargets.Method ||| AttributeTargets.Property

New Rule

AttributeTargets.Method

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 · 2024-02-26T16:23:43Z

❗ 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

….com:edgarfgp/fsharp into enforce-attribute-targets-on-union-case-decl

edgarfgp · 2024-02-27T05:54:09Z

This is ready

src/Compiler/Checking/CheckExpressions.fs

psfinaki

Thanks for the fix, tests and refactoring. That's a good one.

I wonder if we should add some notes to some docs about it. This is a technically correct behavior but might seem unintuitive to F# newcomers.

edgarfgp · 2024-02-27T14:25:07Z

Thanks for the fix, tests and refactoring. That's a good one.

I wonder if we should add some notes to some docs about it. This is a technically correct behavior but might seem unintuitive to F# newcomers.

Im planning to submit a RFC with the AttributeTargets rules once Im done with #13526 to make it clear

psfinaki · 2024-02-27T14:50:40Z

Awesome :)

edgarfgp · 2024-02-29T17:02:28Z

This PR should merged before #16790. Thanks

psfinaki · 2024-02-29T17:48:27Z

@dotnet/fsharp-team-msft

src/Compiler/Checking/CheckExpressions.fs

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

vzarytovskii · 2024-03-01T12:15:55Z

UnionCase compiles down to a static Method so should only enforce AttributeTargets.Method SharpLab

Thought it's a breaking change, I think it's fine from the the "correctness" point of view, however having AttributeTargets.Method working feels weird, since DU constructors being "methods" in the IL is an implementation detail in F# (not in C#).

If something is using generated attributes, they can be "correctly" utilised in C#, but not in F#, since from F# standpoint, New* methods are nonexistent, and cases are treated as constructors, if that makes sense.

That said, I don't mind merging it as it is now, but it still feels weird to me that we allow Method and not Constructor.

edgarfgp · 2024-03-01T12:23:42Z

That said, I don't mind merging it as it is now, but it still feels weird to me that we allow Method and not Constructor.

@vzarytovskii As you know there not an actual spec yet for this, so if Constructor make sense from the F# perspective we can add it

* Real accessibility (#15484) * merge * remove unused binding * temp * temp * temp * temp * temp * temp * fantomas * temp * temp * quotes * temp * realsig build and test * tuples, staticint tests * SerializableAttribute tests * SeqExpressionStepping * AsyncExpressionStepping * misc * AttributeTargets * CCtorDUWithMember ListExpressionStepping * temp * cleanup * fantomas * temp * temp * temp * Automated command ran: fantomas Co-authored-by: KevinRansom <5175830+KevinRansom@users.noreply.github.com> * Some cleanup * clean * fantoms * temp * merge issues * fantomas * temp * Update src/Compiler/TypedTree/TypedTreeBasics.fs Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> * Update src/Compiler/Optimize/Optimizer.fs Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> * inline * Fix plain build. * Update changelog * Fixed release notes * feedback * remove surplus realsigs * Update src/Compiler/TypedTree/TypedTree.fsi Co-authored-by: Petr Pokorny <petr@innit.cz> * Update tests/FSharp.Compiler.ComponentTests/EmittedIL/ComputationExpressions/ComputationExpressions.fs Co-authored-by: Petr Pokorny <petr@innit.cz> * baselines * baselines * baselines * build.sh * restore quotes * moar quotes * mutable police * fantomas * t * Update baselines * Shadowing/LinqCount.fsx baseline * Shadowing lingcount * access.fsx --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> Co-authored-by: Petr Pokorny <petr@innit.cz> * Fix range start of INTERP_STRING_PART (#16785) * use the same mechanism we used to fix the range start of INTERP_STRING_END to also fix the range start of INTERP_STRING_PART * - use a new rule for '"}" +' to catch the correct range start - clean up work around structures introduced before and not needed anymore with this * Revert "- use a new rule for '"}" +' to catch the correct range start" This reverts commit 4d01cda. * add second PR to changelog * remove commented poc code * Enforce union case declarations AttributeTargets (#16764) * Enforce union-cases AttributeTargets * release notes * LanguageFeature.EnforceAttributeTargetsUnionCaseDeclarations * release notes * format code * improve naming * Update src/Compiler/Checking/CheckExpressions.fs Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> * Fix merge conflict --------- Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> * Don't consider parse warnings as errors in ComputeTcIntermediate (#16792) * Fix seqexpression testcases (#16795) * correct realsignature test cases for seqexpr tests * rename typo --------- Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> Co-authored-by: Petr Pokorny <petr@innit.cz> Co-authored-by: dawe <dawedawe@posteo.de> Co-authored-by: Edgar Gonzalez <edgar.gonzalez@fundourselves.com> Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> Co-authored-by: Florian Verdonck <florian.verdonck@outlook.com>

auduchinok · 2024-03-04T15:33:05Z

UnionCase compiles down to a static Method

This is not always the case. There's no single direct mapping between union cases and their compiled .NET representations: depending on the union and its union cases, they compile to different things (nested classes, properties, constructor methods, case test properties, nested Tags enum and so on).

For example, these union cases compile to properties, not methods:

type U =
    | A
    | B

I don't see any tests covering these cases.

Is there an RFC for this?

edgarfgp · 2024-03-04T15:39:10Z

UnionCase compiles down to a static Method

This is not always the case. There's no single direct mapping between union cases and their compiled .NET representations: depending on the union and its union cases, they compile to different things (nested classes, properties, constructor methods, case test properties, nested Tags enum and so on).

For example, these union cases compile to properties, not methods:
type U =
    | A
    | B
I don't see any tests covering these cases.

Is there an RFC for this?

@auduchinok No Yet. Im going through the issues and figuring out which AttributeTargets should be enforced. Will create a RFC at some point as soon I have a clear picture of this.

This is not always the case. There's no single direct mapping between union cases and their compiled .NET representations: depending on the union and its union cases, they compile to different things (nested classes, properties, constructor methods, case test properties, nested Tags enum and so on).

I will look into this as create a PR with tests

T-Gro · 2024-03-05T07:19:44Z

src/Compiler/Checking/CheckDeclarations.fs

+        let attrs =
+            // The attributes of a union case decl get attached to the generated "static factory" method
+            // Enforce that the union-cases can only be targeted by attributes with AttributeTargets.Method
+            if g.langVersion.SupportsFeature(LanguageFeature.EnforceAttributeTargetsUnionCaseDeclarations) then


Following the logic from IlxGen, this should branch on arguments for the case.
Case with no data is a property.
Case with fields is a method.

There are other concerns affecting DU IL generation (structness, null as true value, number of cases), but so far I have not found any other affecting case-level attribute placement beyond what I wrote above (and what Eugene pointed out).

Thanks for the review. See #16807

If you find any other case. Let me know, I really want to improve this area of the compiler

Enforce union-cases AttributeTargets

54b00ab

edgarfgp added 4 commits February 26, 2024 19:06

Merge branch 'main' into enforce-attribute-targets-on-union-case-decl

eb37d50

release notes

fbcedc4

LanguageFeature.EnforceAttributeTargetsUnionCaseDeclarations

e3d7c8f

Merge branch 'enforce-attribute-targets-on-union-case-decl' of github…

4670669

….com:edgarfgp/fsharp into enforce-attribute-targets-on-union-case-decl

edgarfgp changed the title ~~Enforce union-cases AttributeTargets~~ Enforce union case declarations AttributeTargets Feb 26, 2024

edgarfgp added 2 commits February 26, 2024 20:46

release notes

75ef665

format code

bdd617f

edgarfgp marked this pull request as ready for review February 27, 2024 05:53

edgarfgp requested a review from a team as a code owner February 27, 2024 05:53

psfinaki reviewed Feb 27, 2024

View reviewed changes

src/Compiler/Checking/CheckExpressions.fs Outdated Show resolved Hide resolved

improve naming

b478166

psfinaki approved these changes Feb 27, 2024

View reviewed changes

edgarfgp added 2 commits February 27, 2024 15:58

Merge branch 'main' into enforce-attribute-targets-on-union-case-decl

965f3b3

Merge branch 'main' into enforce-attribute-targets-on-union-case-decl

f53381e

edgarfgp requested a review from vzarytovskii February 28, 2024 12:02

edgarfgp added 3 commits February 28, 2024 13:42

Merge branch 'main' into enforce-attribute-targets-on-union-case-decl

50e2f28

Merge branch 'main' into enforce-attribute-targets-on-union-case-decl

c79162c

Merge branch 'main' into enforce-attribute-targets-on-union-case-decl

5b4c13b

brianrourkeboll reviewed Feb 29, 2024

View reviewed changes

src/Compiler/Checking/CheckExpressions.fs Outdated Show resolved Hide resolved

edgarfgp and others added 3 commits February 29, 2024 19:05

Update src/Compiler/Checking/CheckExpressions.fs

7fe37a1

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

Merge branch 'main' into enforce-attribute-targets-on-union-case-decl

8ce9d7b

Fix merge conflict

80cbd2a

vzarytovskii approved these changes Mar 1, 2024

View reviewed changes

vzarytovskii merged commit 111a1f8 into dotnet:main Mar 1, 2024
31 checks passed

edgarfgp deleted the enforce-attribute-targets-on-union-case-decl branch March 2, 2024 13:02

T-Gro reviewed Mar 5, 2024

View reviewed changes

dotnet-bot mentioned this pull request Mar 12, 2024

[Automated] PRs inserted in VS build main-34712.16 #16864

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce union case declarations AttributeTargets #16764

Enforce union case declarations AttributeTargets #16764

edgarfgp commented Feb 26, 2024 •

edited

github-actions bot commented Feb 26, 2024 •

edited

edgarfgp commented Feb 27, 2024

psfinaki left a comment

edgarfgp commented Feb 27, 2024

psfinaki commented Feb 27, 2024

edgarfgp commented Feb 29, 2024 •

edited

psfinaki commented Feb 29, 2024

vzarytovskii commented Mar 1, 2024

edgarfgp commented Mar 1, 2024

auduchinok commented Mar 4, 2024

edgarfgp commented Mar 4, 2024

T-Gro Mar 5, 2024

edgarfgp Mar 5, 2024

edgarfgp Mar 5, 2024

Enforce union case declarations AttributeTargets #16764

Enforce union case declarations AttributeTargets #16764

Conversation

edgarfgp commented Feb 26, 2024 • edited

Description

Old Rule

New Rule

Checklist

github-actions bot commented Feb 26, 2024 • edited

❗ Release notes required

edgarfgp commented Feb 27, 2024

psfinaki left a comment

Choose a reason for hiding this comment

edgarfgp commented Feb 27, 2024

psfinaki commented Feb 27, 2024

edgarfgp commented Feb 29, 2024 • edited

psfinaki commented Feb 29, 2024

vzarytovskii commented Mar 1, 2024

edgarfgp commented Mar 1, 2024

auduchinok commented Mar 4, 2024

edgarfgp commented Mar 4, 2024

T-Gro Mar 5, 2024

Choose a reason for hiding this comment

edgarfgp Mar 5, 2024

Choose a reason for hiding this comment

edgarfgp Mar 5, 2024

Choose a reason for hiding this comment

edgarfgp commented Feb 26, 2024 •

edited

github-actions bot commented Feb 26, 2024 •

edited

edgarfgp commented Feb 29, 2024 •

edited