Skip to content

Conversation

edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Jul 8, 2024

Follow up: #17207

Unions compile down to a class or struct sharplab.

This PR enforces the new rules on unions.

Old rules

  • AttributeTargets.Class
  • AttributeTargets.Interface
  • AttributeTargets.Delegate
  • AttributeTargets.Struct
  • AttributeTargets.Enum

New Rules

  • AttributeTargets.Class
  • AttributeTargets.Struct when using [<Struct>]

Before sharplab

open System

[<AttributeUsage(AttributeTargets.Class)>]
type ClassTargetAttribute() =
     inherit Attribute()

[<AttributeUsage(AttributeTargets.Interface)>]
type InterfaceTargetAttribute() =
     inherit Attribute()

[<AttributeUsage(AttributeTargets.Struct)>]
type StructTargetAttribute() =
     inherit Attribute()

[<InterfaceTarget>] // Does not fail. It should
[<StructTarget>] // Does not fail. It should
[<ClassTarget>]
type Union =
    | Case1
    
[<InterfaceTarget>] // Does not fail. It should
[<StructTarget>] // Does not fail. It should
[<ClassTarget>]
type Union2 =
    | Case1 of string * int
    | Case2 of string

[<ClassTarget>] // Does not fail. It should
[<InterfaceTarget>] // Does not fail. It should
[<Struct>]
type StructUnion =
    | Case1
  
[<ClassTarget>] // Does not fail. It should
[<InterfaceTarget>] // Does not fail. It should
[<Struct>]
type StructUnion2 =
    | Case1 of a: string * int
    | Case2 of string * b: string

After

open System

[<AttributeUsage(AttributeTargets.Class)>]
type ClassTargetAttribute() =
     inherit Attribute()

[<AttributeUsage(AttributeTargets.Interface)>]
type InterfaceTargetAttribute() =
     inherit Attribute()

[<AttributeUsage(AttributeTargets.Struct)>]
type StructTargetAttribute() =
     inherit Attribute()

[<InterfaceTarget>] // Fails as expected
[<StructTarget>]// Fails as expected
[<ClassTarget>]
type Union =
    | Case1
    
[<InterfaceTarget>] // Fails as expected
[<StructTarget>] // Fails as expected
[<ClassTarget>]
type Union2 =
    | Case1 of string * int
    | Case2 of string

[<ClassTarget>]// Fails as expected
[<InterfaceTarget>] // Fails as expected
[<Struct>]
type StructUnion =
    | Case1
  
[<ClassTarget>] // Fails as expected
[<InterfaceTarget>] // Fails as expected
[<Struct>]
type StructUnion2 =
    | Case1 of a: string * int
    | Case2 of string * b: string

Checklist

  • Test cases added
  • Release notes entry updated

Copy link
Contributor

github-actions bot commented Jul 8, 2024

❗ 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/9.0.100.md

@edgarfgp edgarfgp marked this pull request as ready for review July 8, 2024 15:17
@edgarfgp edgarfgp requested a review from a team as a code owner July 8, 2024 15:17
@edgarfgp
Copy link
Contributor Author

edgarfgp commented Jul 8, 2024

This is ready.

Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wish we had this proportion of added code and tests in all PRs :D
Thanks Edgar!

@edgarfgp
Copy link
Contributor Author

Can I get another review please ?

@psfinaki
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@vzarytovskii
Copy link
Member

Just a sanity check, type E = A = 1 | B = 2 will still have target enum, right?

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Jul 16, 2024

Just a sanity check, type E = A = 1 | B = 2 will still have target enum, right?

@vzarytovskii Yes. There are dedicated tests that proves that

@edgarfgp
Copy link
Contributor Author

Any chance to get this merged before the nullable PR ?

@vzarytovskii
Copy link
Member

vzarytovskii commented Jul 17, 2024

Any chance to get this merged before the nullable PR ?

Uh oh, I didn't notice the comment before I've pressed merge button, sorry :(

@edgarfgp
Copy link
Contributor Author

Ok,this is green again

@edgarfgp edgarfgp closed this Jul 20, 2024
@edgarfgp edgarfgp reopened this Jul 20, 2024
@edgarfgp edgarfgp closed this Jul 20, 2024
@edgarfgp edgarfgp reopened this Jul 20, 2024
@psfinaki
Copy link
Contributor

Thanks @edgarfgp, merging now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants