Skip to content

Attribute targets on unions#17389

Merged
psfinaki merged 4 commits into
dotnet:mainfrom
edgarfgp:attribute-targets-on-unions
Jul 22, 2024
Merged

Attribute targets on unions#17389
psfinaki merged 4 commits into
dotnet:mainfrom
edgarfgp:attribute-targets-on-unions

Conversation

@edgarfgp

@edgarfgp edgarfgp commented Jul 8, 2024

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented Jul 8, 2024

Copy link
Copy Markdown
Contributor

❗ 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

edgarfgp commented Jul 8, 2024

Copy link
Copy Markdown
Contributor Author

This is ready.

@psfinaki psfinaki left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

Can I get another review please ?

@psfinaki

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@vzarytovskii

Copy link
Copy Markdown
Member

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

@edgarfgp

edgarfgp commented Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

Any chance to get this merged before the nullable PR ?

@vzarytovskii

vzarytovskii commented Jul 17, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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