Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attribute targets on unions #17389

Merged
merged 4 commits into from
Jul 22, 2024
Merged

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
Member

@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
Member

/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 edgarfgp force-pushed the attribute-targets-on-unions branch from 20c1180 to 7fddd57 Compare July 18, 2024 18:55
@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
Member

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