Generic Attributes #17258

mflibby · 2024-05-28T04:01:09Z

Description

Intention:

The intent of this PR is to implement Generic Attribute support, e.g.:

type ARecordAttribute<^T>()=
    inherit Attribute()


type AFieldAttribute<^T>()=
    inherit Attribute()

[<ARecord<int>>]
type SomeRecord =
    {
        [<AField<string>>]
        someField : string
    }

Checklist

Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated:

github-actions · 2024-05-28T04:01:45Z

❗ 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.400.md

LanguageFeatures.fsi docs/release-notes/.Language/preview.md

mflibby · 2024-05-28T04:06:24Z

Adding of test cases, and fullfilment of minutia (documentation, various "feature" tags, general cleanup, etc) still WIP.

Currently, [<SomeAttribute<int>()>] compiles down to the expected IL. I plan to explore what will be required for inferencing before marking as ready.

Drafting now while I complete the minor details in case anyone wants to provide feedback in the meantime.

…ign with apparent standard

…SecurityAttribute (ILxGen)

mflibby · 2024-06-01T10:40:35Z

@dotnet-policy-service agree company="Hillcrest Research and Development LLC"

abelbraaksma · 2024-06-01T12:25:01Z

Hi @mflibby, this looks very interesting! In your original description, can you link to the reported issue that this is fixing? And if it is a language change (i.e., a new feature that is being added), can you link to the RFC (in https://github.com/fsharp/fslang-design) and the language proposal or discussion chat (https://github.com/fsharp/fslang-suggestions)?

If none exist, can you update the OP with a summary of what this change is doing and why?

mflibby · 2024-06-02T13:05:59Z

@abelbraaksma I've updated the description with the desired information. Please let me know if this is satisfactory, or if more information will be helpful :)

mflibby · 2024-06-02T13:11:58Z

I'm not sure why, but commit 64607ef passed all checks, yet after a few cleanup/a minor change/merges to other changes on main, a large number of the checks have been failing (the only substantive change I made since the passing commit was a minor change to the LexFilter that allowed the type arguments immediately adjacent to the close of an attribute list to be properly parsed).

I'm wondering why I am all of a sudden getting a bunch of seemingly unrelated tests failing? Additionally, windows defender seems to be flagging a bunch of tests, and occasionally quarantining innocuous tests like "Conformance/../E_Literals02.fs".

The investigation continues.

T-Gro · 2024-06-03T11:02:42Z

tests/FSharp.Compiler.ComponentTests/EmittedIL/Attribute/Attribute.fs

+
+module Attribute =
+
+    let verifyCompilation compilation =


Could you please add test cases(s) with mutual recursion (either via and or via a rec module) between the attribute declaration, the type argument and the type it is being applied on?

Do we have a particular point of failure we are looking for here? Off the top of my head this is what comes to mind:
(in a module rec)

type MyAttribute<^T>() = inherit Attribute() [<MyAttribute<AnActualType>>] let x = 1 type AnActualType = class end

(Note: it appears that in a rec module, the usage of an attribute on a binding needs to occur after the definition, but on a type it can occur before the definition.)
and maybe something like

(in a regular module)

[<MyAttribute<AnActualType>>] type SomeClass(arg) = member _.AGoodProperty = arg and MyAttribute<^T>() = inherit Attribute() and AnActualType = int

Oh great call, both rec and and fail right now (made sure to check they pass when there are no type args and both do)

Glad I could suggest a point of failure - I did not have a particular one in mind, I just know that rec modules are a common point of failure for new typechecking features.

Maybe also a type applying "itself" as the type argument to the attribute?

[<MyAttribute<SomeClass<SomeClass<SomeClass<MyAttribute>>>>>] type SomeClass<'a>(arg) =

I still need to figure out why the rec/and cases failed, but I implemented the self referencing case (great case btw, I know there must be some cases where it will be useful but this in particular made me laugh quite hard), and was surprised to find it working right out of the gate!

abelbraaksma · 2024-06-03T12:19:30Z

I've updated the description with the desired information. Please let me know if this is satisfactory, or if more information will be helpful :)

Hey @mflibby, excellent, thanks, now we know where it's coming from and that it was approved 👍. We still need an RFC, though. I may be able to help with that. Language changes require an RFC for documentation and consensus. They capture the what, how and scope of any new feature. The go here: https://github.com/fsharp/fslang-design. It is also used to set up a discussion thread to deal with any unresolved questions.

I can help with the process.

tests/FSharp.Compiler.ComponentTests/EmittedIL/Attribute/Attribute.fs

mflibby · 2024-06-04T00:30:37Z

@abelbraaksma Oh well the order in which I've done things certainly should make drawing up the RFC easier, I will get on that ASAP - thank you for the heads up!

…ting to suit

auduchinok

@mflibby Thanks for this!

Could we please have more tests for various error cases?

Please also add tests for cases where type inference is important, like the one from the RFC:

type MyAttribute<^T>(context : ^T)=
  inherit Attribute()

[<MyAttribute(24)>]
let x = 1

auduchinok · 2024-06-11T10:09:20Z

src/Compiler/TypedTree/TypedTreePickle.fs

@@ -2252,7 +2258,7 @@ and u_attribkind st =

 and u_attrib st : Attrib =
    let a, b, c, d, e, f = u_tup6 u_tcref u_attribkind (u_list u_attrib_expr) (u_list u_attrib_arg) u_bool u_dummy_range st
-    Attrib(a, b, c, d, e, None, f)  // AttributeTargets are not preserved
+    Attrib(a, b, [], c, d, e, None, f)  // AttributeTargets are not preserved (u_list u_ty)


What will happen if an older compiler sees metadata containing type arguments?

If I understand your question correctly, this relates to a situation where in an assembly that actually uses generic attributes (i.e. actually applying attributes to "things") is referenced by another assembly being built by an older version of fsc. In that case, I think the type arguments of used attributes will need to be digested into the "TyConRef" field of the typed Attrib; i.e., it would need to become a new type definition?

Sorry, I've read this change wrong. Not keeping the data is sad, but at the same time changing the metadata format is probably a no-go here for now.

Would that kind of change just be another RFC/PR, or are there other complexities that need to be dealt with?

The problem is we try to maintain the metadata compatibility with the older compilers, and currently there's no preferred way to add new data to it without breaking existing compilers.

@vzarytovskii Do you know if there been any changes in this direction?

The precedence we used to take before:

Absolutely maintain backwards compatibility for existing source code (i.e. code not using the feature)

Have a compiler error if this is used in FSharp.Core

New code using this feature will force consumers of such code to upgrade their tooling (IDE,SDK).

There is an alternative approach of not breaking the pickle format and instead loading the needed info from .NET IL metadata ;; but I do not think this path has been taken before (even though it would be safer from a compatibility perspective, because we do not need to know how to load it for interop scenarios anyway)

I would assume that the purpose of the pickled resources is for the sake of performance improvements (correct me if I'm wrong), in which case I would think that there are some kind of benchmarks concerning the gain of using pickling as opposed to loading the same resources from the IL? Assuming that the cost in this case wouldn't be great, and that it is feasible, loading the info from the IL and not breaking backward compatibility sounds like the better option.

src/Compiler/lex.fsl

auduchinok · 2024-06-11T10:14:27Z

src/Compiler/pars.fsy

@@ -1506,26 +1506,52 @@ attributeListElements:


 /* One custom attribute */
-attribute:
+attribute:     


Would it be possible to add recovery for unfinished attributes? For example, consider typing a new attribute and then adding type arguments to it. We want the parser (and the rest of the analysis) to be able to parse the rest of the file while the attribute is being typed, if that is possible with the language grammar.

[<Attr<>] ()

[<Attr<>>] ()

[<Attr<arg>] ()

[<Attr<_>] ()

[<Attr<arg; Attr2>] ()

[<Attr<(LiteralArg)>] ()

[<Attr< LiteralArg>] ()

I'm not entirely certain how this would be done at the moment, but I will begin playing the idea now.

There is a complication with the lexer and typars being immediately adjacent to a closing >] that I think might end up being an obstacle in this case - I include in case I am missing something and someone can enlighten me on the inner workings on the lexer:
In order to allow for the closing >] to be immediately adjacent to a typar, e.g.:

