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

Enable EnforceAttributeTargets for F# 9.0 #17558

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Conversation

KevinRansom
Copy link
Member

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.

@KevinRansom KevinRansom requested a review from a team as a code owner August 18, 2024 06:16
Copy link
Contributor

github-actions bot commented Aug 18, 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
Copy link
Contributor

edgarfgp commented Aug 18, 2024

#17559 will help with the failing tests regarding CLIMutable

@KevinRansom
Copy link
Member Author

@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 :-)

@edgarfgp
Copy link
Contributor

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

@KevinRansom
Copy link
Member Author

Depends on: #17559 for CliMutable issues.

@edgarfgp
Copy link
Contributor

Depends on: #17559 for CliMutable issues.

I investigated about the special attributes(CLIMutable, AllowMullLiteral etc.) changes where we doubled up on error messages and I think two errors are to be expected. e.g.

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:

  • We show 2 errors to properly enforce the attribute targets in the cases.
  • We show 1 error as before and keep the special treatment for these special attributes. One caveat is that the EnforceAttributeTargets won't be triggered until the special attribute is not longer used on the type.

cc @KevinRansom @T-Gro

@T-Gro
Copy link
Member

T-Gro commented Aug 19, 2024

  • We show 1 error as before and keep the special treatment for these special attributes. One caveat is that the EnforceAttributeTargets won't be triggered until the special attribute is not longer used on the type.

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.

@KevinRansom
Copy link
Member Author

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

@edgarfgp
Copy link
Contributor

@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 AllowNullLiteral to only show 1 error as we did for CLIMutableAttribute in my previous PR and will let you decide in this PR how many errors you want to show :)

@KevinRansom
Copy link
Member Author

@edgarfgp -- I'm going to wrap this pr up, get it passing. We can address duplicated messages as separate PR's making targeted fixes.

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.

5 participants