From 334698223702c2b3a4b30f957783969302bcf41d Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Tue, 28 Nov 2023 14:53:58 +0000 Subject: [PATCH 1/9] Raise a new error if we try to have auto properties on constructor-less types --- src/Compiler/Checking/CheckDeclarations.fs | 21 +++++++-- .../Diagnostics/Records.fs | 19 +++++++- .../ErrorMessages/ClassesTests.fs | 43 +++++++++++++++++++ 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index 54fd5a756a4..f188f596320 100644 --- a/src/Compiler/Checking/CheckDeclarations.fs +++ b/src/Compiler/Checking/CheckDeclarations.fs @@ -4209,9 +4209,6 @@ module TcDeclarations = | SynMemberDefn.LetBindings (range=m) :: _ -> errorR(Error(FSComp.SR.tcTypeDefinitionsWithImplicitConstructionMustHaveLocalBindingsBeforeMembers(), m)) | _ -> () - - - /// Split auto-properties into 'let' and 'member' bindings let private SplitAutoProps members = let membersIncludingAutoProps, vals_Inherits_Abstractslots = @@ -4330,6 +4327,16 @@ module TcDeclarations = match trepr with | SynTypeDefnRepr.ObjectModel(kind, members, m) -> let members = desugarGetSetMembers members + let implementedAutoProperties = + members + |> List.choose ( + fun mem -> + match mem with + | SynMemberDefn.Interface (members= Some defs) -> + let autoProps = defs |> List.filter(isAutoProperty) + Some autoProps + | _ -> None) + |> List.concat CheckMembersForm members @@ -4371,6 +4378,14 @@ module TcDeclarations = members |> List.tryPick (function | SynMemberDefn.ImplicitCtor (ctorArgs = SynSimplePats.SimplePats _ as spats) -> Some spats | _ -> None) + + // Raise a new error if we try to have an interface implementation with auto properties on constructor-less types, + if not implementedAutoProperties.IsEmpty then + for mem in implementedAutoProperties do + match mem with + | SynMemberDefn.AutoProperty(range = m) when implicitCtorSynPats.IsNone -> + errorR(Error(FSComp.SR.tcAutoPropertyRequiresImplicitConstructionSequence(), m)) + | _ -> () // An ugly bit of code to pre-determine if a type has a nullary constructor, prior to establishing the // members of the type diff --git a/tests/FSharp.Compiler.ComponentTests/Diagnostics/Records.fs b/tests/FSharp.Compiler.ComponentTests/Diagnostics/Records.fs index 70dcd710e34..ac317fb8c26 100644 --- a/tests/FSharp.Compiler.ComponentTests/Diagnostics/Records.fs +++ b/tests/FSharp.Compiler.ComponentTests/Diagnostics/Records.fs @@ -91,4 +91,21 @@ let t3 (x: RecTy) (a: AnotherNestedRecTy) = { x with D.C.c = { a with A = 3; B = |> withDiagnostics [ (Warning 3560, Line 9, Col 26, Line 9, Col 65, "This copy-and-update record expression changes all fields of record type 'Test.NestdRecTy'. Consider using the record construction syntax instead.") (Warning 3560, Line 15, Col 62, Line 15, Col 85, "This copy-and-update record expression changes all fields of record type 'Test.AnotherNestedRecTy'. Consider using the record construction syntax instead.") - ] \ No newline at end of file + ] + +[] +let ``Error when implementing interface with auto property in record type``() = + FSharp """ +type Foo = + abstract member X : string with get, set + +type FooImpl = + { name: string } + interface Foo with + member val X = "" with get, set + """ + |> withLangVersion80 + |> asExe + |> compile + |> shouldFail + |> withSingleDiagnostic (Error 912, Line 8, Col 5, Line 8, Col 36, "This declaration element is not permitted in an augmentation") \ No newline at end of file diff --git a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs index 41a672132be..1225941736f 100644 --- a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs @@ -704,4 +704,47 @@ type X = abstract member Bar<'T> : string -> unit """ |> typecheck + |> shouldSucceed + + [] + let ``Error if we try to have auto properties on constructor-less types`` () = + Fsx """ +type Foo = + abstract member X : string with get, set + abstract member Y : string with get, set + abstract member Z : string with get, set + +type FooImpl = + interface Foo with + member val X = "" with get, set + member val Y = "" with get, set + member this.Z + with get() = "" + and set(value) = () + """ + |> asExe + |> compile + |> shouldFail + |> withDiagnostics [ + (Error 3133, Line 9, Col 9, Line 9, Col 40, "'member val' definitions are only permitted in types with a primary constructor. Consider adding arguments to your type definition, e.g. 'type X(args) = ...'."); + (Error 3133, Line 10, Col 9, Line 10, Col 40, "'member val' definitions are only permitted in types with a primary constructor. Consider adding arguments to your type definition, e.g. 'type X(args) = ...'.") + ] + + [] + let ``No error if we try to have auto properties on types with primary constructor`` () = + Fsx """ +type Foo = + abstract member X : string with get, set + abstract member Y : string with get, set + abstract member Z : string with get, set + +type FooImpl() = + interface Foo with + member val X = "" with get, set + member val Y = "" with get, set + member this.Z + with get() = "" + and set(value) = () + """ + |> typecheck |> shouldSucceed \ No newline at end of file From 52926e221f1d1adb6bc6fa3f3c1701b8e06988ae Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Tue, 28 Nov 2023 16:01:09 +0000 Subject: [PATCH 2/9] Update release notes --- docs/release-notes/FSharp.Compiler.Service/8.0.200.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/release-notes/FSharp.Compiler.Service/8.0.200.md b/docs/release-notes/FSharp.Compiler.Service/8.0.200.md index 33e81141302..20b70eaefbf 100644 --- a/docs/release-notes/FSharp.Compiler.Service/8.0.200.md +++ b/docs/release-notes/FSharp.Compiler.Service/8.0.200.md @@ -1 +1,2 @@ -- Miscellaneous fixes to parens analysis - https://github.com/dotnet/fsharp/pull/16262 \ No newline at end of file +- Miscellaneous fixes to parens analysis - https://github.com/dotnet/fsharp/pull/16262 +- Raise a new error when interfaces with auto properties are implemented on constructor-less types - https://github.com/dotnet/fsharp/pull/16352 \ No newline at end of file From 78f2266a99fe5062a3a00422b32928fde6b16a33 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Wed, 29 Nov 2023 09:59:43 +0000 Subject: [PATCH 3/9] improve record test --- tests/FSharp.Compiler.ComponentTests/Diagnostics/Records.fs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/FSharp.Compiler.ComponentTests/Diagnostics/Records.fs b/tests/FSharp.Compiler.ComponentTests/Diagnostics/Records.fs index ac317fb8c26..3529af9e247 100644 --- a/tests/FSharp.Compiler.ComponentTests/Diagnostics/Records.fs +++ b/tests/FSharp.Compiler.ComponentTests/Diagnostics/Records.fs @@ -98,14 +98,16 @@ let ``Error when implementing interface with auto property in record type``() = FSharp """ type Foo = abstract member X : string with get, set + abstract GetValue: unit -> string type FooImpl = { name: string } interface Foo with member val X = "" with get, set + member x.GetValue() = x.name """ |> withLangVersion80 |> asExe |> compile |> shouldFail - |> withSingleDiagnostic (Error 912, Line 8, Col 5, Line 8, Col 36, "This declaration element is not permitted in an augmentation") \ No newline at end of file + |> withSingleDiagnostic (Error 912, Line 9, Col 5, Line 9, Col 36, "This declaration element is not permitted in an augmentation") \ No newline at end of file From b81a7d4e98d71c47ffc809f76217906b580d6d45 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Thu, 30 Nov 2023 11:09:54 +0000 Subject: [PATCH 4/9] Update src/Compiler/Checking/CheckDeclarations.fs Co-authored-by: Tomas Grosup --- src/Compiler/Checking/CheckDeclarations.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index d2ea5a878c9..da0b0f4669a 100644 --- a/src/Compiler/Checking/CheckDeclarations.fs +++ b/src/Compiler/Checking/CheckDeclarations.fs @@ -4398,7 +4398,7 @@ module TcDeclarations = if not implementedAutoProperties.IsEmpty then for mem in implementedAutoProperties do match mem with - | SynMemberDefn.AutoProperty(range = m) when implicitCtorSynPats.IsNone -> + | SynMemberDefn.AutoProperty( isStatic = false; range = m) when implicitCtorSynPats.IsNone -> errorR(Error(FSComp.SR.tcAutoPropertyRequiresImplicitConstructionSequence(), m)) | _ -> () From b2887811d24d7868cb1fa7174261bcc6beeaf864 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Thu, 30 Nov 2023 11:10:01 +0000 Subject: [PATCH 5/9] Update src/Compiler/Checking/CheckDeclarations.fs Co-authored-by: Tomas Grosup --- src/Compiler/Checking/CheckDeclarations.fs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index da0b0f4669a..959ef166da5 100644 --- a/src/Compiler/Checking/CheckDeclarations.fs +++ b/src/Compiler/Checking/CheckDeclarations.fs @@ -4395,7 +4395,6 @@ module TcDeclarations = | _ -> None) // Raise a new error if we try to have an interface implementation with auto properties on constructor-less types, - if not implementedAutoProperties.IsEmpty then for mem in implementedAutoProperties do match mem with | SynMemberDefn.AutoProperty( isStatic = false; range = m) when implicitCtorSynPats.IsNone -> From 8cc8e123e15b81e7fe60432f063a04a4f2051a15 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Thu, 30 Nov 2023 11:10:13 +0000 Subject: [PATCH 6/9] Update tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs Co-authored-by: Tomas Grosup --- .../ErrorMessages/ClassesTests.fs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs index 1225941736f..1465820b185 100644 --- a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs @@ -747,4 +747,20 @@ type FooImpl() = and set(value) = () """ |> typecheck + |> shouldSucceed + + [] + let ``No error if we try to have static autoprop on a type without constructor`` () = + Fsx """ +#nowarn "3535" //We accept that static abstracts are an advanced feature +[] +type Foo = + static abstract member X : string with get, set + +[] +type FooImpl = + interface Foo with + static member val X = "" with get, set + """ + |> typecheck |> shouldSucceed \ No newline at end of file From f6e4c3cdbe9a7daf4fddecedc13ca6722ba97851 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Thu, 30 Nov 2023 11:50:51 +0000 Subject: [PATCH 7/9] Fix indentation --- src/Compiler/Checking/CheckDeclarations.fs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index 959ef166da5..5dd3d30ee50 100644 --- a/src/Compiler/Checking/CheckDeclarations.fs +++ b/src/Compiler/Checking/CheckDeclarations.fs @@ -4395,11 +4395,11 @@ module TcDeclarations = | _ -> None) // Raise a new error if we try to have an interface implementation with auto properties on constructor-less types, - for mem in implementedAutoProperties do - match mem with - | SynMemberDefn.AutoProperty( isStatic = false; range = m) when implicitCtorSynPats.IsNone -> - errorR(Error(FSComp.SR.tcAutoPropertyRequiresImplicitConstructionSequence(), m)) - | _ -> () + for mem in implementedAutoProperties do + match mem with + | SynMemberDefn.AutoProperty( isStatic = false; range = m) when implicitCtorSynPats.IsNone -> + errorR(Error(FSComp.SR.tcAutoPropertyRequiresImplicitConstructionSequence(), m)) + | _ -> () // An ugly bit of code to pre-determine if a type has a nullary constructor, prior to establishing the // members of the type From 25d0610abe3de56de1a27d3e2c32c55cfb62bf04 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Thu, 7 Dec 2023 15:04:01 +0000 Subject: [PATCH 8/9] Fix PR comments --- src/Compiler/Checking/CheckDeclarations.fs | 29 ++++++++----------- .../ErrorMessages/ClassesTests.fs | 21 +++++++++++++- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index 6aef671d7c4..56638b536a2 100644 --- a/src/Compiler/Checking/CheckDeclarations.fs +++ b/src/Compiler/Checking/CheckDeclarations.fs @@ -4202,12 +4202,24 @@ module TcDeclarations = | _ -> () | ds -> // Check for duplicated parameters in abstract methods + // Check for an interface implementation with auto properties on constructor-less types for slot in ds do if isAbstractSlot slot then match slot with | SynMemberDefn.AbstractSlot (slotSig = synVal; range = m) -> CheckDuplicatesArgNames synVal m | _ -> () + + if isInterface slot then + match slot with + | SynMemberDefn.Interface (members= Some defs) when not defs.IsEmpty -> + let autoProps = defs |> List.filter(isAutoProperty) + for au in autoProps do + match au with + | SynMemberDefn.AutoProperty(isStatic = false; range = m) -> + errorR(Error(FSComp.SR.tcAutoPropertyRequiresImplicitConstructionSequence(), m)) + | _ -> () + | _ -> () // Classic class construction let _, ds = List.takeUntil (allFalse [isMember;isAbstractSlot;isInterface;isInherit;isField;isTycon]) ds @@ -4350,16 +4362,6 @@ module TcDeclarations = match trepr with | SynTypeDefnRepr.ObjectModel(kind, members, m) -> let members = desugarGetSetMembers members - let implementedAutoProperties = - members - |> List.choose ( - fun mem -> - match mem with - | SynMemberDefn.Interface (members= Some defs) -> - let autoProps = defs |> List.filter(isAutoProperty) - Some autoProps - | _ -> None) - |> List.concat CheckMembersForm members @@ -4401,13 +4403,6 @@ module TcDeclarations = members |> List.tryPick (function | SynMemberDefn.ImplicitCtor (ctorArgs = SynSimplePats.SimplePats _ as spats) -> Some spats | _ -> None) - - // Raise a new error if we try to have an interface implementation with auto properties on constructor-less types, - for mem in implementedAutoProperties do - match mem with - | SynMemberDefn.AutoProperty( isStatic = false; range = m) when implicitCtorSynPats.IsNone -> - errorR(Error(FSComp.SR.tcAutoPropertyRequiresImplicitConstructionSequence(), m)) - | _ -> () // An ugly bit of code to pre-determine if a type has a nullary constructor, prior to establishing the // members of the type diff --git a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs index 1465820b185..5d8273748b9 100644 --- a/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/ErrorMessages/ClassesTests.fs @@ -747,7 +747,26 @@ type FooImpl() = and set(value) = () """ |> typecheck - |> shouldSucceed + |> shouldSucceed + + [] + let ``No error if we try to have auto properties on types with primary constructor with args`` () = + Fsx """ +type Foo = + abstract member X : string with get, set + abstract member Y : string with get, set + abstract member Z : string with get, set + +type FooImpl(x) = + interface Foo with + member val X = "" with get, set + member val Y = "" with get, set + member this.Z + with get() = "" + and set(value) = () + """ + |> typecheck + |> shouldSucceed [] let ``No error if we try to have static autoprop on a type without constructor`` () = From 9198d7c1b928e4acaa7d0ba5ab956a0a3d55ff26 Mon Sep 17 00:00:00 2001 From: Edgar Gonzalez Date: Thu, 7 Dec 2023 16:14:58 +0000 Subject: [PATCH 9/9] use for loop --- src/Compiler/Checking/CheckDeclarations.fs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Compiler/Checking/CheckDeclarations.fs b/src/Compiler/Checking/CheckDeclarations.fs index 56638b536a2..43dc57fdeb5 100644 --- a/src/Compiler/Checking/CheckDeclarations.fs +++ b/src/Compiler/Checking/CheckDeclarations.fs @@ -4212,14 +4212,13 @@ module TcDeclarations = if isInterface slot then match slot with - | SynMemberDefn.Interface (members= Some defs) when not defs.IsEmpty -> - let autoProps = defs |> List.filter(isAutoProperty) - for au in autoProps do + | SynMemberDefn.Interface (members= Some defs) -> + for au in defs do match au with | SynMemberDefn.AutoProperty(isStatic = false; range = m) -> errorR(Error(FSComp.SR.tcAutoPropertyRequiresImplicitConstructionSequence(), m)) | _ -> () - | _ -> () + | _ -> () // Classic class construction let _, ds = List.takeUntil (allFalse [isMember;isAbstractSlot;isInterface;isInherit;isField;isTycon]) ds