[<Attr<_>>]

We need to deal with the fact that the lexer is going to collect the whole series of greater-thans. Based on my current understanding of how the lexer works, I think the only place we can deal with this is in the LexFilterImpl.peekAdjacentTypars function in Compiler\SyntaxTree\LexFilter.fs, in particular when the stack is being constructed via scan ahead (I assume the scan ahead section is the best place as we need to know that a ] is immediately adjacent in order to be sure we can smash the last gt off the end of what the lexer collects).

tests/service/data/SyntaxTree/Attribute/RangeOfAttribute.fs

mflibby · 2024-06-20T07:51:27Z

src/Compiler/AbstractIL/ilwrite.fs

@@ -353,7 +353,7 @@ type UnsharedRow(elems: RowElement[]) =
 type ILTypeWriterEnv = { EnclosingTyparCount: int }
 let envForTypeDef (tdef: ILTypeDef) = { EnclosingTyparCount=tdef.GenericParams.Length }
 let envForMethodRef env (ty: ILType) = { EnclosingTyparCount=(match ty with ILType.Array _ -> env.EnclosingTyparCount | _ -> ty.GenericArgs.Length) }
-let envForNonGenericMethodRef _mref = { EnclosingTyparCount=Int32.MaxValue }
+//let envForNonGenericMethodRef _mref = { EnclosingTyparCount=Int32.MaxValue } do we need this for attributes? It doesn't appear so


