-
Notifications
You must be signed in to change notification settings - Fork 795
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
Enable EnforceAttributeTargets for F# 9.0 #17558
Conversation
❗ Release notes required
|
#17559 will help with the failing tests regarding |
@edgarfgp I don't really like this fix at all, it was more like an experiment that I got excited about because it was in the evening :-) |
Yeah the symbols duplication is a phenomenon that is proven difficult to diagnose. So thanks for looking into this. |
Depends on: #17559 for CliMutable issues. |
I investigated about the special attributes( 1 error expected[<AttributeUsage (AttributeTargets.Class ||| AttributeTargets.Struct, AllowMultiple=false)>]
[<Sealed>]
type CLIMutableAttribute() =
inherit Attribute()
[<CLIMutable>] // This is compiled to class and it is part the attribute targets
type BadClass() = member x.P = 1
^^^^^^^^
// This type definition may not have the 'CLIMutable' attribute. Only record types may have this attribute.
[<CLIMutable>] // This is compiled to class and it is part the attribute targets
type BadUnion = A | B
^^^^^^^^
// This type definition may not have the 'CLIMutable' attribute. Only record types may have this attribute.
2 errors expected[<CLIMutable>] // This is compile to an interface and it is not part of the attribute targets
^^^^^^^^^^^^
// This attribute is not valid for use on this language element
type BadInterface = interface end
^^^^^^^^^^
// This type definition may not have the 'CLIMutable' attribute. Only record types may have this attribute.
I see two options:
|
Some form of special treatment must remain anyway, as the attributes work on the F# type system, which is tronger than the IL type system enforced via attribute targets. That being said, I think both outcomes are fine - those are two different diagnostics and I do not see a big deal in having both of them in case of wrong usage. |
@edgarfgp In the long run one code issue, one error is ideal. But two is okay, there are plenty of other places where the compiler over complains. So the redundent errors will not be out of place. |
Ok. So Im going to raise a PR today for |
@edgarfgp -- I'm going to wrap this pr up, get it passing. We can address duplicated messages as separate PR's making targeted fixes. |
Fix 17514: #17514
Initially disabled because it over reported name resolutions. The fix is to disable name resolution when verifying attribute applications. This works because the name resolutions for attributes had been reported earlier.