Skip to content

Make attribute target mismatch a warning and not an error. #18492

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+129 −139
Open

Make attribute target mismatch a warning and not an error. #18492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1799aaf
make attribute targets mismatches a warning and not an error.
edgarfgp Apr 23, 2025
55507e9
release notes
edgarfgp Apr 23, 2025
1738018
update tests
edgarfgp Apr 23, 2025
65f5bb6
Merge branch 'main' into fix-attr-targets
edgarfgp Apr 24, 2025
0c97b9d
Merge branch 'main' into fix-attr-targets
edgarfgp Apr 27, 2025
6f2b706
update baselines
edgarfgp Apr 29, 2025
e8f1bb0
Merge branch 'main' into fix-attr-targets
edgarfgp Apr 29, 2025
75d8f5e
Update baselines
edgarfgp Apr 29, 2025
4f2e97e
Merge branch 'fix-attr-targets' of github.com:edgarfgp/fsharp into fi…
edgarfgp Apr 29, 2025
63be5d5
Merge branch 'main' into fix-attr-targets
edgarfgp Apr 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/release-notes/.FSharp.Compiler.Service/9.0.300.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
* Warning for "useless null handling" works with piped syntax constructs now ([PR #18331](https://github.com/dotnet/fsharp/pull/18331))
* Make indent in generated overridden member code depend on the context, not fix to 4. ([PR #18341](https://github.com/dotnet/fsharp/pull/18341))
* Adjust caller info attribute error message range ([PR #18388](https://github.com/dotnet/fsharp/pull/18388))
* Make attribute targets mismatch a warning and not an error ([PR #18492](https://github.com/dotnet/fsharp/pull/18492))

### Breaking Changes
* Struct unions with overlapping fields now generate mappings needed for reading via reflection ([Issue #18121](https://github.com/dotnet/fsharp/issues/17797), [PR #18274](https://github.com/dotnet/fsharp/pull/17877))
2 changes: 1 addition & 1 deletion src/Compiler/Checking/Expressions/CheckExpressions.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11367,7 +11367,7 @@ and TcAttributeEx canFail (cenv: cenv) (env: TcEnv) attrTgt attrEx (synAttr: Syn
if (directedTgts = AttributeTargets.Assembly || directedTgts = AttributeTargets.Module) then
error(Error(FSComp.SR.tcAttributeIsNotValidForLanguageElementUseDo(), mAttr))
else
error(Error(FSComp.SR.tcAttributeIsNotValidForLanguageElement(), mAttr))
warning(Error(FSComp.SR.tcAttributeIsNotValidForLanguageElement(), mAttr))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still want to address the fact that for union cases and popular serialization libraries using [DataMember], there is not much people can do.

For the particular scenario of having attribute on a union case like here : #18298 , and admitting this is a common scenario and does not crash at runtime, we should avoid producing warnings against which users cannot taky any action.

If this scenario would be coverage with a separate warning that would also be opt-in and not opt-out, we would not trigger hard to explain warning by mere updates of the SDK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More specifically, I think the scenario would be a Property-targeting attribute used on a DU case no matter the DU-layout chosen.
(if the layout happens to be a property as well, all good. If it isn't, raise a new separate opt-in warning)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was planning to add a new warning(off by default) in a separate PR to address the attribute on a union case with fields.


match ResolveObjectConstructor cenv.nameResolver env.DisplayEnv mAttr ad ty with
| Exception _ when canFail = TcCanFail.IgnoreAllErrors || canFail = TcCanFail.IgnoreMemberResoutionError -> [ ], true
Expand Down
215 changes: 108 additions & 107 deletions ...tTests/Conformance/BasicGrammarElements/CustomAttributes/AttributeUsage/AttributeUsage.fs
View file
Open in desktop

Large diffs are not rendered by default.

7 changes: 4 additions & 3 deletions ....Compiler.ComponentTests/Conformance/BasicGrammarElements/CustomAttributes/Basic/Basic.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ module CustomAttributes_Basic =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 842, Line 15, Col 7, Line 15, Col 17, "This attribute is not valid for use on this language element")
(Warning 842, Line 15, Col 7, Line 15, Col 17, "This attribute is not valid for use on this language element")
]

// SOURCE=E_AttributeApplication04.fs SCFLAGS="--test:ErrorRanges" # E_AttributeApplication04.fs
Expand All @@ -71,7 +71,7 @@ module CustomAttributes_Basic =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 842, Line 14, Col 3, Line 14, Col 13, "This attribute is not valid for use on this language element")
(Warning 842, Line 14, Col 3, Line 14, Col 13, "This attribute is not valid for use on this language element")
]

// SOURCE=E_AttributeApplication05.fs SCFLAGS="--test:ErrorRanges" # E_AttributeApplication05.fs
Expand All @@ -81,7 +81,8 @@ module CustomAttributes_Basic =
|> verifyCompile
|> shouldFail
|> withDiagnostics [
(Error 842, Line 8, Col 7, Line 8, Col 8, "This attribute is not valid for use on this language element")
(Warning 842, Line 8, Col 7, Line 8, Col 8, "This attribute is not valid for use on this language element")
(Error 824, Line 8, Col 7, Line 8, Col 8, "Attributes are not permitted on 'let' bindings in expressions")
(Warning 20, Line 8, Col 1, Line 8, Col 31, "The result of this expression has type 'int' and is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.")
]

Expand Down
2 changes: 1 addition & 1 deletion .../FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/EntryPoint/EntryPoint.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ module EntryPoint =
|> compile
|> shouldFail
|> withDiagnostics [
(Error 842, Line 9, Col 3, Line 9, Col 13, """This attribute is not valid for use on this language element""")
(Warning 842, Line 9, Col 3, Line 9, Col 13, """This attribute is not valid for use on this language element""")
]

// SOURCE=E_twoattributesonsamefunction001.fs SCFLAGS="--test:ErrorRanges" # E_twoattributesonsamefunction001.fs
Expand Down
4 changes: 2 additions & 2 deletions ...Sharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/LetBindings/Basic/Basic.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ module LetBindings_Basic =
|> shouldFail
|> withDiagnostics [
(Error 683, Line 14, Col 6, Line 14, Col 27, "Attributes are not allowed within patterns")
(Error 842, Line 14, Col 8, Line 14, Col 25, "This attribute is not valid for use on this language element")
(Warning 842, Line 14, Col 8, Line 14, Col 25, "This attribute is not valid for use on this language element")
(Error 683, Line 14, Col 42, Line 14, Col 63, "Attributes are not allowed within patterns")
(Error 842, Line 14, Col 44, Line 14, Col 61, "This attribute is not valid for use on this language element")
(Warning 842, Line 14, Col 44, Line 14, Col 61, "This attribute is not valid for use on this language element")
]

// SOURCE=E_ErrorsForInlineValue.fs SCFLAGS="--test:ErrorRanges" # E_ErrorsForInlineValue.fs
Expand Down
32 changes: 9 additions & 23 deletions tests/FSharp.Compiler.ComponentTests/ErrorMessages/TailCallAttribute.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ namespace N

[<FSharp.Test.FactForNETCOREAPP>]
let ``Warn successfully for invalid tailcalls in type methods`` () =
"""
FSharp """
namespace N
module M =
Expand All @@ -261,29 +261,12 @@ namespace N
printfn "M2 called"
this.M1() + 2 // should warn
"""
|> FSharp
|> withLangVersion80
|> compile
|> shouldFail
|> withResults [
{ Error = Warning 3569
Range = { StartLine = 10
StartColumn = 17
EndLine = 10
EndColumn = 26 }
Message =
"The member or function 'M2' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way." }
{ Error = Warning 3569
Range = { StartLine = 15
StartColumn = 17
EndLine = 15
EndColumn = 26 }
Message =
#if Debug
"The member or function 'M2' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way." }
#else
"The member or function 'M1' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way." }
#endif
|> withDiagnostics [
(Warning 3569, Line 10, Col 17, Line 10, Col 26, "The member or function 'M2' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way.");
(Warning 3569, Line 15, Col 17, Line 15, Col 26, "The member or function 'M1' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way.")
]

[<FSharp.Test.FactForNETCOREAPP>]
Expand Down Expand Up @@ -1481,7 +1464,10 @@ namespace N
|> withLangVersionPreview
|> compile
|> shouldFail
|> withSingleDiagnostic (Error 842, Line 6, Col 11, Line 6, Col 19, "This attribute is not valid for use on this language element")
|> withDiagnostics [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there another error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe warning allow the analysis to continue as opposed to error which just stop the analysis. ?

(Warning 842, Line 6, Col 11, Line 6, Col 19, "This attribute is not valid for use on this language element")
(Warning 3861, Line 7, Col 13, Line 7, Col 18, "The TailCall attribute should only be applied to recursive functions.")
]

[<FSharp.Test.FactForNETCOREAPP>]
let ``Error about attribute on recursive let-bound value`` () =
Expand All @@ -1497,7 +1483,7 @@ namespace N
|> withLangVersionPreview
|> compile
|> shouldFail
|> withSingleDiagnostic (Error 842, Line 6, Col 11, Line 6, Col 19, "This attribute is not valid for use on this language element")
|> withSingleDiagnostic (Warning 842, Line 6, Col 11, Line 6, Col 19, "This attribute is not valid for use on this language element")

[<FSharp.Test.FactForNETCOREAPP>]
let ``Warn about self-defined attribute`` () = // is the analysis available for users of older FSharp.Core versions
Expand Down
1 change: 1 addition & 0 deletions tests/FSharp.Compiler.Service.Tests/PatternMatchCompilationTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ match () with
dumpDiagnostics checkResults |> shouldEqual [
"(3,2--3,25): Attributes are not allowed within patterns"
"(3,4--3,23): This attribute is not valid for use on this language element"
"(3,17--3,22): This is not a valid constant expression or custom attribute value"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we getting a new error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe warning allow the analysis to continue as opposed to error which just stop the analysis. ?

]


Expand Down
2 changes: 1 addition & 1 deletion tests/fsharp/Compiler/Language/StructActivePatternTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ let (|Foo|_|) x = ValueNone
[<Struct>]
let (|Foo|_|) x = ValueNone
"""
[|(FSharpDiagnosticSeverity.Error, 842, (2, 3, 2, 9),
[|(FSharpDiagnosticSeverity.Warning, 842, (2, 3, 2, 9),
"This attribute is not valid for use on this language element");
(FSharpDiagnosticSeverity.Error, 3350, (3, 6, 3, 13),
"Feature 'Boolean-returning and return-type-directed partial active patterns' is not available in F# 8.0. Please use language version 9.0 or greater.")|]
Expand Down
2 changes: 1 addition & 1 deletion tests/fsharpqa/Source/Conformance/UnitsOfMeasure/WithOOP/E_GenericUOM01.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// #Regression #Conformance #UnitsOfMeasure #ObjectOrientedTypes
// Verify error when putting invalid attributes on type arguments
// (We should only allow [<Measure>].)
//<Expects id="FS0842" status="error" span="(8,36)">This attribute is not valid for use on this language element</Expects>
//<Expects id="FS0842" status="warning" span="(8,36)">This attribute is not valid for use on this language element</Expects>

open System

Expand Down