I realize it's generally a "no-no" to do this to obscure functionality (i.e. I dont yet understand what this was for), but it seems that with the introduction of generic attributes, we will need to fully qualify the environment for attribute usage. Since this function was only used in the generation of attributes, my current position is that this function can be deleted - though I am still curious why the count was being maxed for the non-generic case, if anyone has that wisdom on hand.

vzarytovskii · 2024-06-24T17:47:14Z

The problem is we try to maintain the metadata compatibility with the older compilers, and currently there's no preferred way to add new data to it without breaking existing compilers.

@vzarytovskii Do you know if there been any changes in this direction?

Rule of thumb is usually:

Don't break it if possible ^(duh). Problem with this is that we don't really have a mechanism of merging pickled data with IL metadata (in this case - merging FSharpAttribute with ILAttribute) as well as not all the information we would like to have, is there (like aliases, or type measures for example), which, as a result will get lost cross-assembly.
If breaking change is necessary - make it explicit - i.e. only pickle new data when library author explicitly opted in to use that feature, and put the feature under language flag and don't use it in FSharp.Core (for example - let bindings in types, or, in this case, generic attributes).

We have one more case - when we introduced another resource for nullability data, it has multiple problems unfortunately:

We don't have standard mechanisms for managing those, code for handling every single one has to be literally copy-pasted, which kinda makes it prone to errors.
It turns out to be suboptimal - a lot of additional excessive data - a bunch of new tables, instead of just new bit/type somewhere.
It needs to be taken care of in the trimming config files every time it's introduced.

We probably want to think of such mechanism in future, but it will require a very thorough design and will be a huge chunk of work.

I would assume that the purpose of the pickled resources is for the sake of performance improvements (correct me if I'm wrong)i n which case I would think that there are some kind of benchmarks concerning the gain of using pickling as opposed to loading the same resources from the IL?

Not necessary, main purpose is to have/store additional metadata, which we want to use cross-assembly (mostly). I don't really think it will affect perf significatly.

Assuming that the cost in this case wouldn't be great, and that it is feasible, loading the info from the IL and not breaking backward compatibility sounds like the better option.

Yeah, but please, refer to part of this comment below - not all the info we would like to have is in the IL metadata.

TL;DR - I would personally go with second point in the beginning of my comment if possible. We have done it before. We will likely do it in future. And at the moment we, unfortunately, don't have a better mechanism for it.

cc @T-Gro @dsyme

mflibby added 8 commits May 19, 2024 11:51

basic parsing implementation of generic attribute type parameters

c6a11c4

Merge remote-tracking branch 'upstream' into main

80f8ed8

generalized basic parsing for type parameters on attributes

cd39370

Merge branch 'main' into hcrd_main

8093188

threading type parameters into type tree

0064634

Merge branch 'main' into hcrd_main

10c7d7e

fully threaded base implementation of generic attributes

cdb0974

Merge branch 'main' into hcrd_main

c611fb6

mflibby added 10 commits May 28, 2024 00:57

updating originally named TypeParam to TypeArg (for attr types) to al…

799ad54

…ign with apparent standard

updated with language feature and reverted modified test cases

d9ac04e

updated release notes

e545b49

formatting fixes

64607ef

more tests for ensuring appropriate type arg behavior in attributes

45281ef

Merge remote-tracking branch 'upstream' into hcrd_main

b24ffe5

fixed type level generic attributes not being digested properly in Is…

796a4ba

…SecurityAttribute (ILxGen)

fixed lexing issue where type args adjacent to >] clashed

97c326e

cleanup

6566460

Merge branch 'main' into hcrd_main

d81d3d3

mflibby marked this pull request as ready for review June 1, 2024 10:41

mflibby requested a review from a team as a code owner June 1, 2024 10:41

added missing baseline for new ILEmission test

d6c846e

more cleanup

cdb2b4e

T-Gro reviewed Jun 3, 2024

View reviewed changes

tests/FSharp.Compiler.ComponentTests/EmittedIL/Attribute/Attribute.fs Show resolved Hide resolved

mflibby added 2 commits June 6, 2024 22:26

Merge branch 'main' into hcrd_main

e40ed04

fully implemented type args into other production cases and added tes…

24d679a

…ting to suit

auduchinok suggested changes Jun 11, 2024

View reviewed changes

mflibby added 3 commits June 13, 2024 03:56

revert irrelevant formatting change

1e886be

formatting adjustment to avoid unnecessary diff

47c5dc7

added alias generic test

993d1fe

mflibby commented Jun 20, 2024

View reviewed changes

added self reference generic attribute usage test

4255a3f

abonie requested a review from vzarytovskii June 24, 2024 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic Attributes #17258

Generic Attributes #17258

mflibby commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

mflibby commented May 28, 2024 •

edited

Loading

mflibby commented Jun 1, 2024

abelbraaksma commented Jun 1, 2024

mflibby commented Jun 2, 2024

mflibby commented Jun 2, 2024

T-Gro Jun 3, 2024

mflibby Jun 7, 2024 •

edited

Loading

mflibby Jun 7, 2024

T-Gro Jun 7, 2024

mflibby Jun 23, 2024

abelbraaksma commented Jun 3, 2024

mflibby commented Jun 4, 2024

auduchinok left a comment •

edited

Loading

auduchinok Jun 11, 2024

mflibby Jun 13, 2024

auduchinok Jun 14, 2024

mflibby Jun 18, 2024

auduchinok Jun 18, 2024 •

edited

Loading

T-Gro Jun 18, 2024

mflibby Jun 20, 2024

auduchinok Jun 11, 2024 •

edited

Loading

mflibby Jun 13, 2024

mflibby Jun 20, 2024

vzarytovskii commented Jun 24, 2024

Generic Attributes #17258

Are you sure you want to change the base?

Generic Attributes #17258

Conversation

mflibby commented May 28, 2024 • edited Loading

Description

Intention:

Checklist

github-actions bot commented May 28, 2024 • edited Loading

❗ Release notes required

mflibby commented May 28, 2024 • edited Loading

mflibby commented Jun 1, 2024

abelbraaksma commented Jun 1, 2024

mflibby commented Jun 2, 2024

mflibby commented Jun 2, 2024

Choose a reason for hiding this comment

mflibby Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abelbraaksma commented Jun 3, 2024

mflibby commented Jun 4, 2024

auduchinok left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auduchinok Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auduchinok Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzarytovskii commented Jun 24, 2024

mflibby commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

mflibby commented May 28, 2024 •

edited

Loading

mflibby Jun 7, 2024 •

edited

Loading

auduchinok left a comment •

edited

Loading

auduchinok Jun 18, 2024 •

edited

Loading

auduchinok Jun 11, 2024 •

edited

Loading